New issue
Advanced search Search tips

Issue 702196 link

Starred by 3 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug



Sign in to add a comment

Harmony - Standardize dialogs that hand-roll their own versions of non-client components

Project Member Reported by kylixrd@chromium.org, Mar 16 2017

Issue description

Discovered during the Harmony views work, there are several bubbles/dialogs which no longer contain icons to the left of their titles. Others now do have icons where there was none. Additionally, the close button may or may not be present depending on how the dialog/bubble is presented and behaves.
 
Components: Internals>Views
Labels: OS-Chrome OS-Linux OS-Mac OS-Windows
Cc: -bsep@chromium.org
Labels: -Pri-3 -Arch-All Proj-HarmonyDialogs Pri-1
Owner: bsep@chromium.org
Status: Assigned (was: Untriaged)
Kicking this to Bret since he said he has some time.

Basically, how I conceive of this is adding some APIs to DialogDelegate to get the image resource (which may be none) and the dialog title.  The close button thing I'm not instantly sure how to handle that best complies with Hwi's close button spec; I don't know if it should actually be a "ShouldHaveCloseButton()" method here, or if we can do something better with auto-determining that based on modality, and if so where all that's controlled.

Then, just like the centralized code draws the dialog buttons at the bottom instead of each individual dialog doing it, we have the centralized code draw this stuff at the top, and it's not included in the "client area" for each dialog.

Comment 3 by bsep@chromium.org, Mar 18 2017

I can't find a convenient deck of existing dialog screenshots so I'm not sure which dialogs have had icons removed. What are they, if you know?

I also flipped through the harmony mocks and I see two places where there are supposed to be icons in Harmony: the Add to Desktop and the various incarnations of Add Extension. The user menu also kinda looks like it fits in that category but it probably has to be its own thing. Are there any others? As far as I know those both already have icons, so what are the dialogs that are gaining them?

As for the close button it'll probably useful to create the concept of modal vs non-modal, since I can imagine we'll hinge other considerations on that. For example, we already want click-away to dismiss the latter and not the former (I think). But which dialogs are which modality probably needs to be determined by a human.
Translate is an example dialog that's gaining an icon.  I think there are some dialogs that are losing icons (Allen had at least one of these cases).

My takeaway from the last eng sync is that relatively few dialogs are gaining or losing icons; mostly it's maintaining status quo.  But the status quo is currently implemented as one-offs where people explicitly add child views to their content, so it's hard to tell what the outliers are from a quick code search, or have any kind of unified rule.

Hwi might be able to help provide more systematic data here.

Comment 5 by bsep@chromium.org, Mar 18 2017

Ah, the translate dialog doesn't have an icon in the mocks I was looking at. Also thinking about it I probably don't have to worry much about dialogs losing icons, since we'll just delete the relevant code when it comes time to convert them.
That assumes we're willing to remove those icons on the pre-Harmony UI as well, which we may well be willing to do, but I don't know for sure.

Comment 7 by bsep@chromium.org, Mar 18 2017

The Install Extension dialog is a really weird one right now. It has a "content" column and an "icon" column so that the rating/users elements don't draw under the icon. It would be pretty easy to swap the icon column to the left, which would achieve what's in the mock, but that wouldn't achieve the goal of making "draw an icon next to the title" re-usable.

On the other hand I don't think the superclasses have enough control to achieve that layout. If we ignored that part of the mock then the rating/users elements wouldn't look like it's part of the title, but I think it's rare to see them in the first place so maybe we don't care. It would also be a lot more work since we'd have to overhaul that part of the layout.

The Translate bubble looks like it's getting a total redesign, so without a final mock it's hard to tell how it would fit in with this model.

So I'm a bit stuck. If we do want to overhaul Install Extension then it's probably worth making this general. But if we don't I'm worried it would end up just being a special case for Add to Desktop.
I don't know where the mock link is for the Install Extension dialog you're discussing in comment 7.  I looked at the things linked in the spreadsheet that sounded similar and none had icons.

Translate has final mocks.  Ask Hwi for them (then bonus, add some links to the spreadsheet once you have them).

In any case, this bug isn't solely about icon; it's primarily about drawing the title, secondarily the icon, using a consistent path.  So "making this general" is much broader than "is there more than one dialog with an icon".

Comment 9 by bsep@chromium.org, Mar 25 2017

Cc: pkasting@chromium.org
So as a test for this I went to convert the Add to Desktop dialog because it seemed the most straightforward... and actually all the mechanisms we want are already in place. BubbleFrameView supports an icon next to the title, and it even lays out correctly without any changes (16 dips between icon and title).

The only thing it doesn't do is nudge elements out from below itself. But I realized since most dialogs use GridLayout they can just create a blank "clear" column that's the same size as the icon. We could even make a metric for it that's defined in terms of our preferred icon size (which I think is 32 dips). But I think that should be done alongside converting a dialog that needs it.

There's also a method for toggling the close button already. So is there any action here? We mentioned wanting "modal" or "non-modal" to have meaning, but I think it'd be easier to go through and set the right value for the close button for each dialog, and then if we see other behaviors that depend on the same consideration we've already determined which dialogs are which.

For reference, here are the relevant DialogDelegate methods:
* override GetWindowTitle to provide a title
* override GetWindowIcon and ShouldShowWindowIcon to provide an icon
* override ShouldShowCloseButton to hide the close button. the default is to show it
I guess the only thing remaining to do here is to audit the existing dialog to make sure everyone actually uses these methods, instead of rolling their own solutions with manually-added images or labels.

Comment 11 by bsep@chromium.org, Mar 29 2017

Status: WontFix (was: Assigned)
The only two dialogs that are doing anything weird are the translate dialog and the extensions dialog.

The translate dialog doesn't have a title, instead it has an icon+body text that kind of looks like a title, but that's actually according to spec... there's a different look for Harmony so it'll get some attention before we're done.

The install extension dialog has a custom title and icon so that it can put the icon on the right, but again it has a more standard look for Harmony so we'll fix that one up eventually too.

In either case it's pretty obvious that they're not using the standard title because there's too much padding at the top of the dialog. They'll need to use the standard mechanisms in order to pass review. So I'm closing this bug since there's no general work to do.
SGTM.

Now that you noticed those two cases, feel free to go ahead and change them to use standard APIs so we don't just come back later and modify the padding and such to make them _look_ correct :)

Comment 13 by bsep@chromium.org, Apr 5 2017

Status: Assigned (was: WontFix)
Summary: Harmony - Move dialog title/icon/close button to client area (was: Harmony - Need mechanism to supply dialog/bubble title/icon/close button independently)
The new spec has the content spaced out from the title, but may include an icon that has its own column. There's also a dialog that has a different button instead of the close-X.

This is all pretty hard to do cleanly with the current implementation because all those elements are technically in the non-client area. I'm going to try to move these into the client area so everything can be laid out together, and this seems like a good bug to repurpose.
Project Member

Comment 14 by bugdroid1@chromium.org, Apr 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4ced9f3c7e9534b6b730d3b265e39a0fe5b17dae

commit 4ced9f3c7e9534b6b730d3b265e39a0fe5b17dae
Author: estade <estade@chromium.org>
Date: Fri Apr 14 16:01:29 2017

Fix layout of BubbleFrameView when there's only a close button in the
title row.

Push the client area down slightly so it doesn't overlap close. This
is a bandaid until bug 702196 is fixed.

BUG= 709380 ,702196

Review-Url: https://codereview.chromium.org/2807243004
Cr-Commit-Position: refs/heads/master@{#464731}

[modify] https://crrev.com/4ced9f3c7e9534b6b730d3b265e39a0fe5b17dae/ui/views/bubble/bubble_frame_view.cc
[modify] https://crrev.com/4ced9f3c7e9534b6b730d3b265e39a0fe5b17dae/ui/views/bubble/bubble_frame_view_unittest.cc
[modify] https://crrev.com/4ced9f3c7e9534b6b730d3b265e39a0fe5b17dae/ui/views/window/dialog_delegate_unittest.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Apr 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/77227cc6631117209eb6cfbbc5c78bf0d3c8a4b4

commit 77227cc6631117209eb6cfbbc5c78bf0d3c8a4b4
Author: alexmos <alexmos@chromium.org>
Date: Fri Apr 14 19:25:03 2017

Revert of Fix layout of BubbleFrameView when there's only a close button in the (patchset #5 id:40002 of https://codereview.chromium.org/2807243004/ )

Reason for revert:
Breaks DialogTest.AcceptAndCancel on a couple of bots (https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/59105 and https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29/builds/24880).

Original issue's description:
> Fix layout of BubbleFrameView when there's only a close button in the
> title row.
>
> Push the client area down slightly so it doesn't overlap close. This
> is a bandaid until bug 702196 is fixed.
>
> BUG= 709380 ,702196

TBR=tapted@chromium.org,pkasting@chromium.org,bsep@chromium.org,estade@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 709380 ,702196

Review-Url: https://codereview.chromium.org/2821463004
Cr-Commit-Position: refs/heads/master@{#464773}

[modify] https://crrev.com/77227cc6631117209eb6cfbbc5c78bf0d3c8a4b4/ui/views/bubble/bubble_frame_view.cc
[modify] https://crrev.com/77227cc6631117209eb6cfbbc5c78bf0d3c8a4b4/ui/views/bubble/bubble_frame_view_unittest.cc
[modify] https://crrev.com/77227cc6631117209eb6cfbbc5c78bf0d3c8a4b4/ui/views/window/dialog_delegate_unittest.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Apr 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eed63750baeef51b69420b637d4e42c6be2c446d

commit eed63750baeef51b69420b637d4e42c6be2c446d
Author: estade <estade@chromium.org>
Date: Mon Apr 17 21:39:35 2017

Re-land 4ced9f3c7e9534b6b730d: Fix layout of BubbleFrameView
when there's only a close button in the title row.

Push the client area down slightly so it doesn't overlap close. This
is a bandaid until bug 702196 is fixed.

BUG= 709380 , 702196
TBR=tapted@chromium.org

Review-Url: https://codereview.chromium.org/2821893003
Cr-Commit-Position: refs/heads/master@{#465029}

[modify] https://crrev.com/eed63750baeef51b69420b637d4e42c6be2c446d/ui/views/bubble/bubble_frame_view.cc
[modify] https://crrev.com/eed63750baeef51b69420b637d4e42c6be2c446d/ui/views/bubble/bubble_frame_view_unittest.cc
[modify] https://crrev.com/eed63750baeef51b69420b637d4e42c6be2c446d/ui/views/window/dialog_delegate_unittest.cc

Project Member

Comment 17 by bugdroid1@chromium.org, Apr 19 2017

Labels: merge-merged-3071
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3f3e136ba02bfc3735028a34ae39ee4507f9ffc8

commit 3f3e136ba02bfc3735028a34ae39ee4507f9ffc8
Author: Peter Kasting <pkasting@chromium.org>
Date: Wed Apr 19 23:30:14 2017

Re-land 4ced9f3c7e9534b6b730d: Fix layout of BubbleFrameView when there's only a close button in the title row.

Push the client area down slightly so it doesn't overlap close. This
is a bandaid until bug 702196 is fixed.

BUG= 709380 , 702196
TBR=tapted@chromium.org

Review-Url: https://codereview.chromium.org/2821893003
Cr-Commit-Position: refs/heads/master@{#465029}
(cherry picked from commit eed63750baeef51b69420b637d4e42c6be2c446d)

Review-Url: https://codereview.chromium.org/2826323002 .
Cr-Commit-Position: refs/branch-heads/3071@{#71}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/3f3e136ba02bfc3735028a34ae39ee4507f9ffc8/ui/views/bubble/bubble_frame_view.cc
[modify] https://crrev.com/3f3e136ba02bfc3735028a34ae39ee4507f9ffc8/ui/views/bubble/bubble_frame_view_unittest.cc
[modify] https://crrev.com/3f3e136ba02bfc3735028a34ae39ee4507f9ffc8/ui/views/window/dialog_delegate_unittest.cc

Project Member

Comment 18 by bugdroid1@chromium.org, Apr 24 2017

Labels: merge-merged-3029
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/77b32554d91e8fd5a1f093038926d262a4bda5a0

commit 77b32554d91e8fd5a1f093038926d262a4bda5a0
Author: Peter Kasting <pkasting@chromium.org>
Date: Mon Apr 24 17:34:39 2017

Re-land 4ced9f3c7e9534b6b730d: Fix layout of BubbleFrameView when there's only a close button in the title row.

Push the client area down slightly so it doesn't overlap close. This
is a bandaid until bug 702196 is fixed.

BUG= 709380 , 702196
TBR=tapted@chromium.org

Review-Url: https://codereview.chromium.org/2821893003
Cr-Commit-Position: refs/heads/master@{#465029}
(cherry picked from commit eed63750baeef51b69420b637d4e42c6be2c446d)

Review-Url: https://codereview.chromium.org/2840623002 .
Cr-Commit-Position: refs/branch-heads/3029@{#766}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/77b32554d91e8fd5a1f093038926d262a4bda5a0/ui/views/bubble/bubble_frame_view.cc
[modify] https://crrev.com/77b32554d91e8fd5a1f093038926d262a4bda5a0/ui/views/bubble/bubble_frame_view_unittest.cc
[modify] https://crrev.com/77b32554d91e8fd5a1f093038926d262a4bda5a0/ui/views/window/dialog_delegate_unittest.cc

There are also a few password dialogs/bubbles that have links in the title, and use a StyledLabel to get around this:

 - AccountChooserDialogView. BrowserDialogTests include:
    + PasswordDialogViewTest.InvokeDialog_PopupAutoSigninPrompt
    + PasswordDialogViewTest.InvokeDialog_PopupAccountChooserWithSingleCredentialClickSignIn
 - ManagePasswordsBubbleView. BrowserDialogTests include:
    + ManagePasswordsBubbleDialogViewTest.InvokeDialog_PendingPasswordBubble
    + ManagePasswordsBubbleDialogViewTest.InvokeDialog_ManagePasswordBubble (currently disabled)
    + ManagePasswordsBubbleDialogViewTest.InvokeDialog_AutomaticPasswordBubble
    + ManagePasswordsBubbleDialogViewTest.InvokeDialog_AutoSignin

And this last dialog (below) seems to have a custom title because they would rather the dialog get resized than to trim the title (? - according to a comment in ShouldShowWindowTitle()). I haven't investigated how long the title can potentially get, though (the one I get with the BrowserDialogTest below is really short, just says "Sign in easily").

 - AutoSigninFirstRunDialogView. BrowserDialogTests include:
   + PasswordDialogViewTest.InvokeDialog_AutoSigninFirstRun (WIP in https://codereview.chromium.org/2869683003/)
 
Project Member

Comment 21 by bugdroid1@chromium.org, May 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d77878b9c9f4f4db96a21e0974843a7e8cd77b24

commit d77878b9c9f4f4db96a21e0974843a7e8cd77b24
Author: bsep <bsep@chromium.org>
Date: Wed May 17 20:40:13 2017

Revert of Backfill some UI tests. (patchset #9 id:160001 of https://codereview.chromium.org/2861533003/ )

Reason for revert:
Caused compile failure

Original issue's description:
> Backfill some UI tests.
>
> * Added a test for DialogClientView's button layout.
> * Added a test for non client hit results in BubbleFrameView.
> * Added a test for ValidationMessageBubbleView's anchor rect at HighDPI.
>
> BUG=702196, 680019 
>
> Review-Url: https://codereview.chromium.org/2861533003
> Cr-Commit-Position: refs/heads/master@{#472550}
> Committed: https://chromium.googlesource.com/chromium/src/+/819580022f1d2c8dc1eaa4d8614e765a55f95a4b

TBR=sky@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=702196, 680019 

Review-Url: https://codereview.chromium.org/2891803002
Cr-Commit-Position: refs/heads/master@{#472554}

[delete] https://crrev.com/525974cdba654d1611d169a34397304334797175/chrome/browser/ui/views/validation_message_bubble_view_unittest.cc
[modify] https://crrev.com/d77878b9c9f4f4db96a21e0974843a7e8cd77b24/chrome/test/BUILD.gn
[modify] https://crrev.com/d77878b9c9f4f4db96a21e0974843a7e8cd77b24/content/test/test_render_view_host.cc
[modify] https://crrev.com/d77878b9c9f4f4db96a21e0974843a7e8cd77b24/content/test/test_render_view_host.h
[modify] https://crrev.com/d77878b9c9f4f4db96a21e0974843a7e8cd77b24/ui/views/window/dialog_client_view_unittest.cc
[modify] https://crrev.com/d77878b9c9f4f4db96a21e0974843a7e8cd77b24/ui/views/window/dialog_delegate_unittest.cc

Project Member

Comment 22 by bugdroid1@chromium.org, May 18 2017

Noticed that the bookmark bubble looks like a case of hand-rolling a title instead of setting it correctly.
Project Member

Comment 24 by bugdroid1@chromium.org, Jun 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/257ee88f3a0e1b02da7814cbc37f693494bad6d3

commit 257ee88f3a0e1b02da7814cbc37f693494bad6d3
Author: bsep <bsep@chromium.org>
Date: Wed Jun 28 01:19:58 2017

Allow dialogs to use a custom View as their title.

Added a method to DialogDelegate that lets a dialog subclass specify a
View that will be used as the dialog's title. As an example, changed the
Save Password dialog and removed its ad-hoc title. Also removed
SetTitleFontList, as it's not clear how it should interact with a generic
View title, and updated the subclasses that were using it.

BUG= 654115 ,702196

Review-Url: https://codereview.chromium.org/2907983002
Cr-Commit-Position: refs/heads/master@{#482840}

[modify] https://crrev.com/257ee88f3a0e1b02da7814cbc37f693494bad6d3/chrome/browser/ui/views/page_info/page_info_bubble_view.cc
[modify] https://crrev.com/257ee88f3a0e1b02da7814cbc37f693494bad6d3/chrome/browser/ui/views/page_info/page_info_bubble_view.h
[modify] https://crrev.com/257ee88f3a0e1b02da7814cbc37f693494bad6d3/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
[modify] https://crrev.com/257ee88f3a0e1b02da7814cbc37f693494bad6d3/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h
[modify] https://crrev.com/257ee88f3a0e1b02da7814cbc37f693494bad6d3/chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc
[modify] https://crrev.com/257ee88f3a0e1b02da7814cbc37f693494bad6d3/ui/views/bubble/bubble_dialog_delegate.cc
[modify] https://crrev.com/257ee88f3a0e1b02da7814cbc37f693494bad6d3/ui/views/bubble/bubble_dialog_delegate.h
[modify] https://crrev.com/257ee88f3a0e1b02da7814cbc37f693494bad6d3/ui/views/bubble/bubble_dialog_delegate_unittest.cc
[modify] https://crrev.com/257ee88f3a0e1b02da7814cbc37f693494bad6d3/ui/views/bubble/bubble_frame_view.cc
[modify] https://crrev.com/257ee88f3a0e1b02da7814cbc37f693494bad6d3/ui/views/bubble/bubble_frame_view.h
[modify] https://crrev.com/257ee88f3a0e1b02da7814cbc37f693494bad6d3/ui/views/window/dialog_delegate_unittest.cc

Comment 25 by bsep@chromium.org, Jun 29 2017

Summary: Harmony - Standardize dialogs that hand-roll their own versions of non-client components (was: Harmony - Move dialog title/icon/close button to client area)
Making this bug a little more general because moving everything to the client area is out of scope now, and probably isn't necessary anyway.
You mean out of scope for phase 1?  Right, if we get a standardized practice that works, we probably do not need to actually make this client area now, maybe not ever, I dunno.

Comment 27 by bsep@chromium.org, Jun 30 2017

Yes, out of scope for phase 1. We can revisit the larger refactor later, since I think it'll be a lot clearer whether it's necessary then.
Project Member

Comment 28 by bugdroid1@chromium.org, Jul 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f8bac8b678e69aa152968a5099da30f3b6847ca6

commit f8bac8b678e69aa152968a5099da30f3b6847ca6
Author: Bret Sepulveda <bsep@chromium.org>
Date: Tue Jul 04 09:33:52 2017

Harmonize Smart Lock First Run dialog.

I couldn't find a clear reason why the dialog had hand-rolled its title
and buttons, so I removed all that code and used a basic layout. This
patch should be the extent of Harmony changes specific to this dialog.

Bug:  651680 ,702196
Change-Id: I0e40d00c0bc13df19a25b4884e5d533e463c07a7
Reviewed-on: https://chromium-review.googlesource.com/557139
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#484076}
[modify] https://crrev.com/f8bac8b678e69aa152968a5099da30f3b6847ca6/chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc
[modify] https://crrev.com/f8bac8b678e69aa152968a5099da30f3b6847ca6/chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.h

 Issue 685444  has been merged into this issue.
Labels: -Pri-1 -merge-merged-3029 -merge-merged-3071 Pri-3
Project Member

Comment 31 by bugdroid1@chromium.org, Aug 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0d41d488a4beb66a5d7806e5fbb0f994927ec3a2

commit 0d41d488a4beb66a5d7806e5fbb0f994927ec3a2
Author: Bret Sepulveda <bsep@chromium.org>
Date: Tue Aug 22 18:31:40 2017

Standardize title for AccountChooserDialogView.

Now that dialogs can have custom views as their titles, this dialog
can use that mechanism.

Bug: 702196
Change-Id: I5bb5d5ebacd51f00d992d1c7588b650749715cfe
Reviewed-on: https://chromium-review.googlesource.com/609544
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496378}
[modify] https://crrev.com/0d41d488a4beb66a5d7806e5fbb0f994927ec3a2/chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc
[modify] https://crrev.com/0d41d488a4beb66a5d7806e5fbb0f994927ec3a2/chrome/browser/ui/views/passwords/account_chooser_dialog_view.h

Sign in to add a comment