New issue
Advanced search Search tips

Issue 807075 link

Starred by 4 users

Issue metadata

Status: Assigned
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug
Team-Security-UX



Sign in to add a comment

Do not animate HTTP_SHOW_WARNING to DANGEROUS transition

Project Member Reported by est...@chromium.org, Jan 29 2018

Issue description

With certain settings of the #enable-mark-http-as flag, the gray Not Secure chip can transition to a red Not Secure chip when typing in a form field. This transition is a bit confusing/messy-looking, so we'd like to avoid animating that transition specifically (HTTP_SHOW_WARNING -> DANGEROUS).
 

Comment 1 by est...@chromium.org, Jan 30 2018

Labels: Hotlist-HttpBad

Comment 2 by est...@chromium.org, Jan 31 2018

Attached are the before (animated.gif) and after (http-bad-no-animation.gif). Feedback from UX was that the animation is confusing in this case because we're not changing the message/verbose text, just the color/icon. Removing the animation makes it feel like we're highlighting the current security state rather than changing it, which better matches what we're trying to communicate in this transition.
animated.gif
5.0 MB View Download
http-bad-no-animation.gif
8.3 MB View Download
It looks like the bug here is that we implement the animation as a full hide of the old state followed by a full show of the new state.  We should probably just animate the width of the chip from old size to new size, changing the string/icon/color immediately at the start of the transition.

Then in practice, we shouldn't need magic handling of this specific transition, because you'll effectively get no animation.  And we should look better in any other cases where we're animating one with-chip state to another with-chip state to boot (I don't think this is specific to this particular pair of states; we'd have the same problem in principle with any other pair that both have a nonempty chip.)
(Note that if you really wanted you could try to animate the color change, which we have utilities to compute, and crossfade the icon/text rather than just swapping, but those are probably not necessary.)

Comment 5 by est...@chromium.org, Jan 31 2018

> It looks like the bug here is that we implement the animation as a full hide of the old state followed by a full show of the new state.

I'm not sure that's true, I'll have to check with UX. For other transitions, I think the full animation might reasonably draw attention and communicate that the security state of the page has really changed, which is not what we want to communicate in this case.
I'm somewhat skeptical, but even in that case, it sounds like the proper determination would be in the code that has the actual strings for the start and end state; presumably "strings are the same" is a good proxy for "the security state of the page hasn't really changed".

My core concern here is that the code change to implement this enshrines something special about transitioning specifically from WARNING to DANGEROUS.  But assuming it were possible for a page to transition from DANGEROUS to WARNING, the same concern would apply.  And pretend we had three different states whose text was "Might be OK" with different icons; we probably wouldn't want to animate any of those changes either.  These thought experiments are dumb, but they suggest (to me) that we're encoding the decision at the wrong level.

Maybe all this is an artifact of the discrepancy between you saying that the security state here hasn't meaningfully changed, and the strings in these states being identical, versus them actually being different states with different icons and colors.  I might argue that if we really think these are the same thing, they ought to _be_ the same thing.  But perhaps the current state is a temporary transition to a world where they _are_ the same thing and we just can't be there right now :)

Comment 7 by est...@chromium.org, Jan 31 2018

Ok, so we have the following 3 options:
(a) For any transition, change the string/icon/color and then animate from old size to new size.
(b) For any transition, do not animate if the string is the same. (I think that would look similar in implementation to https://chromium-review.googlesource.com/c/chromium/src/+/891630, but saving/comparing the last updated location icon label instead of the last updated security level?)
(c) Specifically disable animation for HTTP_SHOW_WARNING to DANGEROUS transition.

I'd like to get PM/UX opinion on (a) and (b). Another consideration is that this change needs to get merged to M65 where we are running a 1% stable experiment to get metrics ahead of a public announcement. Given that, it would be good to keep the change small and localized to this particular experiment, rather than making a broader change that might risk breakage. Would you be okay landing the current CL (option c) if I then change it to option a or b after getting PM/UX input?
Yes, I trust you to follow up appropriately here :)

And yes, we could implement (b) similarly to (c).  I thought at first it would be more similar to (a), but that only makes sense if we're deciding not to animate when the string _length_ is the same, rather than when the string _content_ is the same.  For cases where two different strings have the same length, (b) still wants to animate, and thus the animation would have to have the old string contents, which are probably annoying to get at.

Comment 9 by est...@chromium.org, Jan 31 2018

Ok, thanks! I'll see what UX thinks and report back here.
Project Member

Comment 10 by bugdroid1@chromium.org, Jan 31 2018

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

commit 6bf91042a211c2ebda9fbe206c8f3f08952a1dbc
Author: Emily Stark <estark@google.com>
Date: Wed Jan 31 17:34:25 2018

Views: Do not animate HTTP_SHOW_WARNING to DANGEROUS transition

This transition (which is currently only possible with an experiment enabled)
can look messy/confusing when animated, so do not animate it.

Bug: 807075
Change-Id: Ib7bda303958739cd4840dbc57910c1c850c5e97a
Reviewed-on: https://chromium-review.googlesource.com/891630
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533312}
[modify] https://crrev.com/6bf91042a211c2ebda9fbe206c8f3f08952a1dbc/chrome/browser/ui/views/location_bar/location_bar_view.cc
[modify] https://crrev.com/6bf91042a211c2ebda9fbe206c8f3f08952a1dbc/chrome/browser/ui/views/location_bar/location_bar_view.h

Project Member

Comment 11 by bugdroid1@chromium.org, Jan 31 2018

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

commit b3bb303745ee75bf22d08914bac63e03c9753de7
Author: Emily Stark <estark@google.com>
Date: Wed Jan 31 17:56:40 2018

Mac: Do not animate HTTP_SHOW_WARNING to DANGEROUS transition

This transition (which is currently only possible with an experiment enabled)
can look messy/confusing when animated, so do not animate it.

Bug: 807075
Change-Id: I1dae69b64e0e9f457b87abf7c4ff45ba1f595dad
Reviewed-on: https://chromium-review.googlesource.com/891587
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533319}
[modify] https://crrev.com/b3bb303745ee75bf22d08914bac63e03c9753de7/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm

Labels: Merge-Request-65
Requesting merge for #10,#11 to M65. I've verified the changes on Canary. These are very small isolated animation tweaks that only affect a specific situation that can happen when an experiment is enabled. (The experiment will be enabled at 1% in 65 stable.)
Project Member

Comment 13 by sheriffbot@chromium.org, Feb 4 2018

Labels: -Merge-Request-65 Hotlist-Merge-Approved Merge-Approved-65
Your change meets the bar and is auto-approved for M65. Please go ahead and merge the CL to branch 3325 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 14 by bugdroid1@chromium.org, Feb 4 2018

Labels: -merge-approved-65 merge-merged-3325
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d0922385e912d9899e8d780c08918671f7e54aad

commit d0922385e912d9899e8d780c08918671f7e54aad
Author: Emily Stark <estark@google.com>
Date: Sun Feb 04 16:05:24 2018

Mac: Do not animate HTTP_SHOW_WARNING to DANGEROUS transition

This transition (which is currently only possible with an experiment enabled)
can look messy/confusing when animated, so do not animate it.

Bug: 807075
Change-Id: I1dae69b64e0e9f457b87abf7c4ff45ba1f595dad
Reviewed-on: https://chromium-review.googlesource.com/891587
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Emily Stark <estark@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#533319}(cherry picked from commit b3bb303745ee75bf22d08914bac63e03c9753de7)
Reviewed-on: https://chromium-review.googlesource.com/900560
Reviewed-by: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#283}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[modify] https://crrev.com/d0922385e912d9899e8d780c08918671f7e54aad/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm

Project Member

Comment 15 by bugdroid1@chromium.org, Feb 4 2018

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

commit a9136f4a29db344393536e65a842f2e01483dd1e
Author: Emily Stark <estark@google.com>
Date: Sun Feb 04 16:11:27 2018

Views: Do not animate HTTP_SHOW_WARNING to DANGEROUS transition

This transition (which is currently only possible with an experiment enabled)
can look messy/confusing when animated, so do not animate it.

TBR=estark@google.com

(cherry picked from commit 6bf91042a211c2ebda9fbe206c8f3f08952a1dbc)

Bug: 807075
Change-Id: Ib7bda303958739cd4840dbc57910c1c850c5e97a
Reviewed-on: https://chromium-review.googlesource.com/891630
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Emily Stark <estark@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#533312}
Reviewed-on: https://chromium-review.googlesource.com/900828
Reviewed-by: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#284}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[modify] https://crrev.com/a9136f4a29db344393536e65a842f2e01483dd1e/chrome/browser/ui/views/location_bar/location_bar_view.cc
[modify] https://crrev.com/a9136f4a29db344393536e65a842f2e01483dd1e/chrome/browser/ui/views/location_bar/location_bar_view.h

Labels: TE-Verified-M65 TE-Verified-65.0.3325.51
Tested this issue on Windows 10, Mac OS 10.12.6 and Ubuntu 14.04 on the latest Chrome Build 65.0.3325.51 by following the below steps.

1. Launched Chrome and enable the flag "Enabled (mark with a Not secure warning and dangerous on form edits)" in #enable-mark-http-as in chrome://flags.
2. Relaunched chrome and navigated to bbc.com and entered some text in search text field.
3. Can observe no animation on the Not Secure menu.
Attached is the screen cast for reference.

Hence adding TE verified labels as the fix is working as intended.

Thanks..
807075.webm
7.6 MB View Download
Status to "Verified"?

Sign in to add a comment