Harmony: fix the bookmarks bubble for Harmony Phase 1 |
|||||||||||||||||||||||||||||||||||||||||||||||||
Issue descriptionChrome Version : 60.0.3109.0 Attached is how it looks in 60.0.3109.0. Issue 651652 tracks the end goal, which does away with the Edit Bookmark dialog completely. That's too much for Harmony Phase 1. For Phase 1, we still need to: - Make buttons equal width - Move Edit to the bottom left position - align the leftmost button to the dialog margin, not the combo/textfield - Put the Cancel (remove) button on the bottom-right for Windows - Audit typography and title/content/button spacings - snap the dialog width to harmony widths And we should: - Use DialogDelegate/DialogClientView properly - i.e. GetDialogButtons(), which will get width snapping "for free" - also, e.g., for TouchBar integration I started looking, and this is going to be much more complicated because of the ios promotion - see Issue 721927 . bookmark_bubble_view.cc is not huge. One option may be to simply make an entirely new bubble and show that instead with --secondary-ui-md. The ios promotion should be its own bubble as well, which just gets shown as a widget rather than tearing out the bookmark bubble's view hierarchy while its still showing.
,
May 26 2017
first I need to mend some "broken windows" - https://codereview.chromium.org/2905243003/
,
May 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/23c963cd61e359cc18548e1c2ed6116fdfbe4fdc commit 23c963cd61e359cc18548e1c2ed6116fdfbe4fdc Author: tapted <tapted@chromium.org> Date: Sat May 27 01:55:52 2017 Reorder methods in BookmarkBubbleView. BUG= 726187 Review-Url: https://codereview.chromium.org/2904333002 Cr-Commit-Position: refs/heads/master@{#475216} [modify] https://crrev.com/23c963cd61e359cc18548e1c2ed6116fdfbe4fdc/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc [modify] https://crrev.com/23c963cd61e359cc18548e1c2ed6116fdfbe4fdc/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h
,
May 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/85c36f9e4420de87c3b5cff31d090f158a5252d0 commit 85c36f9e4420de87c3b5cff31d090f158a5252d0 Author: tapted <tapted@chromium.org> Date: Sun May 28 11:08:22 2017 Cleanup BookmarkBubbleView, remove LocationBarBubbleDelegateView::GetDialogButtons(). We want BookmarkBubbleView to be consistent with other dialogs. E.g. it should use GetDialogButtons(), but LocationBarBubbleDelegateView's override gets in the way, remove it. Most LocationBarBubbleDelegateView will not want it, but don't break them yet. Then, cleanups for BookmarkBubbleView: - Renames GetTitle() to GetBookmarkName() so it's not confused with the dialog title. - Renames close_button_ to save_button_ so it's not confused with the close [x]. - Renames bookmark_details_view_ to bookmark_contents_view_ (there's only one level of detail). Also don't use unique_ptr for a View that's also owned by the View hierarchy. - Removes some FRIEND_TEST_ALL_PREFIXES; use neater techniques. - Removes ScopedAllowIO from the browsertest and reduces boilerplate. BUG= 726187 Review-Url: https://codereview.chromium.org/2905243003 Cr-Commit-Position: refs/heads/master@{#475259} [modify] https://crrev.com/85c36f9e4420de87c3b5cff31d090f158a5252d0/chrome/browser/ui/views/autofill/save_card_bubble_views.cc [modify] https://crrev.com/85c36f9e4420de87c3b5cff31d090f158a5252d0/chrome/browser/ui/views/autofill/save_card_bubble_views.h [modify] https://crrev.com/85c36f9e4420de87c3b5cff31d090f158a5252d0/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc [modify] https://crrev.com/85c36f9e4420de87c3b5cff31d090f158a5252d0/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h [modify] https://crrev.com/85c36f9e4420de87c3b5cff31d090f158a5252d0/chrome/browser/ui/views/bookmarks/bookmark_bubble_view_browsertest.cc [modify] https://crrev.com/85c36f9e4420de87c3b5cff31d090f158a5252d0/chrome/browser/ui/views/bookmarks/bookmark_bubble_view_unittest.cc [modify] https://crrev.com/85c36f9e4420de87c3b5cff31d090f158a5252d0/chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc [modify] https://crrev.com/85c36f9e4420de87c3b5cff31d090f158a5252d0/chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h [modify] https://crrev.com/85c36f9e4420de87c3b5cff31d090f158a5252d0/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc [modify] https://crrev.com/85c36f9e4420de87c3b5cff31d090f158a5252d0/chrome/browser/ui/views/location_bar/zoom_bubble_view.h [modify] https://crrev.com/85c36f9e4420de87c3b5cff31d090f158a5252d0/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc [modify] https://crrev.com/85c36f9e4420de87c3b5cff31d090f158a5252d0/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h [modify] https://crrev.com/85c36f9e4420de87c3b5cff31d090f158a5252d0/chrome/browser/ui/views/translate/translate_bubble_view.cc [modify] https://crrev.com/85c36f9e4420de87c3b5cff31d090f158a5252d0/chrome/browser/ui/views/translate/translate_bubble_view.h
,
May 29 2017
Screenshots for https://codereview.chromium.org/2908963002/
,
May 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f9f292034311121b7777645eb48522ddaa6a5272 commit f9f292034311121b7777645eb48522ddaa6a5272 Author: tapted <tapted@chromium.org> Date: Wed May 31 06:59:30 2017 Use buttons from DialogClientView in BookmakBubbleView. Currently the bookmark bubble builds its own button row, but that's unnecessary and causes it to miss out on shared framework code for layout and button style. BUG= 726187 Review-Url: https://codereview.chromium.org/2908963002 Cr-Commit-Position: refs/heads/master@{#475833} [modify] https://crrev.com/f9f292034311121b7777645eb48522ddaa6a5272/chrome/app/bookmarks_strings.grdp [modify] https://crrev.com/f9f292034311121b7777645eb48522ddaa6a5272/chrome/app/nibs/BookmarkBubble.xib [modify] https://crrev.com/f9f292034311121b7777645eb48522ddaa6a5272/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc [modify] https://crrev.com/f9f292034311121b7777645eb48522ddaa6a5272/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h [modify] https://crrev.com/f9f292034311121b7777645eb48522ddaa6a5272/chrome/browser/ui/views/bookmarks/bookmark_bubble_view_browsertest.cc [modify] https://crrev.com/f9f292034311121b7777645eb48522ddaa6a5272/chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc [modify] https://crrev.com/f9f292034311121b7777645eb48522ddaa6a5272/chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.h
,
Aug 9 2017
I wonder whether it would be easier to just go ahead and implement bug 651652 , which looks like "Add a URL field to the dialog" (hopefully not too hard), since doing that would let us just delete the whole edit bookmark dialog instead of Harmonizing it. In either case, it's been a couple months since the last comment here, not totally sure of the status.
,
Aug 9 2017
The hard part of Issue 651652 is that the Combobox dropdown currently only shows toplevel folders. 'Choose another folder' pops the Edit Bookmark dialog to access the full tree of folders. Safari shows the full bookmark folder tree in the combobox drop down. It doesn't look too bad. It also omits editing the URL - if you want to do do that you gotta use WebUI. As for the bookmark bubble for phase 1. I think it still needs taller textfields and more spacing between labels/fields. Also colons removed from labels. This is P1, so I'll tackle that after Issue 751375 .
,
Aug 9 2017
,
Aug 9 2017
Hmm.. using the default textfield stack layout without --secondary-ui-md results in some extra spacing between the fields.. Maybe too much. Which was fine on http-auth, but is maybe too much for add bookmarks, since it's more prominent.
,
Aug 21 2017
Thanks tapted@ - do you think we're ready for a UX review for this?
,
Sep 5 2017
,
Sep 6 2017
#c11: not ready yet. I got stalled by the problem in #c10. But I've picked this up again. To resolve #c10, I think we should change DISTANCE_CONTROL_LIST_VERTICAL in *pre*-Harmony to match DISTANCE_RELATED_CONTROL_VERTICAL. Currently it is DISTANCE_*UN*RELATED_CONTROL_VERTICAL. Only the bookmark app view and http-auth are using this currently. -> https://chromium-review.googlesource.com/c/chromium/src/+/607755
,
Sep 6 2017
SGTM, just check that we don't wind up with extra space at the bottom of either of those two dialogs. It will make HTTP auth's fields closer together, which seems fine.
,
Sep 6 2017
,
Sep 6 2017
confirmed the non-harmony dialogs also affected by DISTANCE_CONTROL_LIST_VERTICAL look fine
,
Sep 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fd934b3ebbb62a9ae41f13d3821140e58871f99c commit fd934b3ebbb62a9ae41f13d3821140e58871f99c Author: Trent Apted <tapted@chromium.org> Date: Thu Sep 07 07:58:34 2017 Use the Harmony textfield stack in the add bookmark bubble. And remove colons from the field labels (both for Harmony and non-Harmony). The non-Harmony DISTANCE_CONTROL_LIST_VERTICAL also changes from 20px (DISTANCE_UNRELATED_CONTROL_VERTICAL) to 8px (DISTANCE_RELATED_CONTROL_VERTICAL). This avoids excessive vertical padding between the fields. The result is that the non-Harmony http-auth and add bookmark dialogs fields get more compact, which looks OK. This CL removes anon::UnsizedCombobox in bookmark_bubble_view.cc and relies on a GridLayout::FIXED (but stretchy) column to achieve the same result. Bug: 726187 Change-Id: I16785ab33294fd3c9c8bcc659360c955c68abcd7 Reviewed-on: https://chromium-review.googlesource.com/607755 Commit-Queue: Trent Apted <tapted@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Cr-Commit-Position: refs/heads/master@{#500242} [modify] https://crrev.com/fd934b3ebbb62a9ae41f13d3821140e58871f99c/chrome/app/bookmarks_strings.grdp [modify] https://crrev.com/fd934b3ebbb62a9ae41f13d3821140e58871f99c/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc [modify] https://crrev.com/fd934b3ebbb62a9ae41f13d3821140e58871f99c/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h [modify] https://crrev.com/fd934b3ebbb62a9ae41f13d3821140e58871f99c/chrome/browser/ui/views/harmony/chrome_layout_provider.cc [modify] https://crrev.com/fd934b3ebbb62a9ae41f13d3821140e58871f99c/chrome/browser/ui/views/harmony/textfield_layout.cc [modify] https://crrev.com/fd934b3ebbb62a9ae41f13d3821140e58871f99c/chrome/browser/ui/views/harmony/textfield_layout.h
,
Sep 13 2017
This is now ready for review - I'm not aware of any other changes that need to be made. (do we have a way of tagging that? or do we just assign to bettes@ :).
,
Sep 13 2017
Nyaaaargh. BubbleDialogDelegate uses the same margins for the footnote view as it uses for its content. "One does not simply _change_ the padding".
i.e., it's not as simple as just
set_margins(ChromeLayoutProvider::Get()->GetDialogInsetsForContentType(
views::CONTROL, views::CONTROL));
it makes the footnote all ugly.
Plan: assume footnotes are always TEXT content type. (otherwise this needs a bit more plumbing).
Cl-> https://chromium-review.googlesource.com/662883
,
Sep 13 2017
Hi tapted@, one question regarding the blue focus ring around the textfield of the name: On Mac the border of the textfield is mostly overlapped by the focus ring. In your screenshot it is exactly around it. Is this intended? Attached a screenshot how it e.g. looks in the sidebar of Safari. Thanks.
,
Sep 13 2017
"Living in harmony requires much plumbing" mehmet@ - yes, that's the intended design.
,
Sep 13 2017
Okay, thanks for your feedback 🙂
,
Sep 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4518a10bec9b0453c8c325e325d705969a07cbb8 commit 4518a10bec9b0453c8c325e325d705969a07cbb8 Author: Trent Apted <tapted@chromium.org> Date: Fri Sep 15 05:37:13 2017 Harmony: Fix the padding for the add bookmark dialog. - Use GetDialogInsetsForContentType(CONTROL, CONTROL), since the dialog starts with a textfield and ends with a combobox. - Hardcode footnote view padding in BubbleDialogDelegate. - Tweak (simplify) the bookmark bubble dialog test so it can run on Mac. Bug: 726187 Change-Id: I87bf041bb9d45a679de6c54a7d5d787e0d525c2d Reviewed-on: https://chromium-review.googlesource.com/662883 Commit-Queue: Trent Apted <tapted@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Cr-Commit-Position: refs/heads/master@{#502166} [modify] https://crrev.com/4518a10bec9b0453c8c325e325d705969a07cbb8/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc [modify] https://crrev.com/4518a10bec9b0453c8c325e325d705969a07cbb8/chrome/browser/ui/views/bookmarks/bookmark_bubble_view_browsertest.cc [modify] https://crrev.com/4518a10bec9b0453c8c325e325d705969a07cbb8/chrome/test/BUILD.gn [modify] https://crrev.com/4518a10bec9b0453c8c325e325d705969a07cbb8/ui/views/bubble/bubble_dialog_delegate.cc
,
Sep 19 2017
OK. I think all the coding is done for this and it's ready in Canary to review or play around with.
,
Sep 22 2017
In Windows 63.0.3222.0 (Official Build) canary (64-bit) the Bookmarks star icon isn't highlighted (gray bg) as a visual anchor unless you hover over it. Am I understanding correctly that the bookmark icon should always be highlighted when the dialog is active? The blocked-cookies icon as another example is always highlighted when that dialog is active. I'm wondering if the bookmarks dialog is a special case.
,
Sep 25 2017
I can't repro #c26 in 63.0.3222.0 when I click the star icon on Windows - it always gets a dark highlight for me when I click. (it gets shaded on Hover, and then gets a darker shade when I click). It sounds like a bug, and we should fix it. If I press Ctrl+d to bookmark, then there is no highlight. This is different to behaviour on Mac - Cmd+d also highlights the star there. Is that how you're summoning the dialog?
,
Sep 29 2017
The NextAction date has arrived: 2017-09-29
,
Oct 10 2017
+ we should see the grey highlight when hitting ctrl+d for all platforms + expected footer grey color: #fafafa + expected footer stroke: 1dp #000000 0.08a Question: What's the color value of the grey footer text at this moment? They seem a little darker than the expected #000 0.05
,
Oct 11 2017
Ah, I overlooked an important piece to this: all popovers (including Bookmarks) needs a close-x for all platforms.
,
Oct 12 2017
,
Oct 12 2017
Colors updated. > expected footer stroke: 1dp #000000 0.08a Did you mention 1dp because it should be 1pixel? (currently it is 1 dip - 2 pixels on retina). > What's the color value of the grey footer text at this moment? They seem a little darker than the expected #000 0.05 It's currently STYLE_SECONDARY #757575 (#000 at 0.54a) Filed Issue 773979 for the grey highlight under Ctrl+d.
,
Oct 12 2017
>> Did you mention 1dp because it should be 1pixel? (currently it is 1 dip - 2 pixels on retina). Sorry, I thought I saw 1px on retina. Keep as-is!
,
Oct 12 2017
Also, question for bettes@, what is #fafafa? it would be nice to give it a name.... but it's just replacing a constant that also doesn't have a name, and was added without more context over 4 yeargs ago - https://chromiumcodereview.appspot.com/18603006/diff/148004/chrome/browser/ui/views/bookmarks/bookmark_sync_promo_view.cc (cl for #c32 - https://chromium-review.googlesource.com/714838 )
,
Oct 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/94ea775b5bd327da68144d70353683471662de98 commit 94ea775b5bd327da68144d70353683471662de98 Author: Trent Apted <tapted@chromium.org> Date: Thu Oct 12 23:57:42 2017 Update Add Bookmark bubble for Harmony spec review #n - Add a close X - Tweak footnote container colors slightly Per http://crbug.com/726187#c29 Bug: 726187 Change-Id: I6ebb01816db9832f78976ca3786be6fff3527997 Reviewed-on: https://chromium-review.googlesource.com/714838 Reviewed-by: Bret Sepulveda <bsep@chromium.org> Commit-Queue: Trent Apted <tapted@chromium.org> Cr-Commit-Position: refs/heads/master@{#508550} [modify] https://crrev.com/94ea775b5bd327da68144d70353683471662de98/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc [modify] https://crrev.com/94ea775b5bd327da68144d70353683471662de98/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h [modify] https://crrev.com/94ea775b5bd327da68144d70353683471662de98/ui/views/bubble/bubble_frame_view.cc
,
Oct 30 2017
>> Also, question for bettes@, what is #fafafa? This should be the standard background color for all footers in secondary UI. We can call it "footer_background_color" or something to that effect?
,
Oct 30 2017
For posterity, this LGTM from the UX side. You can mark as Fixed.
,
Nov 15 2017
|
|||||||||||||||||||||||||||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||||||||||||||||||||||||||
Comment 1 by tapted@chromium.org
, May 25 2017