Harmony - implement button resize logic |
||||||||
Issue descriptionWhen there's a group of buttons in a row, the buttons should all be sized to the same width, up to some threshold. So if that threshold is MAX_WIDTH, the logic would work like this: * All buttons are sized to their ideal sizes * Buttons that are wider than MAX_WIDTH need no width adjustment and are not considered in the rest of the layout process * The remaining buttons are all sized to the width of the widest remaining button
,
Jan 11 2017
,
Jan 11 2017
Great!
,
Jan 11 2017
views::GridLayout already has the concept of equal-sized-things-laid-out-in-a-row, so I guess something like this https://codereview.chromium.org/2625083003
,
Jan 27 2017
Is comment 0 captured in the specs anywhere? I didn't see it, and if we want to do it, it would be nice (for my own reference at least) to make sure we have it in the main list with everything else. +CC bettes in hopes he can answer that.
,
Jan 27 2017
,
Feb 7 2017
,
Feb 15 2017
Plan is: plan is: 1. Convert DialogClientView to use GridLayout for its buttons and extra view (both Harmony and non-Harmony). Remove most of DialogClientView::GetPreferredSize(). "hope" nothing breaks :) - (this layout logic will probably look a bit like anonymous::AddButtonColumnSet() from https://codereview.chromium.org/2625083003) 1a. If this turns out to be awful/impossible, I'll probably go for completely separate codepaths for DialogClientView::Layout() and DialogClientView::GetPreferredSize() for pre/post Harmony. 2. Add tests for the GridLayout changes in https://codereview.chromium.org/2625083003 3. Add the logic to "pick" what columns in the DialogClientView button row should be included in GridLayout::LinkColumnSizes(..). This will need to "ask" DialogDelgate::CreateExtraView() whether the View* wants to participate in the equal-sizing logic (i.e. button=>yes, checkbox=>no). 4. Look into a bunch of dialogs with different languages/checkboxes: see if we need to move (non-button?) extra views into a separate row. 5?. Make the `Add Bookmark` bubble use DialogDelegate::GetDialogButtons() -- currently it doesn't, and does its own layout. (note the bookmark bubble effectively has its "extra" view - "Edit" - in the middle, but also the mocks remove `Edit`) - maybe we just make a brand new Add Bookmark bubble for http://crbug.com/651652
,
Feb 21 2017
I think it's going to work \o/ - https://codereview.chromium.org/2705553002/ has (1. and 3.) implemented, along with a views_example for creating dialogs so we can play with various string lengths. There's *one* failing test to investigate. I manually inspected a bunch of dialogs and they look unchanged. Now I need to split that CL. Probably into at least 3 parts..
,
Feb 22 2017
Screenshot for https://codereview.chromium.org/2701373002/
,
Feb 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b83507471b95f9fd24b95e6cc720c6e44ce92f8f commit b83507471b95f9fd24b95e6cc720c6e44ce92f8f Author: tapted <tapted@chromium.org> Date: Thu Feb 23 00:40:42 2017 Add a DialogExample example to views_examples. See http://crbug.com/671820#c10 for how it looks. This is helpful for demonstrating behaviours and experimenting with layout, e.g., for long strings. BUG= 671820 Review-Url: https://codereview.chromium.org/2701373002 Cr-Commit-Position: refs/heads/master@{#452311} [modify] https://crrev.com/b83507471b95f9fd24b95e6cc720c6e44ce92f8f/ui/views/examples/BUILD.gn [add] https://crrev.com/b83507471b95f9fd24b95e6cc720c6e44ce92f8f/ui/views/examples/dialog_example.cc [add] https://crrev.com/b83507471b95f9fd24b95e6cc720c6e44ce92f8f/ui/views/examples/dialog_example.h [modify] https://crrev.com/b83507471b95f9fd24b95e6cc720c6e44ce92f8f/ui/views/examples/examples_window.cc
,
Feb 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d9c220fe3846b32984b6f4b9ca7c43cd62edd98f commit d9c220fe3846b32984b6f4b9ca7c43cd62edd98f Author: tapted <tapted@chromium.org> Date: Thu Feb 23 01:00:07 2017 Cleanups for DialogClientView, DialogClientViewTest. This is a precursor for http://crrev.com/2706423002 with some orthogonal stuff to make review there easier. We want to use a LayoutManager for DialogClientView. This requires DCV to know about, and manage, all of its direct child views. Start by consolidating "CreateExtraView()" and other view creation into a "SetupViews()" method, and add a DCHECK to ensure client code isn't inserting random views. But. Currently a test inserts a random view to test focus traversal. To properly test focus, it needs a FocusManager, but the test harness doesn't use a Widget, so it doesn't have a FocusManager. So add a Widget to the test harness. Turns out we don't actually need a `TestDialogClientView`, or any of the protected methods in DialogClientView it was using: make them all private. BUG= 671820 Review-Url: https://codereview.chromium.org/2705553002 Cr-Commit-Position: refs/heads/master@{#452321} [modify] https://crrev.com/d9c220fe3846b32984b6f4b9ca7c43cd62edd98f/ui/views/window/dialog_client_view.cc [modify] https://crrev.com/d9c220fe3846b32984b6f4b9ca7c43cd62edd98f/ui/views/window/dialog_client_view.h [modify] https://crrev.com/d9c220fe3846b32984b6f4b9ca7c43cd62edd98f/ui/views/window/dialog_client_view_unittest.cc
,
Feb 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/23e39591317d71645dda43792fadd858bdf892ab commit 23e39591317d71645dda43792fadd858bdf892ab Author: tapted <tapted@chromium.org> Date: Mon Feb 27 22:43:39 2017 Only perform dialog Layout() once during Widget::Init(). For Widgets that require a non-client view Layout is currently performed twice during Widget::Init(): - once before the initial bounds of the Widget are determined (when setting the RootView's ContentsView), and - once after (when setting the initial bounds). The first Layout() is unnecessary and can lead to bugs when the RootView attempts a layout at an invalid size. Layout of text in particular can be affected since giving it a size influences text wrapping, and subsequent answers for Label::GetPreferredSize() will change. In particular, on Mac and Linux, zero-sized windows are not valid, so a NativeWidget may take on a size of 1x1 until its initial bounds is determined. If RootView performs a layout at this size, it can "lock" a Label to an non-preferred, but non-zero, size. This causes Label (and gfx::RenderText) to report a new preferred size of 0x0 (since no text will fit). So when Widget initialization later tries to determine what the initial bounds of the Widget should be, the Label is not accounted for. Keeping the Label with a 0x0 size instead causes Label::GetPreferredSize() to report a meaningful preferred size in more cases. But also layout is expensive and shouldn't be done multiple times whenever a Widget is created if we can avoid it. BUG= 671820 Review-Url: https://codereview.chromium.org/2712383002 Cr-Commit-Position: refs/heads/master@{#453365} [modify] https://crrev.com/23e39591317d71645dda43792fadd858bdf892ab/ui/views/widget/root_view.cc [modify] https://crrev.com/23e39591317d71645dda43792fadd858bdf892ab/ui/views/widget/root_view_unittest.cc [modify] https://crrev.com/23e39591317d71645dda43792fadd858bdf892ab/ui/views/widget/widget.cc
,
Feb 28 2017
Right, this is a case where we definitely shouldn't force all three buttons to that large size.
,
Mar 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cc8ff78f49b8fe8265a15ed227ec273c08dd695a commit cc8ff78f49b8fe8265a15ed227ec273c08dd695a Author: tapted <tapted@chromium.org> Date: Mon Mar 06 04:03:05 2017 Use GridLayout for DialogClientView's button row. This allows the button sizes to be linked, which is desired for Harmony. Also GridLayout allows a lot of bespoke arithmetic to be removed from DialogClientView. It also fixes some obscure glitches that can be triggered in dialog_example.cc (e.g. failing to propagate GetMinimumSize() for resizable dialogs, and failing to reliably incorporate contents_view() height when showing non-bubble dialogs). Add a test for this: DialogClientViewTest.MinMaxPreferredSize. This would previously fail, but it wasn't terrible since dialogs with content views that really care about their size constraints tend to be WebUI which don't have buttons anyway (so ClientView's defaults worked OK). BUG= 671820 Review-Url: https://codereview.chromium.org/2706423002 Cr-Commit-Position: refs/heads/master@{#454823} [modify] https://crrev.com/cc8ff78f49b8fe8265a15ed227ec273c08dd695a/ui/views/examples/dialog_example.cc [modify] https://crrev.com/cc8ff78f49b8fe8265a15ed227ec273c08dd695a/ui/views/window/dialog_client_view.cc [modify] https://crrev.com/cc8ff78f49b8fe8265a15ed227ec273c08dd695a/ui/views/window/dialog_client_view.h [modify] https://crrev.com/cc8ff78f49b8fe8265a15ed227ec273c08dd695a/ui/views/window/dialog_client_view_unittest.cc
,
Mar 6 2017
,
Mar 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/81aeead4d36ae4401894ab0b059d9721a30c9521 commit 81aeead4d36ae4401894ab0b059d9721a30c9521 Author: tapted <tapted@chromium.org> Date: Tue Mar 07 23:20:45 2017 Implement Harmony-style consistent button widths for Collected Cookies. Adds a member to GridLayout's ColumnSet which imposes a threshold to ignore a column when unifying same sized columns. This also needs to be done for DialogClientView, perhaps via ViewsDelegate, which may be influenced by http://crbug.com/687349 . BUG= 671820 , 610428 TEST=GridLayoutTest.TwoColumnsLinkedSizes Review-Url: https://codereview.chromium.org/2625083003 Cr-Commit-Position: refs/heads/master@{#455281} [modify] https://crrev.com/81aeead4d36ae4401894ab0b059d9721a30c9521/chrome/browser/ui/views/collected_cookies_views.cc [modify] https://crrev.com/81aeead4d36ae4401894ab0b059d9721a30c9521/chrome/browser/ui/views/harmony/harmony_layout_delegate.cc [modify] https://crrev.com/81aeead4d36ae4401894ab0b059d9721a30c9521/chrome/browser/ui/views/harmony/layout_delegate.cc [modify] https://crrev.com/81aeead4d36ae4401894ab0b059d9721a30c9521/chrome/browser/ui/views/harmony/layout_delegate.h [modify] https://crrev.com/81aeead4d36ae4401894ab0b059d9721a30c9521/ui/views/layout/grid_layout.cc [modify] https://crrev.com/81aeead4d36ae4401894ab0b059d9721a30c9521/ui/views/layout/grid_layout.h [modify] https://crrev.com/81aeead4d36ae4401894ab0b059d9721a30c9521/ui/views/layout/grid_layout_unittest.cc
,
Apr 6 2017
From Alan: > Can we make the threshold 112? (https://codereview.chromium.org/2625083003/diff/140001/chrome/browser/ui/views/harmony/harmony_layout_delegate.cc uses kHarmonyLayoutUnit * 8 = 128). Note I'm waiting on https://codereview.chromium.org/2765883004/ to plumb through the constant to DialogDelegateView - currently only collected cookies enforces the max width. Also there's a bug to fix. I failed to realise that checkboxes are actually a subclass of LabelButton, so can currently influence the size of the other buttons. Instead I'll check whether the extra view matches the type of the buttons exactly.
,
May 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7fb6670abe4d5f8d6caa26f39bf147c3844a3ec8 commit 7fb6670abe4d5f8d6caa26f39bf147c3844a3ec8 Author: tapted <tapted@chromium.org> Date: Thu May 04 01:24:20 2017 Don't allow Checkboxes to participate in equal-sized-buttons in Harmony dialogs. Currently when a checkbox is the extra view, the checkbox tends to cause the other buttons in a dialog to get bigger. This is because Checkbox extends LabelButton, so the check using AsCustomButton() will classify it the same way. To fix, explicitly filter out Checkboxes from the equal-sizing logic in DialogClientView. BUG= 671820 Review-Url: https://codereview.chromium.org/2855623004 Cr-Commit-Position: refs/heads/master@{#469230} [modify] https://crrev.com/7fb6670abe4d5f8d6caa26f39bf147c3844a3ec8/ui/views/window/dialog_client_view.cc [modify] https://crrev.com/7fb6670abe4d5f8d6caa26f39bf147c3844a3ec8/ui/views/window/dialog_client_view_unittest.cc
,
May 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bee127cb13495a34bdf6e05c5729840c2a0a89a1 commit bee127cb13495a34bdf6e05c5729840c2a0a89a1 Author: tapted <tapted@chromium.org> Date: Mon May 08 09:04:26 2017 Harmony: Apply the upper bound on equal-sized button widths in DialogClientView. This involves moving the layout constant into views. The explicit check for IsSecondaryUiMaterial() is no longer needed since linking with an upper bound of 0 pixels has the same effect as not linking at all. Also, reduce the upper bound from 128 to 112 pixels, per http://crbug.com/671820#c19 BUG= 671820 Review-Url: https://codereview.chromium.org/2860953002 Cr-Commit-Position: refs/heads/master@{#469929} [modify] https://crrev.com/bee127cb13495a34bdf6e05c5729840c2a0a89a1/chrome/browser/ui/views/collected_cookies_views.cc [modify] https://crrev.com/bee127cb13495a34bdf6e05c5729840c2a0a89a1/chrome/browser/ui/views/harmony/chrome_layout_provider.cc [modify] https://crrev.com/bee127cb13495a34bdf6e05c5729840c2a0a89a1/chrome/browser/ui/views/harmony/chrome_layout_provider.h [modify] https://crrev.com/bee127cb13495a34bdf6e05c5729840c2a0a89a1/chrome/browser/ui/views/harmony/harmony_layout_provider.cc [modify] https://crrev.com/bee127cb13495a34bdf6e05c5729840c2a0a89a1/ui/views/BUILD.gn [modify] https://crrev.com/bee127cb13495a34bdf6e05c5729840c2a0a89a1/ui/views/bubble/bubble_frame_view_unittest.cc [modify] https://crrev.com/bee127cb13495a34bdf6e05c5729840c2a0a89a1/ui/views/layout/layout_provider.cc [modify] https://crrev.com/bee127cb13495a34bdf6e05c5729840c2a0a89a1/ui/views/layout/layout_provider.h [add] https://crrev.com/bee127cb13495a34bdf6e05c5729840c2a0a89a1/ui/views/test/test_layout_provider.cc [add] https://crrev.com/bee127cb13495a34bdf6e05c5729840c2a0a89a1/ui/views/test/test_layout_provider.h [modify] https://crrev.com/bee127cb13495a34bdf6e05c5729840c2a0a89a1/ui/views/window/dialog_client_view.cc [modify] https://crrev.com/bee127cb13495a34bdf6e05c5729840c2a0a89a1/ui/views/window/dialog_client_view_unittest.cc
,
May 8 2017
I think that's everything for this. Some dialogs roll their own button row and might need their own logic though.
,
Aug 9 2017
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by shrike@chromium.org
, Dec 14 2016Status: Assigned (was: Available)