Hello bettes@,

Given how narrow this dialog is I wanted to see what would happen when the Default button has a longer title, so I ran Chrome in Russian. I've attached the result, which surprised me at first, but in retrospect made me realize we have not really talked about what dialogs should do when they have wide titles. I don't think this result is good (we should prefer getting taller in this case).

Deleted: Screen Shot 2016-09-30 at 6.29.11 PM.png 66.5 KB

	Screen Shot 2016-09-30 at 6.29.11 PM.png 66.5 KB View Download

Yeah we haven't explicity addressed this yet but you have the right idea. We should accommodate overflow cases by increasing the height, and never the width as the widths should be constrained to our 3 buckets. 

From there, we should ensure that the text overflows with the correct line-height attached to that specific class of typography. 

Let me know if this answers your question. 

https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(MD)/Secondary%20UI%20Previews%20and%20specs%20(exports)/Spec#%2FSPEC-secondary-UI-03b-dialog-keylines.png%3Fz=width

Deleted: Screen Shot 2016-10-03 at 9.54.58 AM.png 368 KB

	Screen Shot 2016-10-03 at 9.54.58 AM.png 368 KB View Download

Great! I will work on getting this implemented.

To sign off on the dialogs I'm thinking we need screenshots from them running in a language like Russian that will cause titles to wrap. What are your thoughts?

I think that's a good idea. Hopefully that's not too much overhead for you guys

I am concerned about the overhead, but if we don't do it we will be flying blind on non-English languages.

This is how the dialog now looks in material mode and non-material mode. I've added material layout spacing/distances using the LayoutDelegate. I've not found any material spec specific to the indent of the radio group so it's left as-is.

Deleted: RPHDialogComparison.png 17.4 KB

	RPHDialogComparison.png 17.4 KB View Download

It's getting there. Some tweaks:

* The dialog title and "Manage handler settings" title should start 16pt from the left edge of the dialog (looks closer to 13pt)
* The Done button should be 16pt from the right edge of the dialog and 16pt from the bottom
* When you move the Done button up be sure to move the "Manage handler settings" text up by the same amount (so that they share the same baseline)
* The dialog should default to 320pt width
* Title text should be 15pt, regular weight
* Link text should be 12pt, regular weight
* Remove the gray rule
* Place the radio buttons flush with the text (so 16pt from left edge of the dialog).

shrike@, Thanks! That's precisely the information I need.

Here is another iteration. It was really tricky to locate which margin/inset was actually controlling the distance to the outer border.

Deleted: RPHBubbleDialogProgress1.png 6.1 KB

	RPHBubbleDialogProgress1.png 6.1 KB View Download

Great, and thanks for the hard work on this (I guess updating these dialogs will take non-trivial amounts of time). Does the LayoutDelegate provide all the layout constants you need? If not we need to add whatever's missing.

Remaining tweaks (+/-):

1. Please move the radio buttons 7pt to the left
2. Please move the Manage handler settings text 2pts to the left (should be aligned with the dialog title)
3. Please narrow the dialog by about 30pts (it should be 320pt wide)
4. Please double-check that the title text is 15pt and the link text is 12pt - they appear to be the same point size; I think the title text is too small
5. After you change the title text, check that the title is 16pt from the top of the dialog. Currently it's about 20pt. If the title moves up, everything else will need to move up also (and the dialog will need to shrink in height by that much).
6. Can you also make sure that the title text will appear in title case on the Mac?

Yes, this is taking a non-trivial amount of time. There are many layers of delegates in play. In some cases, one delegate may explicitly zero out a value from the ancestor only to be replaced by another field.

It is a little unclear exactly *how* the radio buttons are to be moved since the controls themselves had that indent built in. You can see how the control highlighting on mouse hover is aligned... I'll check if there is a property on the radio button to control that offset.

This dialog is broken up across ui/views and chrome/browser/ui/. Much of the changes I'm making are down in ui/views which cannot access chrome/browser/ui. The constants are obtained from a like-named header ui/views/layout/layout_constants.h. I think something similar to the LayoutDelegate will need to be done down in ui/views. (ui/views/layout?)

Deleted: RPHBubbleDialogProgress1.png 7.8 KB

	RPHBubbleDialogProgress1.png 7.8 KB View Download

I see what you're saying about the radio button layout. I'm not sure what to recommend - I believe sky@ would say that all layout parameters that pertain to a particular style of layout (i.e. Harmony, pre-Harmony) should live outside of ui/views, and that makes sense to me. Perhaps he can recommend a solution that avoids introducing knowledge of Harmony in ui/views.

ui/base already has knowledge of Material design, and by extension has knowledge of Harmony by virtue of using the ui::MaterialDesignController::IsSecondaryUiMaterial() static function.

ui/base/material_design/material_design_controller.h

Here are some of the main views with added borders to better delineate their bounds. The main client view is 16pts from the edge all around. The Link label's text isn't right up to the edge likely because of another inset/border setting.

The radio buttons use a default inset based on the hard-coded value from views::LabelButtonAssetBorder::GetDefaultInsetsForStyle().

Deleted: RPHBubbleDialogBorderDemo.png 6.3 KB

	RPHBubbleDialogBorderDemo.png 6.3 KB View Download

This is getting close. Figured out how to remove the left inset on the radio buttons (it's a bit of a hack, which should be refined during code review). Enlarged the title text and set dialog width to 320.

Deleted: RPHBubbleDialogProgress2.png 6.8 KB

	RPHBubbleDialogProgress2.png 6.8 KB View Download

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

commit 44ec2a4fd5e1c052077575f84b5b5cbdb703cef6
Author: kylixrd <kylixrd@chromium.org>
Date: Tue Jan 10 21:03:16 2017

Convert Register Protocol Handler dialog for Harmony/Material Design

NOTE: this CL only addresses the primary content of the referenced issue. The other issue regarding the wrapping of text should be a separate issue and confined to the implementing views/controls.

BUG= 652024 

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

[modify] https://crrev.com/44ec2a4fd5e1c052077575f84b5b5cbdb703cef6/chrome/browser/ui/views/chrome_views_delegate.cc
[modify] https://crrev.com/44ec2a4fd5e1c052077575f84b5b5cbdb703cef6/chrome/browser/ui/views/chrome_views_delegate.h
[modify] https://crrev.com/44ec2a4fd5e1c052077575f84b5b5cbdb703cef6/chrome/browser/ui/views/content_setting_bubble_contents.cc
[modify] https://crrev.com/44ec2a4fd5e1c052077575f84b5b5cbdb703cef6/chrome/browser/ui/views/cookie_info_view.cc
[modify] https://crrev.com/44ec2a4fd5e1c052077575f84b5b5cbdb703cef6/chrome/browser/ui/views/harmony/harmony_layout_delegate.cc
[modify] https://crrev.com/44ec2a4fd5e1c052077575f84b5b5cbdb703cef6/chrome/browser/ui/views/harmony/layout_delegate.cc
[modify] https://crrev.com/44ec2a4fd5e1c052077575f84b5b5cbdb703cef6/chrome/browser/ui/views/harmony/layout_delegate.h
[modify] https://crrev.com/44ec2a4fd5e1c052077575f84b5b5cbdb703cef6/chrome/browser/ui/views/layout_utils.cc
[modify] https://crrev.com/44ec2a4fd5e1c052077575f84b5b5cbdb703cef6/chrome/browser/ui/views/login_view.cc
[modify] https://crrev.com/44ec2a4fd5e1c052077575f84b5b5cbdb703cef6/ui/views/bubble/bubble_dialog_delegate.cc
[modify] https://crrev.com/44ec2a4fd5e1c052077575f84b5b5cbdb703cef6/ui/views/layout/layout_constants.h
[modify] https://crrev.com/44ec2a4fd5e1c052077575f84b5b5cbdb703cef6/ui/views/views_delegate.cc
[modify] https://crrev.com/44ec2a4fd5e1c052077575f84b5b5cbdb703cef6/ui/views/views_delegate.h
[modify] https://crrev.com/44ec2a4fd5e1c052077575f84b5b5cbdb703cef6/ui/views/window/dialog_client_view.cc

Hello kylixrd@,

Sorry, way behind on my e-mail from the break, so I hadn't seen your previous messages. Great work, and thank you! The text title has some top padding inside the view (not sure what UX will say about that), and there's the extra padding you mentioned before the text link text, but other than that it looks good.

It looks like the distance between the radio buttons and their labels is less than the 16 px in the spec.

Re: c#19, yes - I'm not sure what that distance is, but it's definitely less than 16pt. The radio button control should start 16pt from the left edge of the dialog.

I'm not sure to what you're referring. The padding should be 16px all the way around the inside of the dialog. The radio button instances were explicitly tweaked to eliminate the padding on the left so they would align at the 16px boundary. See image from #16

Re: c#21, I'm not sure if you're responding to my c#20, but we are in agreement.

Annotated version of your screenshot attached in hopes of clarity.

See https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(MD)/Secondary%20UI%20Previews%20and%20specs%20(exports)/Spec#%2FSPEC-secondary-UI-03b-dialog-keylines.png%3Fz=width for spec.  This is something we should change globally for all Harmony controls, it's not specific to this dialog.

Deleted: RPHBubbleDialogProgress2.png 9.0 KB

	RPHBubbleDialogProgress2.png 9.0 KB View Download

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

commit a5d37aea27ce6ff61d1b7c55c65fe4d7ecc662fe
Author: kylixrd <kylixrd@chromium.org>
Date: Wed Feb 15 19:44:33 2017

DialogBrowserTest implementation to invoke Content settings bubble dialogs.

There needs to be a way to invoke all the various content settings
bubble dialog views. This wires up the BrowserDialogTest framework in
order to invoke/test them.

BUG= 652024 

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

[modify] https://crrev.com/a5d37aea27ce6ff61d1b7c55c65fe4d7ecc662fe/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h
[modify] https://crrev.com/a5d37aea27ce6ff61d1b7c55c65fe4d7ecc662fe/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
[modify] https://crrev.com/a5d37aea27ce6ff61d1b7c55c65fe4d7ecc662fe/chrome/browser/ui/content_settings/content_setting_image_model.cc
[modify] https://crrev.com/a5d37aea27ce6ff61d1b7c55c65fe4d7ecc662fe/chrome/browser/ui/content_settings/content_setting_image_model.h
[modify] https://crrev.com/a5d37aea27ce6ff61d1b7c55c65fe4d7ecc662fe/chrome/browser/ui/location_bar/location_bar.h
[modify] https://crrev.com/a5d37aea27ce6ff61d1b7c55c65fe4d7ecc662fe/chrome/browser/ui/test/browser_dialog_browsertest.cc
[add] https://crrev.com/a5d37aea27ce6ff61d1b7c55c65fe4d7ecc662fe/chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc
[modify] https://crrev.com/a5d37aea27ce6ff61d1b7c55c65fe4d7ecc662fe/chrome/browser/ui/views/location_bar/location_bar_view.cc
[modify] https://crrev.com/a5d37aea27ce6ff61d1b7c55c65fe4d7ecc662fe/chrome/browser/ui/views/location_bar/location_bar_view.h
[modify] https://crrev.com/a5d37aea27ce6ff61d1b7c55c65fe4d7ecc662fe/chrome/test/BUILD.gn

Fixed.

Deleted: RPHBubbleDialogProgress3.png 7.2 KB

	RPHBubbleDialogProgress3.png 7.2 KB View Download

Ugh - really not liking the fat spacing between control and title.

I'm more OK with it than you are, but I think 8 DIP might make more sense, since these labels are intimately related (in the sense I covered in the meeting this week) to the buttons they're next to.

I don't feel strongly.  Someone who does is welcome to yell at Alan.

I probably will talk to bettes@ about it.

Looking at this dialog on the Mac, for some reason the link text is 1pt higher than the button title. The non-Mac screenshot above has the correct alignment.

Deleted: Screen Shot 2017-02-20 at 2.29.53 PM.png 58.6 KB

	Screen Shot 2017-02-20 at 2.29.53 PM.png 58.6 KB View Download

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

commit c34b8926fd843300658dcdaec4c69c6967cf86c6
Author: kylixrd <kylixrd@chromium.org>
Date: Fri Mar 10 16:38:49 2017

Refactor ViewsDelegate and MD-ify the icon-to-text spacing for checkbox and radiobutton.

ViewsDelegate should have the layout related functions coalesced into a handful of functions which take an enum element for each metric.

BUG= 652024 , 687349 

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

[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/chrome/browser/first_run/try_chrome_dialog_view.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/chrome/browser/ui/views/chrome_views_delegate.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/chrome/browser/ui/views/chrome_views_delegate.h
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/chrome/browser/ui/views/chrome_views_delegate_win.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/chrome/browser/ui/views/extensions/media_galleries_dialog_views_unittest.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/chrome/browser/ui/views/global_error_bubble_view_unittest.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/chrome/browser/ui/views/harmony/layout_delegate.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/chrome/browser/ui/views/toolbar/reload_button_unittest.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/ui/views/bubble/bubble_dialog_delegate.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/ui/views/bubble/bubble_frame_view.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/ui/views/controls/button/label_button.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/ui/views/controls/button/md_text_button.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/ui/views/examples/dialog_example.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/ui/views/layout/layout_constants.h
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/ui/views/views_delegate.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/ui/views/views_delegate.h
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/ui/views/window/dialog_client_view.cc
[modify] https://crrev.com/c34b8926fd843300658dcdaec4c69c6967cf86c6/ui/views/window/dialog_delegate.cc

Latest version of the RPH dialog. This one is wider because of the dialog width sizing logic is choosing 448px instead of 320px. If 320px is chosen, the "Allow mail.google.com to...." text is word-wrapped.

Based on the current dialog width logic, the label text will only wrap if the text exceeds the maximum allowed dialog width of 512px.

Deleted: RPHBubbleDialogProgress4.png 5.8 KB

	RPHBubbleDialogProgress4.png 5.8 KB View Download

Instructions on how to trigger this UI?

Also see the following bug for relevant UI requests (change of manage button)

https://bugs.chromium.org/p/chromium/issues/detail?id=700196#c6

> Instructions on how to trigger this UI? 

There's a good chance that one or both of https://mail.google.com/ or https://calendar.google.com/ show the "thingy" icon on the right side of the omnibox, next to the bookmark star. (see screengrab in the description).

If not, go to chrome://settings/handlers and `Dots -> Remove` should cause those sites to surface the thingy icon again.

Deleted: Screen Shot 2017-08-16 at 12.56.08 pm.png 31.3 KB

	Screen Shot 2017-08-16 at 12.56.08 pm.png 31.3 KB View Download

Here's the latest image of what this dialog looks like.

Deleted: RegisterProtocolHandlerDIalog.png 5.7 KB

	RegisterProtocolHandlerDIalog.png 5.7 KB View Download

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

commit 7cb003450d7134776478ebd606ce006d931b6303
Author: Allen Bauer <kylixrd@chromium.org>
Date: Mon Oct 30 20:26:59 2017

Never show the close(x) icon on content setting bubbles.

Bug:  652024 
Change-Id: I77bfbe29d728e3f3f8bdc0d7d2b1f728d8da969b
Reviewed-on: https://chromium-review.googlesource.com/740085
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512603}
[modify] https://crrev.com/7cb003450d7134776478ebd606ce006d931b6303/chrome/browser/ui/views/content_setting_bubble_contents.cc

Hi kylixrd@ and bsep@ - re: c37, the change description "Never show the close(x) icon on content setting bubbles." is the opposite of the spec( crbug.com/707264#c2 ). Could you help clarify? The desired outcome of the close (x) should look like the screenshot on c36.

So the close (x) icon *is* supposed to be present? What does it mean to use that to close the dialog?

c39: Correct. Close (x) closes popover dialogs without committing any changes (same as Esc). 

- Summary is in:  crbug.com/707264 
- Here's the UI review: go/crhcxur

Please let me know if this needs further clarification. Thanks! 

To add more clarity, *modal* dialogs, on the other hand, should *not* have Close (x), but have Cancel.  crbug.com/707263 

It should run Close(), same as what pressing ESC already does. I did point out that the patch seemed inconsistent with the spec, but I figured there was a reason for the inconsistency.

This specific Close (x) spec intends to *not* make any deviations.

Ok, I've reverted the erroneous commit. The close (x) should return.

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

commit 33d3e76152c874871a60bd3603d45781b362037e
Author: Allen Bauer <kylixrd@chromium.org>
Date: Tue Oct 31 15:45:23 2017

Revert "Never show the close(x) icon on content setting bubbles."

This reverts commit 7cb003450d7134776478ebd606ce006d931b6303.

Reason for revert: See comment on  https://crbug.com/652024#c40 . The close (x) should be shown in these instances.

Original change's description:
> Never show the close(x) icon on content setting bubbles.
> 
> Bug:  652024 
> Change-Id: I77bfbe29d728e3f3f8bdc0d7d2b1f728d8da969b
> Reviewed-on: https://chromium-review.googlesource.com/740085
> Reviewed-by: Bret Sepulveda <bsep@chromium.org>
> Commit-Queue: Allen Bauer <kylixrd@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#512603}

TBR=bsep@chromium.org,kylixrd@chromium.org

Change-Id: I901d8e394364095ebde746a3aa42e973d8c54868
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  652024 
Reviewed-on: https://chromium-review.googlesource.com/747041
Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512838}
[modify] https://crrev.com/33d3e76152c874871a60bd3603d45781b362037e/chrome/browser/ui/views/content_setting_bubble_contents.cc

Thanks kylixrd@!

The NextAction date has arrived: 2017-11-10

This dialog/bubble should be ready for review. If there are any other tweaks, other than any fonts or color issues, please reassign. For font/color issues, those are currently being handled in a more centralized manner.

LGTM