1146282 - [Control Center] New styling for host paragraph at the top of the identity panel

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Tim Taubert [:ttaubert] (inactive)]

Bug 1125230 includes a wireframe for v1.0 of the control center. The host section at the top showing the hostname and security information needs a new styling.

To not lose information here we will need a subpanel and will have to wait for UX to give us more directions on that. The color for EV certs (currently bright green) is something that security team surely has an opinion about too.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

This is only about the paragraph itself.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: screenshots-v1.zip

Hey Aislinn,

please see attached the different security states.

dv.png: For DV certs
ev.png: For EV certs
chromeui.png: For internal pages that are listed as trusted pages.
internal.png: For internal pages that don't enjoy a trusted status (for whatever reason)
http.png: For HTTP pages without TLS/SSL.
mixed.png: For pages showing a mixed content warning.
prefs.png: The DV case again simpy showing a few preferences.

Comment 3

[Ash Grigas]

(In reply to Tim Taubert [:ttaubert] from comment #2)
> Created attachment 8614716 [details]
> screenshots-v1.zip
> 
> Hey Aislinn,
> 
> please see attached the different security states.
> 
> dv.png: For DV certs
> ev.png: For EV certs
> chromeui.png: For internal pages that are listed as trusted pages.
> internal.png: For internal pages that don't enjoy a trusted status (for
> whatever reason)
> http.png: For HTTP pages without TLS/SSL.
> mixed.png: For pages showing a mixed content warning.
> prefs.png: The DV case again simpy showing a few preferences.

Looks good.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Corrected connection state colors and the vertical alignment of the prefs section as discussed on IRC.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1146282-New-styling-for-the-host-section-of-the-.patch

* Removes the now unused small icon below Larry.
* Shares as many styles as possible between OS themes.
* Removes some complexity from the previous panel structure (e.g. reusing the "supplemental" section instead of having a dedicated "encryption label")

Comment 6

[:Gijs (he/him)]

Comment on attachment 8615868 [details] [diff] [review]
0001-Bug-1146282-New-styling-for-the-host-section-of-the-.patch

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

::: browser/base/content/browser.js
@@ +6943,5 @@
> +      try {
> +        host = this.getEffectiveHost();
> +      } catch (e) {
> +        // Some URIs might have no hosts.
> +        host = this._lastUri.specIgnoringRef;

This shows almost the entire URI for a data URI, for instance. I don't think that's a good idea.

::: browser/themes/shared/controlcenter/panel.inc.css
@@ +12,5 @@
> +  padding: 0;
> +}
> +
> +#identity-popup-container {
> +  min-width: 280px;

I realize you're just copying this, but I expect this should really be measured in EM, and potentially it might need to be localizable (does "Weitere Informationen" fit in the box right now?).

I'm also a little sad we need it. I wish our popup sizing code was sane.

@@ +25,5 @@
>  
> +@media (min-resolution: 1.1dppx) {
> +  #identity-popup.chromeUI > #identity-popup-container > #identity-popup-icon {
> +    list-style-image: url("chrome://branding/content/icon128.png");
> +    -moz-image-region: rect(0, 128px, 128px, 0);

Why is this necessary in the 2x case but not in the regular one? Seems unnecessary.

@@ +41,5 @@
> +
> +.identity-popup-description {
> +  white-space: pre-wrap;
> +  -moz-padding-start: 15px;
> +  margin: 2px 0 4px;

This used to be 4px 0 2px; Intentional?

@@ +58,5 @@
> +  margin-bottom: 0;
> +  font-weight: 700;
> +}
> +
> +#identity-popup-content-host ,

nit: no space before ,

@@ +74,5 @@
> +  color: #59b32d;
> +}
> +
> +#identity-popup-connection-not-secure {
> +  color: #f2a100;

Neither of these colors has enough contrast (per WCAG specs, cf. http://webaim.org/resources/contrastchecker/ ).

Is it really necessary to use color in this way? If so, the following darkened versions work:

green: #418220
orange: becomes brown by the time that it's fully legible against the semi-transparent white background. :-(

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gijs Kruitbosch from comment #6)
> > +#identity-popup-connection-not-secure {
> > +  color: #f2a100;
> 
> Neither of these colors has enough contrast (per WCAG specs, cf.
> http://webaim.org/resources/contrastchecker/ ).
> 
> Is it really necessary to use color in this way? If so, the following
> darkened versions work:
> 
> green: #418220
> orange: becomes brown by the time that it's fully legible against the
> semi-transparent white background. :-(

Aislinn, Stephen, what can we do about this?

Comment 8

[Ash Grigas]

(In reply to Tim Taubert [:ttaubert] from comment #7)
> (In reply to :Gijs Kruitbosch from comment #6)
> > > +#identity-popup-connection-not-secure {
> > > +  color: #f2a100;
> > 
> > Neither of these colors has enough contrast (per WCAG specs, cf.
> > http://webaim.org/resources/contrastchecker/ ).
> > 
> > Is it really necessary to use color in this way? If so, the following
> > darkened versions work:
> > 
> > green: #418220
> > orange: becomes brown by the time that it's fully legible against the
> > semi-transparent white background. :-(
> 
> Aislinn, Stephen, what can we do about this?

Two ideas: change the orange to red for more contrast OR color the icon only not the text and the icon in the drop down.

Stephen - thoughts?

Comment 9

[Ash Grigas]

(In reply to agrigas from comment #8)
> (In reply to Tim Taubert [:ttaubert] from comment #7)
> > (In reply to :Gijs Kruitbosch from comment #6)
> > > > +#identity-popup-connection-not-secure {
> > > > +  color: #f2a100;
> > > 
> > > Neither of these colors has enough contrast (per WCAG specs, cf.
> > > http://webaim.org/resources/contrastchecker/ ).
> > > 
> > > Is it really necessary to use color in this way? If so, the following
> > > darkened versions work:
> > > 
> > > green: #418220
> > > orange: becomes brown by the time that it's fully legible against the
> > > semi-transparent white background. :-(
> > 
> > Aislinn, Stephen, what can we do about this?
> 
> Two ideas: change the orange to red for more contrast OR color the icon only
> not the text and the icon in the drop down.
> 
> Stephen - thoughts?

I'm going to make a call here and say we use red for the text color to provide more contrast. I suggest using our mozilla red which is hex value: D74345

Comment 10

[Stephen Horlander [:shorlander]]

(In reply to agrigas from comment #9)
> (In reply to agrigas from comment #8)
> > (In reply to Tim Taubert [:ttaubert] from comment #7)
> > > (In reply to :Gijs Kruitbosch from comment #6)
> > > > > +#identity-popup-connection-not-secure {
> > > > > +  color: #f2a100;
> > > > 
> > > > Neither of these colors has enough contrast (per WCAG specs, cf.
> > > > http://webaim.org/resources/contrastchecker/ ).
> > > > 
> > > > Is it really necessary to use color in this way? If so, the following
> > > > darkened versions work:
> > > > 
> > > > green: #418220
> > > > orange: becomes brown by the time that it's fully legible against the
> > > > semi-transparent white background. :-(
> > > 
> > > Aislinn, Stephen, what can we do about this?
> > 
> > Two ideas: change the orange to red for more contrast OR color the icon only
> > not the text and the icon in the drop down.
> > 
> > Stephen - thoughts?
> 
> I'm going to make a call here and say we use red for the text color to
> provide more contrast. I suggest using our mozilla red which is hex value:
> D74345

Red works. Although I worry about this being scarier than it needs to be.

On the specific topic of contrast: There are a bunch of places in our UI that would not pass that contrast checker. That has not traditionally been a blocker on styling. Is there a reason that it is in this case?

Comment 11

[:Gijs (he/him)]

(In reply to Stephen Horlander [:shorlander] from comment #10)
> On the specific topic of contrast: There are a bunch of places in our UI
> that would not pass that contrast checker.

I am curious what these would be. a 3:1 contrast I expect we manage pretty much everywhere (though probably not 4.5:1). The green already in the location bar (for EV certs) does manage this, for instance - the green here is lighter, and the background is transparent, which means contrast is significantly worse.

> That has not traditionally been a
> blocker on styling. Is there a reason that it is in this case?

I mean, the reason I checked is that I myself found the text not super legible. I have decent eyes and can read it (besides having already read the patch, so I *knew* what the text said), but the semi-transparency of the panel and the already-slight contrast don't make for a good result. I also *personally* don't think colour adds much here.

FWIW, for things like sec508 compliance, this is one of the trivial things that people fall over. I was under the impression that we were still looking to be in compliance there, but I could be wrong.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

FTR, Steven said on IRC that we should go with the red proposed by Aislinn and the darker green proposed by Gijs for now and see how that works out.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gijs Kruitbosch from comment #6)
> > +@media (min-resolution: 1.1dppx) {
> > +  #identity-popup.chromeUI > #identity-popup-container > #identity-popup-icon {
> > +    list-style-image: url("chrome://branding/content/icon128.png");
> > +    -moz-image-region: rect(0, 128px, 128px, 0);
> 
> Why is this necessary in the 2x case but not in the regular one? Seems
> unnecessary.

Just copied this over but good point, we don't need this.

> > +.identity-popup-description {
> > +  white-space: pre-wrap;
> > +  -moz-padding-start: 15px;
> > +  margin: 2px 0 4px;
> 
> This used to be 4px 0 2px; Intentional?

"4px 0 2px" was only used for #identity-popup-content-verifier. It was "2px 0 4px" for .identity-popup-description before too.

> @@ +74,5 @@
> > +  color: #59b32d;
> > +}
> > +
> > +#identity-popup-connection-not-secure {
> > +  color: #f2a100;
> 
> Neither of these colors has enough contrast (per WCAG specs, cf.
> http://webaim.org/resources/contrastchecker/ ).

Colors fixed locally to the dark green and red.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gijs Kruitbosch from comment #6)
> > +#identity-popup-container {
> > +  min-width: 280px;
> 
> I realize you're just copying this, but I expect this should really be
> measured in EM, and potentially it might need to be localizable (does
> "Weitere Informationen" fit in the box right now?).

Yeah, there's enough space for the German version :)

Changed to |min-width: 25em;| which is roughly the same width (276px).

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gijs Kruitbosch from comment #6)
> > +      try {
> > +        host = this.getEffectiveHost();
> > +      } catch (e) {
> > +        // Some URIs might have no hosts.
> > +        host = this._lastUri.specIgnoringRef;
> 
> This shows almost the entire URI for a data URI, for instance. I don't think
> that's a good idea.

Very good point, thanks for spotting that. I'll turn the host into a <label crop=end> so that it's always cropped at the end. Talked to Aislinn about that a few days ago and we decided to always crop it as the full domain name and the full organization name (for EV certs) is shown in the URL bar already and there's no harm shortening those strings in a secondary UI.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Interdiff-to-address-review-feedback.patch

Interdiff based on the first patch. Addresses all review feedback.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8615868 [details] [diff] [review]
0001-Bug-1146282-New-styling-for-the-host-section-of-the-.patch

I'll fold the two patches of course once they're ready.

Comment 18

[:Gijs (he/him)]

Comment on attachment 8617856 [details] [diff] [review]
0002-Interdiff-to-address-review-feedback.patch

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

::: browser/base/content/browser.js
@@ +6960,5 @@
>        break;
>      }
>  
>      // Push the appropriate strings out to the UI
> +    this._identityPopupContentHost.value = host;

Nit: comment above this to say that because we crop, we need to use value (textContent would wrap) ?

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gijs Kruitbosch from comment #18)
> >      // Push the appropriate strings out to the UI
> > +    this._identityPopupContentHost.value = host;
> 
> Nit: comment above this to say that because we crop, we need to use value
> (textContent would wrap) ?

Will do, thanks!

Comment 20

[Pulsebot]

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

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/0d6ee6e7ff2a

Comment 22

[Tanvi Vyas[:tanvi]]

Tim, did you miss this state in your screenshots?
https://people.mozilla.org/~tvyas/FigureG.jpg

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tanvi Vyas [:tanvi] from comment #22)
> Tim, did you miss this state in your screenshots?
> https://people.mozilla.org/~tvyas/FigureG.jpg

There is a screenshot for mixed content, not exactly sure what you're asking though? We didn't address any URL bar styling, only the control center.

Comment 24

[Catalin Varga [QA][:VarCat]]

I verified this bug using the following environment:

FF 41
Build Id:  20150716004006
OS: Win 7 x64, Ubuntu 14.04 x64, Mac Os X 10.9.5

Durint the testing I've noticed that on the attached zip the text color from chromeui.png is green but in reality is black, is this expected behavior?

Comment 25

[Catalin Varga [QA][:VarCat]]

The page I've used for testing is https://www.bennish.net/mixed-content.html

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Catalin Varga [QA][:VarCat] from comment #24)
> Durint the testing I've noticed that on the attached zip the text color from
> chromeui.png is green but in reality is black, is this expected behavior?

I can't remember how we ended up here but I think it makes sense given that we only want to tell people that this page is secure because it's internal. I don't think we should put too much emphasis on this part here given that we don't have a real "connection state".

But I'll defer to Ash on this, she might have a different opinion on this :)

(We're talking about internal pages like about:home, and about:license.)

Comment 27

[Ash Grigas]

(In reply to Tim Taubert [:ttaubert] from comment #26)
> (In reply to Catalin Varga [QA][:VarCat] from comment #24)
> > Durint the testing I've noticed that on the attached zip the text color from
> > chromeui.png is green but in reality is black, is this expected behavior?
> 
> I can't remember how we ended up here but I think it makes sense given that
> we only want to tell people that this page is secure because it's internal.
> I don't think we should put too much emphasis on this part here given that
> we don't have a real "connection state".
> 
> But I'll defer to Ash on this, she might have a different opinion on this :)
> 
> (We're talking about internal pages like about:home, and about:license.)

I'm not sure exactly what you're referring to without a screenshot but guessing its the text color for the internal hosted pages? Tim is correct we aren't changing the text color to green/secure but leaving it black since these aren't exactly verified secure connection states like the other UI that shows green text.

Comment 28

[Catalin Varga [QA][:VarCat]]

I was referring to the internal hosted pages, thanks for your feedback, marking this bug as verified.