1175678 - Update icons for different Mixed Content Blocking states in the URL bar

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Brian Grinstead [:bgrins]]

Tracking Protection is going to keep the shield, so we need a new icon to anchor the mixed content blocking doorhanger onto.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

We have three states (instead of the one yield sign):

1) Green lock, gray yield sign.
2) Yellow yield sign.
3) Gray lock, yellow yield sign.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

4) A grey lock, with a red cross (Active MCB disabled)

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

In the design [1] there actually are only three MCB states:

1) Mixed Passive Content loaded
2) Mixed Active Content blocked (and maybe passive loaded)
3) Mixed Active Content loaded (and maybe passive too)

For those states I assume we have the following icons:

1) Green lock, gray yield sign - http://i.imgur.com/RSusBmm.png
2) Gray lock, yellow yield sign - http://i.imgur.com/NVADplu.png
3) Gray lock, red strike-through - http://i.imgur.com/8sUvP3i.png

I'm confused because in the design, the icons for 1 + 2 seem exactly the other way around. Now we only block active content, never passive content (except with that one pref set). So it seems we should display "passive content loaded" as more secure than "active content blocked, passive content loaded"? The security of both is not any different (because the active content is blocked) but we would make site owners (and users) aware that the sites tries to load active content.

Aislinn, Tanvi, is what I said above correct?

[1] https://photos-3.dropbox.com/t/2/AAA_m0YMrHZl8PnYO13v-A3L-zhr8JaPFr82dyIkEg1Q-A/12/12247711/png/32x32/1/1435723200/0/2/DV%20SSL%20url%20and%20doorhanger%20spec.png/CJ_F6wUgASACIAMgBygBKAIoAygH/vg6PZdJtX0e9CM4qLwvBeEyf5_pCIJtD_5KkcH5nqn4?size_mode=5

Comment 4

[Tanvi Vyas[:tanvi]]

Here are the states and there corresponding icons:

0) no mixed active detected, no mixed passive detected - green lock

1) mixed active blocked, no mixed passive detected- green lock with grey triangle

2) mixed active blocked, mixed passive loaded -  grey lock with yellow triangle 

3) no mixed active detected, mixed passive loaded - grey lock with yellow triangle

4) mixed active loaded, mixed passive may or may not be detected and loaded - lock with an x

Note that this does mean  users will have trouble distinguishing between case 2 and case 3.  How will they know if there's just mixed passive content on the page or if there's also mixed active that we've blocked? We decided we were okay with this ambiguity,  since we preferred that over adding yet another icon  to this already complex problem.  The user can figure out if something is being blocked by clicking on the grey lock with the yellow triangle and learning more.The user can figure out if something is being blocked by clicking on the Greylock with the yellow triangle and learning more.  I'm happy to discuss this more tomorrow.   I know Anthony had a lot of questions about the fennec UI  which may be even more complicated than the desktop one.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Great, thanks for clarifying! That means I need to add a few new checks/states to cover those correctly.

Comment 6

[Ash Grigas]

(In reply to Tanvi Vyas [:tanvi] from comment #4)
> Here are the states and there corresponding icons:
> 
> 0) no mixed active detected, no mixed passive detected - green lock
> 
> 1) mixed active blocked, no mixed passive detected- green lock with grey
> triangle
> 
> 2) mixed active blocked, mixed passive loaded -  grey lock with yellow
> triangle 
> 
> 3) no mixed active detected, mixed passive loaded - grey lock with yellow
> triangle
> 
> 4) mixed active loaded, mixed passive may or may not be detected and loaded
> - lock with an x
> 
> Note that this does mean  users will have trouble distinguishing between
> case 2 and case 3.  How will they know if there's just mixed passive content
> on the page or if there's also mixed active that we've blocked? We decided
> we were okay with this ambiguity,  since we preferred that over adding yet
> another icon  to this already complex problem.  The user can figure out if
> something is being blocked by clicking on the grey lock with the yellow
> triangle and learning more.The user can figure out if something is being
> blocked by clicking on the Greylock with the yellow triangle and learning
> more.  I'm happy to discuss this more tomorrow.   I know Anthony had a lot
> of questions about the fennec UI  which may be even more complicated than
> the desktop one.

One minor correction to state 4 - the new icon Bram did is a lock with red line through it.

Comment 7

[Tanvi Vyas[:tanvi]]

I'm unclear about the differences between EV and DV.  When do we show a plain green lock and when do we show a green lock with a check bar?  And where?  In the url bar icon or the drop down menu?

Comment 8

[Tanvi Vyas[:tanvi]]

Adding example pages that all happen to be EV.  I don't have DV test pages.  Also notes the icons I have listed below with each state are the icons that go in the url bar itself.

> 0) no mixed active detected, no mixed passive detected - green lock
>
https://people.mozilla.org/~tvyas/index.html

> 1) mixed active blocked, no mixed passive detected- green lock with grey
> triangle
>
https://people.mozilla.org/~tvyas/mixedcontent.html 


> 2) mixed active blocked, mixed passive loaded -  grey lock with yellow
> triangle 
>
https://people.mozilla.org/~tvyas/mixedboth.html

> 3) no mixed active detected, mixed passive loaded - grey lock with yellow
> triangle
>
https://people.mozilla.org/~tvyas/mixeddisplay.html


> 4) mixed active loaded, mixed passive may or may not be detected and loaded
> - lock with a strikethrough
>
https://people.mozilla.org/~tvyas/mixedcontent.html AFTER your disable protection - this shows you mixed active loaded, no mixed passive detected
AND
https://people.mozilla.org/~tvyas/mixedboth.html AFTER you disable protection - this shows you mixed active loaded, mixed passive loaded

Comment 9

[Stephen Horlander [:shorlander]]

Attachment: identity-not-secure-16.svg

16 x 16 version of not secure site icon.

Comment 10

[Stephen Horlander [:shorlander]]

Attachment: identity-not-secure-16.svg

Oops, discovered an error.

Comment 11

[Tanvi Vyas[:tanvi]]

Note that you could also end up in a state like this:
http://people.mozilla.org/~tvyas/mixedgrandiframe.html

HTTP top level page.  Embeds an HTTPS iframe.  The HTTPS iframe has mixed active content that the browser blocks.  Right now, we show a shield icon to indicate the user that something is blocked on the page.  Next to it is the grey globe.  I'm not sure how to handle this case in the new control center.  I don't want to add a state for a globe with a grey triangle over it in the url bar :(  We could just punt on the problem - don't show the user any visible indication that we've blocked things on the page.  If they somehow realize to click the globe icon, we could add an override option in the control center.

Screen shot: https://people.mozilla.org/~tvyas/http-page-mixed-content-shield.png
Test case: http://people.mozilla.org/~tvyas/mixedgrandiframe.html
Background on why we have this behavior: https://bugzilla.mozilla.org/show_bug.cgi?id=824871#c2

Comment 12

[Ash Grigas]

(In reply to Tanvi Vyas [:tanvi] from comment #11)
> Note that you could also end up in a state like this:
> http://people.mozilla.org/~tvyas/mixedgrandiframe.html
> 
> HTTP top level page.  Embeds an HTTPS iframe.  The HTTPS iframe has mixed
> active content that the browser blocks.  Right now, we show a shield icon to
> indicate the user that something is blocked on the page.  Next to it is the
> grey globe.  I'm not sure how to handle this case in the new control center.
> I don't want to add a state for a globe with a grey triangle over it in the
> url bar :(  We could just punt on the problem - don't show the user any
> visible indication that we've blocked things on the page.  If they somehow
> realize to click the globe icon, we could add an override option in the
> control center.
> 
> Screen shot:
> https://people.mozilla.org/~tvyas/http-page-mixed-content-shield.png
> Test case: http://people.mozilla.org/~tvyas/mixedgrandiframe.html
> Background on why we have this behavior:
> https://bugzilla.mozilla.org/show_bug.cgi?id=824871#c2

Yeah agree we shouldn't add another unique state of the globe. I think the globe alone is ok. Since the page is http even if elements are https, the overall message we want to send is is not secure. I'll add a state for this here:
https://www.dropbox.com/s/2xpibze26vfmmyw/not%20secure%20url%20and%20doorhanger.png?dl=0

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1175678-Update-icons-for-different-Mixed-Content.patch

These states and modes in the gIdentityHandler are a mess. We map flags to internal modes and then map the mode back to className-flags. I'd love to clean this up but that's a little out of scope.

Combining the EV/DV icons is pending rbarnes' signing it off in bug 1180859. The patch here does that already.

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8630570 [details] [diff] [review]
0001-Bug-1175678-Update-icons-for-different-Mixed-Content.patch

Review of attachment 8630570 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/osx/jar.mn
@@ +49,5 @@
>    skin/classic/browser/[e-mail]
>    skin/classic/browser/heartbeat-icon.svg                   (../shared/heartbeat-icon.svg)
>    skin/classic/browser/heartbeat-star-lit.svg               (../shared/heartbeat-star-lit.svg)
>    skin/classic/browser/heartbeat-star-off.svg               (../shared/heartbeat-star-off.svg)
> +  skin/classic/browser/identity-not-secure.svg              (../shared/identity-not-secure.svg)

Yay, SVG in …/shared/!

I kinda wish this wasn't directly in the shared directory (e.g. in themes/shared/controlcenter/?). I think it's hard to find things with images and css files mixed at the top. An identity block directory would also work and identity-block.inc.css could move to either one.

Unfortunately, I wouldn't be surprised if this affects tpaint/ts_paint so it would be good to do a try run to avoid the noise/churn on talos.

::: browser/themes/shared/devtools/netmonitor.inc.css
@@ +143,4 @@
>  }
>  
>  .security-state-insecure {
> +  list-style-image: url(chrome://browser/skin/identity-not-secure.svg);

I assume you tested that these appear at the correct dimensions still.

::: browser/themes/shared/identity-block.inc.css
@@ +59,5 @@
>  
>  #page-proxy-favicon {
>    width: 16px;
>    height: 16px;
> +  list-style-image: url(chrome://browser/skin/identity-not-secure.svg);

/me wishes you were using mozreview so it would be easier to skim the file path change and code movement.

@@ +78,4 @@
>  }
>  
> +.mixedActiveContent > #page-proxy-favicon[pageproxystate="valid"] {
> +  list-style-image: url(chrome://browser/skin/identity-mixed-active-loaded.svg);

I'm guessing you moved the mixedActiveContent rule after the mixedDisplayContent one to ensure that it overrides the latter if both apply since it's more important?

@@ +90,3 @@
>    .chromeUI > #page-proxy-favicon[pageproxystate="valid"] {
>      list-style-image: url(chrome://branding/content/[e-mail]);
> +    -moz-image-region: rect(0, 32px, 32px, 0);

We should get a bug on file to get this as SVG so our own branding doesn't look worse than the other icons at scale factors between 1 and 2 (exclusive).

@@ +95,3 @@
>  
> +#page-proxy-favicon[pageproxystate="invalid"] {
> +  opacity: 0.3;

Was it intentional that you moved this ruleset below the @media? If not, could you put it back so blame doesn't change.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Matthew N. [:MattN] from comment #14)
> /me wishes you were using mozreview so it would be easier to skim the file
> path change and code movement.

I'd happily do that if it supported git. I don't feel like going back to hg :|

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Matthew N. [:MattN] from comment #14)
> >    .chromeUI > #page-proxy-favicon[pageproxystate="valid"] {
> >      list-style-image: url(chrome://branding/content/[e-mail]);
> > +    -moz-image-region: rect(0, 32px, 32px, 0);
> 
> We should get a bug on file to get this as SVG so our own branding doesn't
> look worse than the other icons at scale factors between 1 and 2 (exclusive).

Filed bug 1181541.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Matthew N. [:MattN] from comment #14)
> Yay, SVG in …/shared/!
> 
> I kinda wish this wasn't directly in the shared directory (e.g. in
> themes/shared/controlcenter/?). I think it's hard to find things with images
> and css files mixed at the top. An identity block directory would also work
> and identity-block.inc.css could move to either one.

Moved the SVGs and the .inc.css file to browser/themes/shared/identity-block/.

> Unfortunately, I wouldn't be surprised if this affects tpaint/ts_paint so it
> would be good to do a try run to avoid the noise/churn on talos.

Will do!

> ::: browser/themes/shared/devtools/netmonitor.inc.css
> @@ +143,4 @@
> >  }
> >  
> >  .security-state-insecure {
> > +  list-style-image: url(chrome://browser/skin/identity-not-secure.svg);
> 
> I assume you tested that these appear at the correct dimensions still.

Yes, looks good.

> > +.mixedActiveContent > #page-proxy-favicon[pageproxystate="valid"] {
> > +  list-style-image: url(chrome://browser/skin/identity-mixed-active-loaded.svg);
> 
> I'm guessing you moved the mixedActiveContent rule after the
> mixedDisplayContent one to ensure that it overrides the latter if both apply
> since it's more important?

I ordered them after severity of the icons itself, that probably doesn't make a lot of sense. I reordered the patch to minimize the diff and reduce blame loss.

> > +#page-proxy-favicon[pageproxystate="invalid"] {
> > +  opacity: 0.3;
> 
> Was it intentional that you moved this ruleset below the @media? If not,
> could you put it back so blame doesn't change.

Done.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #17)
> (In reply to Matthew N. [:MattN] from comment #14)
> > Unfortunately, I wouldn't be surprised if this affects tpaint/ts_paint so it
> > would be good to do a try run to avoid the noise/churn on talos.
> 
> Will do!

http://compare-talos.mattn.ca/?oldRevs=404f59795314&newRev=b6502eb1127c&server=graphs.mozilla.org&submit=true

Looks like tpaint/ts_paint is not affected, yay.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/568a7fb27376

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/24248dc22c89

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c12c9d82b7c5

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/568a7fb27376
https://hg.mozilla.org/mozilla-central/rev/24248dc22c89
https://hg.mozilla.org/mozilla-central/rev/c12c9d82b7c5

Comment 23

[Catalin Varga [QA][:VarCat]]

Verified as fixed using:

Win 7 x64, Mac Os X 10.8.5, Ubuntu 12.04 x86
FF 42
Build Id: 20150722030205

Comment 24

[Paul Silaghi, QA [:pauly]]

43.0a1 (2015-09-09) Win 7
Is it ok how "https" looks when the protection is disabled?
http://i.imgur.com/xciyxd3.png

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Paul Silaghi, QA [:pauly] from comment #24)
> 43.0a1 (2015-09-09) Win 7
> Is it ok how "https" looks when the protection is disabled?
> http://i.imgur.com/xciyxd3.png

That's bug 1186892.

Comment 26

[Paul Silaghi, QA [:pauly]]

Thanks Tim.
Marking verified per comments 23, 25.