New issue
Advanced search Search tips

Issue 719274 link

Starred by 1 user
Status: Fixed
Owner:
ellyjo...@chromium.org
Closed: May 2017
Cc:
tapted@chromium.org
pkasting@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Task



Sign in to add a comment

A smarter default for views::DialogDelegate::ShouldSnapFrameWidth() -> bool (and addressing dialogs with buttons that return `NONE` from DialogDelegate::GetDialogButtons())

Project Member Reported by tapted@chromium.org, May 8 2017 
Chrome Version       : 60.0.3080.5
OS Version: OS X 10.12.4

Overriding ShouldSnapFrameWidth() gives a concrete DialogDelegate control over whether it should be subject to Harmony consistent dialog-width snapping.

Some dialogs are "tooltip-like", such as the PageInfo bubble for chrome:// pages. They shouldn't have extra padding on the right. See  Issue 718696 .

Other examples:
  views::TooltipIcon -  http://crbug.com/635173#c14 
  HomePageUndoBubble -  http://crbug.com/635173#c21 

A common feature of these is that they don't have dialog buttons. So, DialogDelegate::ShouldSnapFrameWidth() could `return DialogDelegate::GetDialogButtons() != ui::DIALOG_BUTTON_NONE;`.

But there are dialogs that roll their own buttons which should snap dialog widths. (Note that dialogs with buttons that return ui::DIALOG_BUTTON_NONE from GetDialogButtons may be considered a bug.). Examples (from https://codereview.chromium.org/2863923002/#msg11)
 - AddBookmark bubble - has 3 buttons, but `Cancel` is in the `ExtraView` spot rather than in the middle.
 - The new signin flow dialogs - they have a few "sheets" that slide in with different buttons.
 - WebUI dialogs, with WebUI buttons - chromecast, print preview.
 - The TranslateBubble - has "TODO: This should be using GetDialogButtons."


Currently (in https://codereview.chromium.org/2863923002/), ShouldSnapFrameWidth() returns true by default.

Comment 1 by pkasting@chromium.org, May 8 2017

Cc: -ellyjo...@chromium.org
Owner: ellyjo...@chromium.org
Status: Assigned (was: Available)
Elly, what do you think?  It seems like making this heuristic tweak would reduce the number of cases where we need to override ShouldSnapFrameWidth().  Got a better idea?

Comment 3 by ellyjo...@chromium.org, May 10 2017 

How's this for a heuristic: if the BubbleDialogDelegateView has exactly one child view, don't snap?

That covers both cases mentioned in the initial bug that shouldn't be snapped (both have one child view, a Label) and does snap all the other mentioned dialogs in the initial bug.

I'll put up a CL implementing this shortly and we can play with it a bit.

Comment 4 by ellyjo...@chromium.org, May 10 2017 

Nope, that doesn't work. In all cases it has one child view, which is the content view, and then the content view has one or more child views of its own, but for the tooltip case, the content view is a StyledLabel which has children of its own. Back to the drawing board!

Comment 5 by pkasting@chromium.org, May 10 2017 

I was going to say the "one child" thing felt too much like "magic heuristic that happens to coincide in practice with what we want", whereas the idea of "check whether there are buttons" matches better with the actual determination we were using to decide about snapping.  Most of the exceptions are due to people rolling their own buttons, some of which we can fix and some of which it seems OK to special-case around.

Comment 7 by ellyjo...@chromium.org, May 15 2017

Status: Fixed (was: Assigned)
►

Sign in to add a comment