New issue
Advanced search Search tips

Issue 652031 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: 2017-09-29
OS: Linux , Windows , Chrome
Pri: 1
Type: Bug
Launch-M-Target: 64-Beta

Blocked on:
issue 758282

Blocking:
issue 630357


Show other hotlists

Hotlists containing this issue:
Harmony-Ready-For-Review


Sign in to add a comment

Harmony - update popups blocked and mixed-content dialogs

Project Member Reported by shrike@chromium.org, Oct 1 2016

Issue description

Comment 1 by shrike@chromium.org, Oct 11 2016

Labels: Proj-HarmonyDialogs-HardToSummon
Just found out that permission.site also does popups, so if you navigate there and select "Popup (delayed 2 seconds)" option you should be able to get this dialog to appear that way.

Comment 3 by shrike@chromium.org, Dec 10 2016

Labels: -Proj-HarmonyDialogs-HardToSummon
OK, thank you.
Owner: ----
Status: Available (was: Assigned)
Per shrike, unassigning his Harmony bugs for now.

Comment 5 by tapted@chromium.org, Mar 14 2017

Description: Show this description
Labels: -M-56
Labels: Pri-1
Description: Show this description
Description: Show this description
Owner: kylixrd@chromium.org
Status: Assigned (was: Available)
Assigning to Allen on the basis that this looks like it has a ScrollView in it :D
The client area of this view is constructed using ContentSettingBubbleContents, which is used for many different incarnations of content bubbles. I'm going to assume here that all similar bubble dialogs should sport the same scrolling region.

Given that, how many items in the list should be visible? From the image, it appears to be 4 items before scrolling is to occur. I'll go with that for now. If the height is some fixed number of pixels/dips, I've not been able to find any specific spec.
Cc: pkasting@chromium.org bettes@chromium.org
Here's the blocked popups bubble with a scrolling view. This is on Windows, which is using the Windows style scrollbar.

It's using Harmony-spec icon-to-text spacing and vertical spacing between list items. This is clearly very different from the mock above.

Looks like there will need to be some work on getting the scroll view to use a different scroll-bar style (assuming that's going to be the universal style across platforms). The overall width of the scrolling region needs to fill the horizontal space. It should also be made to look more like a bullet-list with tighter vertical spacing.
BlockedPopupDialogScrolling.png
12.8 KB View Download
I've set up some time to discuss in person. 
Getting closer...

I've changed the "Manage pop-up blocking..." link to be just a since "Manage" button. The vertical spacing of the list items is tighter, like the mock.

The list currently uses the "favicon" image along with "related control horizontal spacing" between the icon and the link text.
BlockedPopupDialog.png
11.1 KB View Download
This one now fills the available width of dialog (less the dialog insets) for the scrollable view.

Another discussion point; according to the harmony spec, there is a consistent inset around the inner edge of the dialog. However, according to the above mock, this inset is ignored for the scrolling region.

BlockedPopupsFillWidth.png
11.5 KB View Download
Blockedon: 758282
Notes from our meeting on 8/22: 

Scrollbar: 
Please use the native scrollbar.

Scrollview: 
Review with bsep@ on the status of scrollviews. We agreed to restrict the rule lines to obey the 16pt outer-padding as opposed to full width.

Manage btn: 
Position the button in the bottom left corner. Styling doesn't change. 

Strings: 
 crbug.com/758282 

List of popups: 
Thanks for adding the unicode "•" to the list. It'd be preferred if you could add 2 spacebars between bullet and site.

Close-x: 
Close-x buttons needed to be added for all popovers 


Labels: Launch-M-Target-64-Beta
NextAction: 2017-09-29
This is the mixed content dialog.
Mixed-Script.png
4.3 KB View Download
Description: Show this description
Summary: Harmony - update popups blocked and mixed-content dialogs (was: Harmony - update popups blocked dialog)
Project Member

Comment 21 by bugdroid1@chromium.org, Sep 12 2017

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

commit 4ba1c8da24589b525f3d9f46339b5ee1f4a10354
Author: Allen Bauer <kylixrd@chromium.org>
Date: Tue Sep 12 15:09:52 2017

Blocked popup & mixed content dialogs better matches the Material Design mocks.

Added blocked popup content to interactive invocation of blocked popups dialog.

TBR=grt@chromium.org

Bug:  652031 
Change-Id: I1c3f4bca0fe682897e0ed98e61957735b0d46320
Reviewed-on: https://chromium-review.googlesource.com/612496
Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Adrienne Porter Felt <felt@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501278}
[modify] https://crrev.com/4ba1c8da24589b525f3d9f46339b5ee1f4a10354/chrome/app/generated_resources.grd
[modify] https://crrev.com/4ba1c8da24589b525f3d9f46339b5ee1f4a10354/chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc
[modify] https://crrev.com/4ba1c8da24589b525f3d9f46339b5ee1f4a10354/chrome/browser/ui/content_settings/content_setting_bubble_model.cc
[modify] https://crrev.com/4ba1c8da24589b525f3d9f46339b5ee1f4a10354/chrome/browser/ui/content_settings/content_setting_bubble_model.h
[modify] https://crrev.com/4ba1c8da24589b525f3d9f46339b5ee1f4a10354/chrome/browser/ui/views/content_setting_bubble_contents.cc
[modify] https://crrev.com/4ba1c8da24589b525f3d9f46339b5ee1f4a10354/chrome/browser/ui/views/content_setting_bubble_contents.h
[modify] https://crrev.com/4ba1c8da24589b525f3d9f46339b5ee1f4a10354/chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc
[add] https://crrev.com/4ba1c8da24589b525f3d9f46339b5ee1f4a10354/chrome/test/data/popup_blocker/popup-many-10.html

Status: Fixed (was: Assigned)
The NextAction date has arrived: 2017-09-29

Comment 24 Deleted

I'd like to keep these popovers sized at 320 if possible. We should accommodate overflow text on 2 lines when necessary, but we should remove the URL from the radio button. 

Pop-ups Blocked
The following pop-ups were blocked on this page

( ) Always allow pop-ups
( ) Continue blocking pop-ups


popup-today.png
1.5 MB View Download
popup-320.png
1.5 MB View Download
Im realizing that the mocks dont have this particular dialog sized at 320 so if it takes significant effort to force the sizing, then we shouldn't worry about it. 

https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(MD)/Secondary%20UI%20Previews%20and%20specs%20(exports)/Preview/11-Permissions/Pop%20ups#%2F18-Permissions_pop-ups.png%3Fz=width
In the above example, forcing the size to 320 would look fine. However, other websites which have much longer URLs would cause wrapping on the "Always allow popups from xxxx" radio button. The list of blocked popup URLs may also extend beyond the width of the scroll view.

Right now the dialog is requesting a width >320, so the next "harmony" width, 448, is selected.
Regarding the change to the "Always allow pop-ups" label change, there are other content blocked dialogs, such as blocked cookies, images, sound, javascript, etc... Nearly all of them include the URL in the radio button label to make it clear for what site the content is being blocked.

Should we remove the URL from all those other blocked content types?
Blocked popup dialog bubble "forced" to be 320px wide. The radio button label with the URL is elided, as is the list of blocked pop-up URLs.
BlockedPopupDialog320.png
10.0 KB View Download
It seems like including the URL in the radio label is clearer.  I'd rather not remove it if we can avoid doing so. Better to just let that radio label wrap, if that's possible (can we do multiline radio labels at all today?).

Then in the long run we shouldn't have an issue with the dialog being too wide, because the radio's preferred width will be based on the label preferred widths, which, being multiline, would be min(actual necessary width, n chars * average char width) for whatever |n| we select.  So the dialog will get wider only if there's such a long URL that if forces width expansion even when on its own line -- which is what we want.

In the short run, we could just do nothing at all (comment 26).
It seems that the radio button label elides when there isn't enough space to show the whole thing. A tool-tip will be displayed with the whole label content revealed. See the image from c#29 above.
Right, the eliding sorta works, but wrapping would be better IMO.
Here's the bubble clamped to 320 and the radio label wrapped. This required surfacing MultiLine on the LabelButton for the label.

Oh balance, I do prefer to let this dialog use the established snap points, however if the URL is ever really long, the dialog could still become uncontrollably wide. (IOW, it will snap to multiples of 16px harmony units above 512px).

There should still be some clamped limit on the width where the text on the radio buttons would then wrap.
BlockedPopupDialog320_wrapped.png
9.7 KB View Download
Here's the blocked plugins bubble with the clamped width and wrapped radio buttons. This looks a little cluttered.
BlockedPlugin.png
7.0 KB View Download
Cc: -bettes@chromium.org kylixrd@chromium.org
Owner: bettes@chromium.org
Need a look at the current, nearly final, incarnation of this dialog to determine what final tweaks and adjustments are needed.
When wrapping the radio labels, can we made the radio bubble vertically align with the first line of text on its label (and not with the entire label)?  That would help things look cleaner.
Cc: -kylixrd@chromium.org
Owner: kylixrd@chromium.org
+ wrapping the URL lgtm
+ clamped 320 lgtm

+ Vertical spacing: The spacing between radio labels in c33 and 34 is tighter than what i see on 65.0.3299.2 (see attachment). I prefer the larger spacing. 

+ Vertical alignment: I agree with c36, we should vertically align the first line of text to the component. 

https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(MD)/Secondary%20UI%20Previews%20and%20specs%20(exports)/Preview/11-Permissions/Microphone%20and%20Camera#%2F12-Permissions_microphone_and_camera_block.png%3Fz=width

Both of those adjustments should make this feel less cluttered. 
Screen Shot 2017-12-19 at 4.36.20 PM.png
55.0 KB View Download
Also, the scrollable section here looks fine when honoring the "client side padding." For reasons mentioned in the other bug, let's just do that everywhere. 

Apologies for the flip-flopping here. 
I'm not sure this looks cleaner with the radio bubble vertically aligned this way. Opinions?
Top Align Radio Button.png
9.7 KB View Download
Project Member

Comment 40 by bugdroid1@chromium.org, Jan 24 2018

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

commit c219bffe76f3d1a45f7799538e79f9e71da0e7e5
Author: Allen Bauer <kylixrd@chromium.org>
Date: Wed Jan 24 22:02:41 2018

Add vertical image centering/top alignment to LabelButton. This allows radio buttons (and checkboxes) to align the image with the first line if the label is wrapped.


Bug:  652031 
Change-Id: I6c6ac29540c83367936547788eda76d2c659af4c
Reviewed-on: https://chromium-review.googlesource.com/857332
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531700}
[modify] https://crrev.com/c219bffe76f3d1a45f7799538e79f9e71da0e7e5/ui/views/controls/button/image_button.h
[modify] https://crrev.com/c219bffe76f3d1a45f7799538e79f9e71da0e7e5/ui/views/controls/button/label_button.cc
[modify] https://crrev.com/c219bffe76f3d1a45f7799538e79f9e71da0e7e5/ui/views/controls/button/label_button_unittest.cc

@39: Definitely looks much better to me.  It does look like the radio button is a tiny bit high relative to the label.  Compare to comment 29, where the center of the radio button is basically aligned to the center of the label's x-height portion.  Ideally, we'd still have such alignment with the first line even in the multiline case.

This is a difference of 1 px in this case, though, and I can live with it.
Project Member

Comment 42 by bugdroid1@chromium.org, Feb 16 2018

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

commit e983a4e5bb74dda566a30dc325a4380b56cbd8b7
Author: Bret Sepulveda <bsep@chromium.org>
Date: Fri Feb 16 20:12:24 2018

Refactor ContentSettingBubbleContents' layout.

This patch aims to simplify the layout of ContentSettingBubbleContents
by adding a special View for media comboboxes and using BoxLayout for
the main content. Also includes some miscellaneous cleanup.

The visible changes of this patch are:
* The scroll view extends the full width of the dialog.
* The "?" button is the same size as versions in other dialogs.

Bug:  652031 
Change-Id: If920a89eee92ff3a0343be0be07cffe1b1b6084c
Reviewed-on: https://chromium-review.googlesource.com/920747
Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Commit-Queue: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537396}
[modify] https://crrev.com/e983a4e5bb74dda566a30dc325a4380b56cbd8b7/chrome/browser/ui/views/content_setting_bubble_contents.cc
[modify] https://crrev.com/e983a4e5bb74dda566a30dc325a4380b56cbd8b7/chrome/browser/ui/views/content_setting_bubble_contents.h

Comment 43 by bsep@chromium.org, Feb 17 2018

We probably had a separate bug for these, but they're all controlled by the same class so I'll leave it here: the MIDI and Location versions of this bubble don't have a title so they look pretty weird. It'd would be great to get that fixed.

I also noticed these tests are broken in ways the real dialogs aren't:
* ContentSettingBubbleDialogTest.InvokeUi_geolocation
* ContentSettingBubbleDialogTest.InvokeUi_midi_sysex 
* ContentSettingBubbleDialogTest.InvokeUi_protocol_handlers
Labels: Needs-Feedback
Tested the issue on latest chrome canary 66.0.3352.0 using Windows 10, We are able to see the pop-up similar to that of attachments shown in C#33, 34, 37 and 39. Attaching the screen shot of same. 

@kylixrd: Could you please have a look at the attachment from 66.0.3352.0 and help us in verifying the fix as ET team is not clear about the expected UI.

Thanks!
652031.png
43.5 KB View Download
Yes, that attachment is correct. The scroll-bars will only appear when the number of items exceeds around 4 items.
Cc: kylixrd@chromium.org
Owner: bettes@chromium.org
Time for some UX review
Status: Fixed (was: Assigned)

Sign in to add a comment