Harmony - update popups blocked and mixed-content dialogs |
||||||||||||||||||||
Issue descriptionWe need an easy way to bring up this dialog. Mock: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(MD)/Secondary%20UI%20Previews%20and%20specs%20(exports)/Preview/11-Permissions#%2F18-Permissions_pop-ups.png%3Fz=width
,
Dec 9 2016
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.
,
Dec 10 2016
OK, thank you.
,
Jan 24 2017
Per shrike, unassigning his Harmony bugs for now.
,
Mar 14 2017
,
Aug 9 2017
,
Aug 10 2017
,
Aug 10 2017
,
Aug 10 2017
,
Aug 10 2017
Assigning to Allen on the basis that this looks like it has a ScrollView in it :D
,
Aug 14 2017
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.
,
Aug 14 2017
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.
,
Aug 16 2017
I've set up some time to discuss in person.
,
Aug 16 2017
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.
,
Aug 17 2017
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.
,
Aug 23 2017
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
,
Sep 5 2017
,
Sep 11 2017
This is the mixed content dialog.
,
Sep 11 2017
,
Sep 11 2017
,
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
,
Sep 12 2017
,
Sep 29 2017
The NextAction date has arrived: 2017-09-29
,
Oct 31 2017
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
,
Nov 1 2017
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
,
Nov 8 2017
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.
,
Nov 20 2017
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?
,
Nov 20 2017
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.
,
Nov 21 2017
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).
,
Nov 21 2017
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.
,
Nov 21 2017
Right, the eliding sorta works, but wrapping would be better IMO.
,
Nov 30 2017
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.
,
Nov 30 2017
Here's the blocked plugins bubble with the clamped width and wrapped radio buttons. This looks a little cluttered.
,
Dec 11 2017
Need a look at the current, nearly final, incarnation of this dialog to determine what final tweaks and adjustments are needed.
,
Dec 12 2017
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.
,
Dec 20 2017
+ 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.
,
Dec 20 2017
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.
,
Jan 9 2018
I'm not sure this looks cleaner with the radio bubble vertically aligned this way. Opinions?
,
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
,
Jan 24 2018
@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.
,
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
,
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
,
Feb 22 2018
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!
,
Feb 22 2018
Yes, that attachment is correct. The scroll-bars will only appear when the number of items exceeds around 4 items.
,
Feb 28 2018
Time for some UX review
,
Apr 9 2018
|
||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||
Comment 1 by shrike@chromium.org
, Oct 11 2016