Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2788323002:
SafeBrowsing: update interstitial layouts (Closed)
|
Created:
3 years, 8 months ago by Nate Fischer Modified:
3 years, 8 months ago CC:
chromium-reviews, Jialiu Lin Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSafeBrowsing: change interstitial sizes
This CL changes the CSS max-height, max-width, etc. dimensions for
determining when to use mobile vs. desktop interstitial layouts. In
particular, it targets:
* wide and short views -> mobile landscape
* skinny and tall views -> mobile portrait
* wide and medium-height -> mobile landscape (w/ details on the same page)
The phablet layout has been removed because it seems to actually be better
to just use the mobile layout instead.
This also allows the mobile layout to remain centered even for very wide
views (parts of it were left-justified before), and reduces the
top-margin for the icon in the mobile layout, since we were leaving a
huge gap.
BUG=707481
Review-Url: https://codereview.chromium.org/2788323002
Cr-Commit-Position: refs/heads/master@{#466746}
Committed: https://chromium.googlesource.com/chromium/src/+/2f0527c9fcfdb3b53628acec1b3195563f71ec51
Patch Set 1 #Patch Set 2 : Allow details to show below when view height is 600-736px #Patch Set 3 : Don't let details button overlap back-to-safety #Patch Set 4 : Increase top margin for middle-sized interstitials #Patch Set 5 : Rebase off master #
Total comments: 10
Patch Set 6 : Switch to proportional margins #
Total comments: 4
Patch Set 7 : Address reviewer comments #
Messages
Total messages: 43 (22 generated)
The CQ bit was checked by ntfschr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ntfschr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== SafeBrowsing: change interstitial sizes This CL changes the CSS max-height, max-width, etc. dimensions for determining when to use mobile vs. desktop interstitial layouts. In particular, it targets: * wide and short views -> mobile landscape * skinny and tall views -> mobile portrait * wide and medium-height -> mobile landscape This CL also removes the phablet layout, since it typically just lends to poor visibility (as compared to the mobile layout). This also allows the mobile layout to remain centered even for very wide views (parts of it were left-justified before), and reduces the top-margin for the icon in the mobile layout, since we were leaving a huge gap. BUG=707481 ========== to ========== SafeBrowsing: change interstitial sizes This CL changes the CSS max-height, max-width, etc. dimensions for determining when to use mobile vs. desktop interstitial layouts. In particular, it targets: * wide and short views -> mobile landscape * skinny and tall views -> mobile portrait * wide and medium-height -> mobile landscape (w/ details on the same page) This also allows the mobile layout to remain centered even for very wide views (parts of it were left-justified before), and reduces the top-margin for the icon in the mobile layout, since we were leaving a huge gap. BUG=707481 ==========
ntfschr@chromium.org changed reviewers: + edwardjung@chromium.org, nparker@chromium.org
Description was changed from ========== SafeBrowsing: change interstitial sizes This CL changes the CSS max-height, max-width, etc. dimensions for determining when to use mobile vs. desktop interstitial layouts. In particular, it targets: * wide and short views -> mobile landscape * skinny and tall views -> mobile portrait * wide and medium-height -> mobile landscape (w/ details on the same page) This also allows the mobile layout to remain centered even for very wide views (parts of it were left-justified before), and reduces the top-margin for the icon in the mobile layout, since we were leaving a huge gap. BUG=707481 ========== to ========== SafeBrowsing: change interstitial sizes This CL changes the CSS max-height, max-width, etc. dimensions for determining when to use mobile vs. desktop interstitial layouts. In particular, it targets: * wide and short views -> mobile landscape * skinny and tall views -> mobile portrait * wide and medium-height -> mobile landscape (w/ details on the same page) The phablet layout has been removed because it seems to actually be better to just use the mobile layout instead. This also allows the mobile layout to remain centered even for very wide views (parts of it were left-justified before), and reduces the top-margin for the icon in the mobile layout, since we were leaving a huge gap. BUG=707481 ==========
nparker@: for owners approval edwardjung@: for UX design For your convenience, I've uploaded screenshots here: * Slides, which show before/after comparisons: https://docs.google.com/a/google.com/presentation/d/1X0HveKR7A5FLrakxznB5bMyA... * All screenshots as PNGs: https://drive.google.com/drive/folders/0B6dOsuQZDwrMM2hESDFSZ1dVbjQ?usp=sharing I took the screenshots comparing Chrome stable (before changes) with a compiled desktop chromium (after changes), using DevTools to select view dimensions. Let me know if you want more screenshots. For the general motivation behind this, please see my presentation: https://docs.google.com/a/google.com/presentation/d/15YuYw1qQRKDJTz-2V_UIEcK1...
Nate, I'm currently OOO at a conference so won't be able to look at this until the end of the week. I'm sharing the deck with bettes@ our visual designer who may have comments. On 2017/04/04 23:32:56, Nate Fischer wrote: > nparker@: for owners approval > edwardjung@: for UX design > > For your convenience, I've uploaded screenshots here: > > * Slides, which show before/after comparisons: > https://docs.google.com/a/google.com/presentation/d/1X0HveKR7A5FLrakxznB5bMyA... > * All screenshots as PNGs: > https://drive.google.com/drive/folders/0B6dOsuQZDwrMM2hESDFSZ1dVbjQ?usp=sharing > > I took the screenshots comparing Chrome stable (before changes) with a compiled > desktop chromium (after changes), using DevTools to select view dimensions. Let > me know if you want more screenshots. > > For the general motivation behind this, please see my presentation: > https://docs.google.com/a/google.com/presentation/d/15YuYw1qQRKDJTz-2V_UIEcK1...
On 2017/04/05 at 12:54:37, edwardjung wrote: > Nate, I'm currently OOO at a conference so won't be able to look at this until the end of the week. I'm sharing the deck with bettes@ our visual designer who may have comments. > > > On 2017/04/04 23:32:56, Nate Fischer wrote: > > nparker@: for owners approval > > edwardjung@: for UX design > > > > For your convenience, I've uploaded screenshots here: > > > > * Slides, which show before/after comparisons: > > https://docs.google.com/a/google.com/presentation/d/1X0HveKR7A5FLrakxznB5bMyA... > > * All screenshots as PNGs: > > https://drive.google.com/drive/folders/0B6dOsuQZDwrMM2hESDFSZ1dVbjQ?usp=sharing > > > > I took the screenshots comparing Chrome stable (before changes) with a compiled > > desktop chromium (after changes), using DevTools to select view dimensions. Let > > me know if you want more screenshots. > > > > For the general motivation behind this, please see my presentation: > > https://docs.google.com/a/google.com/presentation/d/15YuYw1qQRKDJTz-2V_UIEcK1... Thanks, Edward! Also, I just uploaded screenshots for Nexus 5X and 6P for both malware and HTTPS SSL interstitials.
> Thanks, Edward! Also, I just uploaded screenshots for Nexus 5X and 6P for both > malware and HTTPS SSL interstitials. Thanks Nate, the stylesheet is shared with the network errors. Could you add some screen shots for the net error pages to ensure they are not impacted negatively. chrome://network-error/-106 and chrome://network-error/-100 are good ones to check. I'm happy to patch and do some testing myself but that will have to wait until I get back to the office on Monday.
On 2017/04/06 at 13:01:50, edwardjung wrote: > > Thanks, Edward! Also, I just uploaded screenshots for Nexus 5X and 6P for both > > malware and HTTPS SSL interstitials. > > Thanks Nate, the stylesheet is shared with the network errors. Could you add some screen shots for the net error pages to ensure they are not impacted negatively. chrome://network-error/-106 and chrome://network-error/-100 are good ones to check. > I uploaded screenshots of both those pages--they're at the end of the slide deck. Very little impact on the dino page, and the other net error seems to me to have improved visibility along the lines of the other interstitials. > I'm happy to patch and do some testing myself but that will have to wait until I get back to the office on Monday.
I had a chat with maxwalker@ (our designer who is working on webview safe browsing interstitials) regarding this update. There are a number of screen sizes where we feel that the space could be better utilised, especially on desktop middling sizes where there isn't a need to anchor the buttons and switch modes so quickly where we do have enough space to accommodate the details. Here are some of the example screenshots from patching this CL: https://drive.google.com/drive/folders/0B4AXZ3lfv81gUDB5MDdzNzdHYVU?usp=sharing We want to do a bit more testing around this. It's possible we may need to tweak the breakpoints a bit more and do a bit more padding adjustment. On 2017/04/07 04:44:31, Nate Fischer wrote: > On 2017/04/06 at 13:01:50, edwardjung wrote: > > > Thanks, Edward! Also, I just uploaded screenshots for Nexus 5X and 6P for > both > > > malware and HTTPS SSL interstitials. > > > > Thanks Nate, the stylesheet is shared with the network errors. Could you add > some screen shots for the net error pages to ensure they are not impacted > negatively. chrome://network-error/-106 and chrome://network-error/-100 are good > ones to check. > > > > I uploaded screenshots of both those pages--they're at the end of the slide > deck. Very little impact on the dino page, and the other net error seems to me > to have improved visibility along the lines of the other interstitials. > > > I'm happy to patch and do some testing myself but that will have to wait until > I get back to the office on Monday.
On 2017/04/11 at 14:58:55, edwardjung wrote: > I had a chat with maxwalker@ (our designer who is working on webview safe browsing interstitials) regarding this update. > > There are a number of screen sizes where we feel that the space could be better utilised, especially on desktop middling sizes where there isn't a need to anchor the buttons and switch modes so quickly where we do have enough space to accommodate the details. > > Here are some of the example screenshots from patching this CL: > https://drive.google.com/drive/folders/0B4AXZ3lfv81gUDB5MDdzNzdHYVU?usp=sharing > > We want to do a bit more testing around this. It's possible we may need to tweak the breakpoints a bit more and do a bit more padding adjustment. > > > On 2017/04/07 04:44:31, Nate Fischer wrote: > > On 2017/04/06 at 13:01:50, edwardjung wrote: > > > > Thanks, Edward! Also, I just uploaded screenshots for Nexus 5X and 6P for > > both > > > > malware and HTTPS SSL interstitials. > > > > > > Thanks Nate, the stylesheet is shared with the network errors. Could you add > > some screen shots for the net error pages to ensure they are not impacted > > negatively. chrome://network-error/-106 and chrome://network-error/-100 are good > > ones to check. > > > > > > > I uploaded screenshots of both those pages--they're at the end of the slide > > deck. Very little impact on the dino page, and the other net error seems to me > > to have improved visibility along the lines of the other interstitials. > > > > > I'm happy to patch and do some testing myself but that will have to wait until > > I get back to the office on Monday. Sure thing. If we can do even better than I did here, I think that's great news--I tried to keep changes simple to make it easier to review.
Edward, anything you want me to try out here? It wasn't clear to me if you want action from me or not.
On 2017/04/12 16:10:11, Nate Fischer wrote: > Edward, anything you want me to try out here? It wasn't clear to me if you want > action from me or not. Sorry, was OOO. I'd recorded a video showing various states where the layout is not optimal for the screen size. If you want to have a look I think these are main problem areas on desktop: + (7 seconds in the video) tall narrow layouts, we can switch to the anchored buttons as on mobile, but provide proportional top padding at the top. + (29 seconds in the video) Wide but shallow layouts. Again provide proportional padding at the top. On desktop there shouldn't be anchored buttons in any of the wider layouts. When I get the chance, I can see if I can locate the original prototypes where I think I did do a proportional top padding version.
On 2017/04/20 at 09:55:09, edwardjung wrote: > On 2017/04/12 16:10:11, Nate Fischer wrote: > > Edward, anything you want me to try out here? It wasn't clear to me if you want > > action from me or not. > > Sorry, was OOO. I'd recorded a video showing various states where the layout is not optimal for the screen size. > > If you want to have a look I think these are main problem areas on desktop: > + (7 seconds in the video) tall narrow layouts, we can switch to the anchored buttons as on mobile, but provide proportional top padding at the top. > + (29 seconds in the video) Wide but shallow layouts. Again provide proportional padding at the top. On desktop there shouldn't be anchored buttons in any of the wider layouts. > > When I get the chance, I can see if I can locate the original prototypes where I think I did do a proportional top padding version. Thanks, Edward! I'll take a look and see if I can address the issues.
The CQ bit was checked by ntfschr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
On 2017/04/20 at 19:03:41, Nate Fischer wrote:
> On 2017/04/20 at 09:55:09, edwardjung wrote:
> > On 2017/04/12 16:10:11, Nate Fischer wrote:
> > > Edward, anything you want me to try out here? It wasn't clear to me if you
want
> > > action from me or not.
> >
> > Sorry, was OOO. I'd recorded a video showing various states where the layout
is not optimal for the screen size.
> >
> > If you want to have a look I think these are main problem areas on desktop:
> > + (7 seconds in the video) tall narrow layouts, we can switch to the
anchored buttons as on mobile, but provide proportional top padding at the top.
> > + (29 seconds in the video) Wide but shallow layouts. Again provide
proportional padding at the top. On desktop there shouldn't be anchored buttons
in any of the wider layouts.
> >
> > When I get the chance, I can see if I can locate the original prototypes
where I think I did do a proportional top padding version.
>
> Thanks, Edward! I'll take a look and see if I can address the issues.
PTAL. I increased the margin back to 96px if the page is tall enough to support
it:
- If a narrow interstitial (mobile portrait) is taller than 650px,
increase the top margin to 96px.
- If a wide interstitial (mobile landscape) is taller than 500px,
increase the top margin to 96px.
This targets the sizes 369x841 and 1113x709 (if these were not the sizes you
referred to, please correct me).
You can find screenshots containing this patchset at:
https://drive.google.com/open?id=0B6dOsuQZDwrMTG9GREg0MW54am8
The CQ bit was checked by ntfschr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks Nate, I think this is mostly there. I would like to switch to proportional top margins using vh values. This is well supported now. I do need to check these changes iOS but it seems to be supported in the latest WkWebview. I've shared videos with you. https://codereview.chromium.org/2788323002/diff/80001/components/security_int... File components/security_interstitials/core/browser/resources/interstitial_v2.css (right): https://codereview.chromium.org/2788323002/diff/80001/components/security_int... components/security_interstitials/core/browser/resources/interstitial_v2.css:438: margin: 24px auto 12px; Switch this to: margin: 3vh auto 12px; https://codereview.chromium.org/2788323002/diff/80001/components/security_int... components/security_interstitials/core/browser/resources/interstitial_v2.css:467: margin-top: 96px; Switch this to use 10vh over 96px. This provides a proportional top margin to the vertical height. https://codereview.chromium.org/2788323002/diff/80001/components/security_int... components/security_interstitials/core/browser/resources/interstitial_v2.css:486: margin-top: 20px; margin-top: 7vh; https://codereview.chromium.org/2788323002/diff/80001/components/security_int... components/security_interstitials/core/browser/resources/interstitial_v2.css:492: margin-top: 96px; margin-top: 10vh; https://codereview.chromium.org/2788323002/diff/80001/components/security_int... File components/security_interstitials/core/browser/resources/interstitial_v2_mobile.js (right): https://codereview.chromium.org/2788323002/diff/80001/components/security_int... components/security_interstitials/core/browser/resources/interstitial_v2_mobile.js:16: '(max-height: 640px) and (min-height: 240px) and ' + This max-height should match the value used in the css - 736px. otherwise you get weird state of anchor buttons and the details text doesn't appear over the main text. Specifically for net errors there isn't enough space underneath the main content.
I don't have an iOS device to try this out on, but this LGTM otherwise. https://codereview.chromium.org/2788323002/diff/80001/components/security_int... File components/security_interstitials/core/browser/resources/interstitial_v2.css (right): https://codereview.chromium.org/2788323002/diff/80001/components/security_int... components/security_interstitials/core/browser/resources/interstitial_v2.css:467: margin-top: 96px; On 2017/04/21 at 12:16:30, edwardjung wrote: > Switch this to use 10vh over 96px. This provides a proportional top margin to the vertical height. Done https://codereview.chromium.org/2788323002/diff/80001/components/security_int... components/security_interstitials/core/browser/resources/interstitial_v2.css:486: margin-top: 20px; On 2017/04/21 12:16:30, edwardjung wrote: > margin-top: 7vh; Done. https://codereview.chromium.org/2788323002/diff/80001/components/security_int... components/security_interstitials/core/browser/resources/interstitial_v2.css:492: margin-top: 96px; On 2017/04/21 12:16:30, edwardjung wrote: > margin-top: 10vh; Done. https://codereview.chromium.org/2788323002/diff/80001/components/security_int... File components/security_interstitials/core/browser/resources/interstitial_v2_mobile.js (right): https://codereview.chromium.org/2788323002/diff/80001/components/security_int... components/security_interstitials/core/browser/resources/interstitial_v2_mobile.js:16: '(max-height: 640px) and (min-height: 240px) and ' + On 2017/04/21 12:16:30, edwardjung wrote: > This max-height should match the value used in the css - 736px. otherwise you > get weird state of anchor buttons and the details text doesn't appear over the > main text. Specifically for net errors there isn't enough space underneath the > main content. Done. I couldn't see a difference either way--I'll trust your judgment here.
Just a couple of other changes. Thanks. https://codereview.chromium.org/2788323002/diff/80001/components/security_int... File components/security_interstitials/core/browser/resources/interstitial_v2_mobile.js (right): https://codereview.chromium.org/2788323002/diff/80001/components/security_int... components/security_interstitials/core/browser/resources/interstitial_v2_mobile.js:16: '(max-height: 640px) and (min-height: 240px) and ' + No difference to the SSL, malware interstitials. This is specifically for some network error pages. https://codereview.chromium.org/2788323002/diff/100001/components/security_in... File components/security_interstitials/core/browser/resources/interstitial_v2.css (right): https://codereview.chromium.org/2788323002/diff/100001/components/security_in... components/security_interstitials/core/browser/resources/interstitial_v2.css:438: margin: 24px auto 12px; Switch this to: margin: 7vh auto 12px; https://codereview.chromium.org/2788323002/diff/100001/components/security_in... components/security_interstitials/core/browser/resources/interstitial_v2.css:484: (orientation: portrait) { Nit: align the opening brackets
https://codereview.chromium.org/2788323002/diff/100001/components/security_in... File components/security_interstitials/core/browser/resources/interstitial_v2.css (right): https://codereview.chromium.org/2788323002/diff/100001/components/security_in... components/security_interstitials/core/browser/resources/interstitial_v2.css:438: margin: 24px auto 12px; On 2017/04/24 at 10:30:41, edwardjung wrote: > Switch this to: > margin: 7vh auto 12px; done https://codereview.chromium.org/2788323002/diff/100001/components/security_in... components/security_interstitials/core/browser/resources/interstitial_v2.css:484: (orientation: portrait) { On 2017/04/24 at 10:30:41, edwardjung wrote: > Nit: align the opening brackets done
lgtm
On 2017/04/24 19:17:24, edwardjung wrote: > lgtm Thanks for the help on this, Edward! nparker@, can you review for owners approval?
echoing edwarjung's LGTM, for OWNERS. Thanks Nate.
The CQ bit was checked by ntfschr@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493064745494870,
"parent_rev": "ebc9428960482240e8a1114b7b795cabf42a6b82", "commit_rev":
"2f0527c9fcfdb3b53628acec1b3195563f71ec51"}
Message was sent while issue was closed.
Description was changed from ========== SafeBrowsing: change interstitial sizes This CL changes the CSS max-height, max-width, etc. dimensions for determining when to use mobile vs. desktop interstitial layouts. In particular, it targets: * wide and short views -> mobile landscape * skinny and tall views -> mobile portrait * wide and medium-height -> mobile landscape (w/ details on the same page) The phablet layout has been removed because it seems to actually be better to just use the mobile layout instead. This also allows the mobile layout to remain centered even for very wide views (parts of it were left-justified before), and reduces the top-margin for the icon in the mobile layout, since we were leaving a huge gap. BUG=707481 ========== to ========== SafeBrowsing: change interstitial sizes This CL changes the CSS max-height, max-width, etc. dimensions for determining when to use mobile vs. desktop interstitial layouts. In particular, it targets: * wide and short views -> mobile landscape * skinny and tall views -> mobile portrait * wide and medium-height -> mobile landscape (w/ details on the same page) The phablet layout has been removed because it seems to actually be better to just use the mobile layout instead. This also allows the mobile layout to remain centered even for very wide views (parts of it were left-justified before), and reduces the top-margin for the icon in the mobile layout, since we were leaving a huge gap. BUG=707481 Review-Url: https://codereview.chromium.org/2788323002 Cr-Commit-Position: refs/heads/master@{#466746} Committed: https://chromium.googlesource.com/chromium/src/+/2f0527c9fcfdb3b53628acec1b31... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/2f0527c9fcfdb3b53628acec1b31...
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2842633002/ by hcarmona@chromium.org. The reason for reverting is: This looks like the cause of failures here: https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20C... First seen here: https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20C.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
