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

VERIFIED FIXED in Firefox 41

Status

()

Firefox
General
P1
normal
Rank:
1
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 41
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox41 verified)

Details

(Whiteboard: [fxprivacy], URL)

Attachments

(3 attachments)

(Assignee)

Description

3 years ago
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.
Flags: qe-verify+
Flags: firefox-backlog+
(Assignee)

Updated

3 years ago
Depends on: 1146284
(Assignee)

Updated

3 years ago
Depends on: 1146287
(Assignee)

Updated

3 years ago
Whiteboard: [fxprivacy]
(Assignee)

Updated

3 years ago
Blocks: 1168415
(Assignee)

Comment 1

3 years ago
This is only about the paragraph itself.
Summary: [Control Center] New styling for host section at the top of the identity panel → [Control Center] New styling for host paragraph at the top of the identity panel

Updated

3 years ago
Blocks: 1168461
(Assignee)

Updated

3 years ago
No longer blocks: 1168461
Depends on: 1168461
(Assignee)

Updated

3 years ago
Blocks: 1168461
No longer depends on: 1168461
(Assignee)

Updated

3 years ago
Points: 8 → 3

Updated

3 years ago
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 41.2 - Jun 8

Updated

3 years ago
Priority: -- → P1
Blocks: 1168885
(Assignee)

Comment 2

3 years ago
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.
Attachment #8614716 - Flags: ui-review?(agrigas)

Comment 3

3 years ago
(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.

Updated

3 years ago
Attachment #8614716 - Flags: ui-review?(agrigas)
(Assignee)

Comment 4

3 years ago
Corrected connection state colors and the vertical alignment of the prefs section as discussed on IRC.
(Assignee)

Comment 5

3 years ago
Created attachment 8615868 [details] [diff] [review]
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")
Attachment #8615868 - Flags: review?(gijskruitbosch+bugs)

Comment 6

3 years ago
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. :-(
Attachment #8615868 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 7

3 years ago
(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?
Flags: needinfo?(shorlander)
Flags: needinfo?(agrigas)

Comment 8

3 years ago
(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?
Flags: needinfo?(agrigas)

Updated

3 years ago
Rank: 1

Comment 9

3 years ago
(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

Updated

3 years ago
Flags: needinfo?(shorlander)

Updated

3 years ago
Iteration: 41.2 - Jun 8 → 41.3 - Jun 29
(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?
Flags: needinfo?(gijskruitbosch+bugs)

Comment 11

3 years ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 12

3 years ago
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.
(Assignee)

Comment 13

3 years ago
(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.
(Assignee)

Comment 14

3 years ago
(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).
(Assignee)

Comment 15

3 years ago
(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.
(Assignee)

Comment 16

3 years ago
Created attachment 8617856 [details] [diff] [review]
0002-Interdiff-to-address-review-feedback.patch

Interdiff based on the first patch. Addresses all review feedback.
Attachment #8617856 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 17

3 years ago
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.
Attachment #8615868 - Flags: review?(gijskruitbosch+bugs)

Comment 18

3 years ago
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) ?
Attachment #8617856 - Flags: review?(gijskruitbosch+bugs) → review+

Updated

3 years ago
Attachment #8615868 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 19

3 years ago
(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!
https://hg.mozilla.org/mozilla-central/rev/0d6ee6e7ff2a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
QA Contact: catalin.varga

Comment 22

3 years ago
Tim, did you miss this state in your screenshots?
https://people.mozilla.org/~tvyas/FigureG.jpg
(Assignee)

Comment 23

3 years ago
(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.
(Assignee)

Updated

3 years ago
Depends on: 1180859
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?
Flags: needinfo?(ttaubert)
The page I've used for testing is https://www.bennish.net/mixed-content.html
(Assignee)

Comment 26

3 years ago
(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.)
Flags: needinfo?(ttaubert) → needinfo?(agrigas)

Comment 27

3 years ago
(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.
Flags: needinfo?(agrigas)
I was referring to the internal hosted pages, thanks for your feedback, marking this bug as verified.
Status: RESOLVED → VERIFIED
status-firefox41: fixed → verified
You need to log in before you can comment on or make changes to this bug.