New issue
Advanced search Search tips

Issue 651623 link

Starred by 2 users

Issue metadata

Status: Duplicate
Merged: issue 671820
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug

Blocking:
issue 630357



Sign in to add a comment

Harmony - need special layout logic for pairs of buttons

Project Member Reported by shrike@chromium.org, Sep 29 2016

Issue description

Discussed with bettes@ - the basic idea is that in a lot of cases two buttons that are next to each other should have the same width. Exceptions:

* Buttons should not be narrower than the minimum button width
* Button A should not have the same width as Button B if Button B is wider than the "too wide" width

This logic will be applied to buttons at the bottom of a dialog, but also to places like the "Block" and "Remove" buttons in the Collected Cookies dialog.

This layout style needs a name, so that it can be described in the spec.

 
We can call these "paired buttons" and have a custom LayoutManager that we use for them.

Comment 2 by shrike@chromium.org, Sep 30 2016

Cc: bettes@chromium.org
Should have added bettes@. Adding him now so that he's aware we are calling these "paired buttons".

Comment 3 by shrike@chromium.org, Sep 30 2016

Labels: -Pri-3 Pri-2
Labels: Proj-HarmonyControls
Labels: -Proj-HarmonyControls Proj-HarmonyDialogs
what is the "too wide" width?

It turns out our dialogs already set a minimum size, so they follow the described behavior if "minimum" and "too wide" have the same value. (Currently that's 75dp)
Cc: kylixrd@chromium.org
Labels: -Pri-2 Pri-1
* Does the general spec describe this anywhere?  If not, can we update it to do so?
* Does this apply to all cases where we have buttons at the bottoms of dialogs, e.g. OK/Cancel?
* Does this apply to any other cases outside those?

The answers to the last two bullets determine how we should wire this in -- this will be easy to forget to do if people have to attach custom layout managers every time they want it, so ideally our "unified dialog handling framework" (I talked to kylixrd about this yesterday) will do this automatically.

P1 to make sure we have the spec up to date and code frameworks in place here in advance of actually needing to use this in a bunch of dialogs.

Comment 8 by shrike@chromium.org, Jan 27 2017

Cc: -bettes@chromium.org ellyjo...@chromium.org
Owner: bettes@chromium.org
bettes@ - we discussed this but as pkasting@ points out it's not declared anywhere in the spec. Can you add to the spec?
Cc: -kylixrd@chromium.org -ellyjo...@chromium.org tapted@chromium.org
+CC tapted, who implemented something here, and can maybe clarify for Alan what rules were used in the end, so we can ensure (a) UX agrees that's the right thing and (b) it's in the spec.
Yah - this is mostly working with r455281 (collected cookies) and r454823 (dialog client view). You should be able to try it out in Canary for all Dialogs with --secondary-ui-md flipped.

I used  Issue 671820  (I suggest closing this out as soon as we have a spec).

The logic just searches through all buttons in a row looking for the largest, then applies that size to all buttons in the row. But if a button is larger than BUTTON_MAX_LINKABLE_WIDTH it is ignored (its size doesn't change, and it is skipped when searching for the largest button).

All that's left to do is expose the constant `LayoutDelegate::Metric::BUTTON_MAX_LINKABLE_WIDTH` [1], which currently only src/chrome can see, to DialogClientView, in src/ui/views, and pop it in near this [2] code which links the column sizes (currently with no maximum).  Note the IsSecondaryUiMaterial() check won't be needed then since a BUTTON_MAX_LINKABLE_WIDTH effectively does the same thing and not linking columns.

Currently (without that), there are some corner cases like  http://crbug.com/671820#c14  which result in too-wide dialogs.

For BUTTON_MAX_LINKABLE_WIDTH, we chose kHarmonyLayoutUnit * 8   (i.e. 128).


[1] https://codereview.chromium.org/2625083003/diff/140001/chrome/browser/ui/views/collected_cookies_views.cc#newcode78
[2] https://codereview.chromium.org/2706423002/diff/380001/ui/views/window/dialog_client_view.cc#newcode393
Mergedinto: 671820
Status: Duplicate (was: Assigned)

Sign in to add a comment