1263171 - Identity box text for internal pages is unreadable on dark themes in Nightly/Aurora

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P4)

Description

[Mike de Boer [:mikedeboer]]

Attached you'll find a screenshot of the Vertex Dark theme - there are many more - which shows that the default, non-verified-state font color of the Identity Box is too dark to be discernible from the background.

In bug 1244500 I proposed the following fix:

```css
#nav-bar:not(:-moz-lwtheme)[brighttext] #identity-box:not(:hover):not([open=true]) {
  --identity-box-chrome-color: #fff;
}
```

But Gijs mentioned that things are probably not that straightforward: https://bugzilla.mozilla.org/show_bug.cgi?id=1244500#c24. And I think he's right :)

Comment 1

[Daniel Veditz [:dveditz]]

Forgot the attachment?

Comment 2

[Mike de Boer [:mikedeboer]]

(In reply to Daniel Veditz [:dveditz] from comment #1)
> Forgot the attachment?

Not really, the code in comment 0 is incomplete. There's more work to be done here.

Comment 3

[Dão Gottwald [:dao]]

(In reply to Mike de Boer [:mikedeboer] from comment #2)
> (In reply to Daniel Veditz [:dveditz] from comment #1)
> > Forgot the attachment?
> 
> Not really, the code in comment 0 is incomplete. There's more work to be
> done here.

You wrote "Attached you'll find a screenshot of the Vertex Dark theme" in comment 0. There's no screenshot.

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Screenshot demonstrating the issue

Comment 5

[Dão Gottwald [:dao]]

So this only affects the "Nightly" label, not the "Firefox" label nor EV cert labels. Right?

Comment 6

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #5)
> So this only affects the "Nightly" label, not the "Firefox" label nor EV
> cert labels. Right?

Very true.

Comment 7

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #5)
> > So this only affects the "Nightly" label, not the "Firefox" label nor EV
> > cert labels. Right?
> 
> Very true.

In this particular case, yes. It'd depend on the shade used whether other labels are affected. It seems likely that a more vibrant green for EV certs would be useful on some darker themes.

Comment 8

[Dão Gottwald [:dao]]

No, we're using a white background for EV cert labels.

Comment 9

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #8)
> No, we're using a white background for EV cert labels.

Is there a reason not to do the same for the "chromeui" identity box state as well?

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Mike de Boer [:mikedeboer] from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #5)
> > So this only affects the "Nightly" label, not the "Firefox" label nor EV
> > cert labels. Right?
> 
> Very true.

I take it this means that this issue doesn't affect release. Is that correct?

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking 46 and 47 as unaffected as this should only be affecting nightly.

Comment 12

[Mike de Boer [:mikedeboer]]

This does affect release, unfortunately - but only on dark GTK themes.

Comment 13

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #12)
> This does affect release, unfortunately - but only on dark GTK themes.

How/why? They never show "Nightly", right, just "Firefox", per comment #5 / comment #6 ?

Comment 14

[Mike de Boer [:mikedeboer]]

You are right. Sorry for spamming this bug with my confusion.

Comment 15

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #9)
> (In reply to Dão Gottwald [:dao] from comment #8)
> > No, we're using a white background for EV cert labels.
> 
> Is there a reason not to do the same for the "chromeui" identity box state
> as well?

Mainly that it's unnecessary for the Firefox label, because the orange should provide good contrast with native -moz-field backgrounds.

It would be nice if we could use the same color across channels to avoid these kind of channel-specific bugs.

Comment 16

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #15)
> It would be nice if we could use the same color across channels to avoid
> these kind of channel-specific bugs.

+1! Stephen, what do you think about unifying the text color inside the identity box for in-chrome UI across all channels?

Comment 17

[Mike de Boer [:mikedeboer]]

I just noticed that on Firefox 45 the text inside the identity box is blue as well! So this affects Fx 44+.

Comment 18

[Dão Gottwald [:dao]]

Oh, I wonder if this has changed or I just completely misremember us having used orange there.

Anyway, we should probably use a white background then, like we do for EV.

Comment 19

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #17)
> I just noticed that on Firefox 45 the text inside the identity box is blue
> as well! So this affects Fx 44+.

I run beta as my regular browser. It uses orange for Firefox, as do all release versions I've ever seen. Under what circumstances do you see blue?

Comment 20

[Mike de Boer [:mikedeboer]]

On Fx 45.0.1, Mozilla Firefox for Ubuntu, canonical - 1.0, visitin about:home.

Comment 21

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #20)
> On Fx 45.0.1, Mozilla Firefox for Ubuntu, canonical - 1.0, visitin
> about:home.

Ah, I can reproduce this. However, this is because they seem to not be building with MOZ_OFFICIAL_BRANDING:

https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/identity-block/identity-block.inc.css#11-18

Using a regular release build from mozilla.org the text *is* orange.

Pinging Mike and Sylvestre in case they know why not, what with all the recent todo about branding on Linux distros.


In any case, I agree that we should probably just make the background be white if that's what we do for EV already. Should probably doublecheck we don't break the devedition dark theme.

Comment 22

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #21)
> In any case, I agree that we should probably just make the background be
> white if that's what we do for EV already. Should probably doublecheck we
> don't break the devedition dark theme.

From a technical POV I still prefer using the same color across the board, which apparently would be orange after all. And if everyone agrees that the contrast with orange is good enough, we won't need the white background.

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Chris, does it ring a bell? (comment #21)

Comment 24

[Mike Hommey [:glandium]]

I don't know why ubuntu is not setting MOZ_OFFICIAL_BRANDING, but why is it not simply set in the branding? If we have color differences due to branding, it seems to me those differences should be coded in the branding itself, not in some file depending on #ifdef that may or may not match the branding being used.

Comment 25

[:Gijs (he/him)]

(In reply to Mike Hommey [:glandium] from comment #24)
> I don't know why ubuntu is not setting MOZ_OFFICIAL_BRANDING, but why is it
> not simply set in the branding? If we have color differences due to
> branding, it seems to me those differences should be coded in the branding
> itself,

I don't think we have branding files that are appropriate for this. The only files I know of are DTD and .properties files, and we're not talking about locale strings. The color is used from CSS, so you end up needing preprocessing either way... We don't have a way to use DTDs or .properties files from .css files. Do I just misunderstand what you're proposing?

Comment 26

[Mike Hommey [:glandium]]

You can add css files to browser/branding/*/content. There is one already for the about dialog, I don't see why there couldn't be more.

Comment 27

[Dão Gottwald [:dao]]

Let's not lose track. To recap, this bug goes away if we use the same color (orange) for the "chromeUI" identity box label regardless of branding. The label itself spells out the product name unambiguously and the color change for unofficial builds is just a gimmick with little practical value.

Ubuntu makes this bug worse by not setting MOZ_OFFICIAL_BRANDING, but that's not what this bug is about. The discussion on how to work around that is pretty much off-topic.

Comment 28

[Chris Coulson]

(In reply to Sylvestre Ledru [:sylvestre] from comment #23)
> Chris, does it ring a bell? (comment #21)

We build with --enable-official-branding. Is that not enough?

Comment 29

[Chris Coulson]

Hmmm, ok, I thought we did but apparently that's no longer the case

Comment 30

[Chris Coulson]

This is resolved in our 46.0 builds

Comment 31

[Jim Mathies [:jimm]]

dropping tracking.

Comment 34

[:Gijs (he/him)]

Attachment: Bug 1263171 - remove nightly/aurora-specific branding colors,

Review commit: https://reviewboard.mozilla.org/r/60028/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60028/

Comment 35

[Karl Tomlinson (:karlt)]

Not gtk3, as reported in bug 994754.

Comment 36

[:Gijs (he/him)]

Comment on attachment 8763905 [details]
Bug 1263171 - remove nightly/aurora-specific branding colors,

20:51	shorlander	Gijs_away: It's pretty bad in relation to the not orange icon ;)
20:51	shorlander	Gijs_away: I would prefer just using system colors over the orange
20:54	shorlander	Gijs_away: cl.ly/3Q0a0w1I0q31
20:54	shorlander	Same color
20:54	shorlander	On both sides

Dão, would something like this work as far as you're concerned?

Comment 37

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #36)
> 20:51	shorlander	Gijs_away: I would prefer just using system colors over the
> orange

What exactly does this mean?

Comment 38

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #37)
> (In reply to :Gijs Kruitbosch from comment #36)
> > 20:51	shorlander	Gijs_away: I would prefer just using system colors over the
> > orange
> 
> What exactly does this mean?

Well, we'll have to ask Stephen, but I assume using whatever the matching foreground system color is for the background color we currently use on the identity box (looks like that'd be -moz-fieldtext on linux, so likely something white/off-white if -moz-field produces a dark box).

Comment 39

[Stephen Horlander [:shorlander]]

(In reply to :Gijs Kruitbosch from comment #38)
> (In reply to Dão Gottwald [:dao] from comment #37)
> > (In reply to :Gijs Kruitbosch from comment #36)
> > > 20:51	shorlander	Gijs_away: I would prefer just using system colors over the
> > > orange
> > 
> > What exactly does this mean?
> 
> Well, we'll have to ask Stephen, but I assume using whatever the matching
> foreground system color is for the background color we currently use on the
> identity box (looks like that'd be -moz-fieldtext on linux, so likely
> something white/off-white if -moz-field produces a dark box).

Yes, if we are worried about legibility then I would prefer using the system color rather than sticking the orange text next to the blue icon. Which as Gijs says would probably be -moz-fieldtext?

Comment 40

[:Gijs (he/him)]

(In reply to Stephen Horlander [:shorlander] from comment #39)
> (In reply to :Gijs Kruitbosch from comment #38)
> > (In reply to Dão Gottwald [:dao] from comment #37)
> > > (In reply to :Gijs Kruitbosch from comment #36)
> > > > 20:51	shorlander	Gijs_away: I would prefer just using system colors over the
> > > > orange
> > > 
> > > What exactly does this mean?
> > 
> > Well, we'll have to ask Stephen, but I assume using whatever the matching
> > foreground system color is for the background color we currently use on the
> > identity box (looks like that'd be -moz-fieldtext on linux, so likely
> > something white/off-white if -moz-field produces a dark box).
> 
> Yes, if we are worried about legibility then I would prefer using the system
> color rather than sticking the orange text next to the blue icon. Which as
> Gijs says would probably be -moz-fieldtext?

WFM. Dão?

Comment 41

[Dão Gottwald [:dao]]

Sure.

Comment 42

[Dão Gottwald [:dao]]

Gijs, can you update the patch?

Comment 43

[:Gijs (he/him)]

Attachment: Bug 1263171 - use -moz-fieldtext for identity box text for chrome pages on unofficial/nightly/aurora builds,

Review commit: https://reviewboard.mozilla.org/r/75362/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75362/

Comment 44

[Dão Gottwald [:dao]]

Comment on attachment 8786418 [details]
Bug 1263171 - use -moz-fieldtext for identity box text for chrome pages on unofficial/nightly/aurora builds,

--identity-box-chrome-color is already a text color, introducing --identity-box-chrome-text-color next to that is just confusing.

I think we can completely get rid of --identity-box-chrome-color and wrap the rule setting that color in an ifdef.

Comment 45

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #44)
> Comment on attachment 8786418 [details]
> Bug 1263171 - use -moz-fieldtext for identity box text for chrome pages on
> unofficial/nightly/aurora builds,
> 
> --identity-box-chrome-color is already a text color, introducing
> --identity-box-chrome-text-color next to that is just confusing.
> 
> I think we can completely get rid of --identity-box-chrome-color and wrap
> the rule setting that color in an ifdef.

I don't understand what you're asking for here. Do you want a somewhat similar ifdef inside the new rule I'm adding rather than adding a separate property, and then move the existing property into an ifdef inside the one rule where we set color on the icon as well? That would require then also updating devedition, which seems to have code using the (existing) property. Or are you asking for something else? In which case, maybe it would be easier if you just write the patch? It would avoid playing 20 questions.

Comment 46

[Dão Gottwald [:dao]]

Attachment: patch

Yes, it does mean updating devedition. (I don't think there was any other way to read my comment...)

Comment 47

[:Gijs (he/him)]

Comment on attachment 8786522 [details] [diff] [review]
patch

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

> I don't think there was any other way to read my comment...

I wouldn't have written this patch based on your comment, not even without the additional change to .verifiedIdentity.

::: browser/themes/shared/identity-block/identity-block.inc.css
@@ +19,3 @@
>  }
>  
> +%ifdef MOZ_OFFICIAL_BRANDING

This means the specificity for the icon + label color changes depending on whether we're building with/without official branding. That sets us up for regressions that happen on official branding but not otherwise (or vice versa). I would prefer for the ifdef to be inside the rule, and keep the explicit -moz-fieldtext, even if that's already what applies now because of some other rule.

@@ +22,1 @@
>  #urlbar[pageproxystate="valid"] > #identity-box.chromeUI > #connection-icon,

So, I was concerned this would change the color of the firefox/nightly icon. It doesn't, because that's #identity-icon.

Under what circumstances does this icon show up when the identity-box has the chromeUI class? Because I believe the answer to that is "never", which means we might as well nuke this selector from orbit to reduce confusion and useless CSS. 

If the answer isn't "never", can you clarify what effect this has in the official branding case?

Comment 48

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #47)
> This means the specificity for the icon + label color changes depending on
> whether we're building with/without official branding. That sets us up for
> regressions that happen on official branding but not otherwise (or vice
> versa). I would prefer for the ifdef to be inside the rule, and keep the
> explicit -moz-fieldtext, even if that's already what applies now because of
> some other rule.

I don't understand how this will prevent official branding regressions. If the rule breaks we still won't notice on nightly/aurora because it's effectively a no-op there.

> >  #urlbar[pageproxystate="valid"] > #identity-box.chromeUI > #connection-icon,
> 
> So, I was concerned this would change the color of the firefox/nightly icon.
> It doesn't, because that's #identity-icon.
> 
> Under what circumstances does this icon show up when the identity-box has
> the chromeUI class? Because I believe the answer to that is "never", which
> means we might as well nuke this selector from orbit to reduce confusion and
> useless CSS. 
> 
> If the answer isn't "never", can you clarify what effect this has in the
> official branding case?

Seems orthogonal to this bug since I didn't touch the selector, but I think the #connection-icon selector can indeed be removed.

Comment 49

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #48)
> (In reply to :Gijs Kruitbosch from comment #47)
> > This means the specificity for the icon + label color changes depending on
> > whether we're building with/without official branding. That sets us up for
> > regressions that happen on official branding but not otherwise (or vice
> > versa). I would prefer for the ifdef to be inside the rule, and keep the
> > explicit -moz-fieldtext, even if that's already what applies now because of
> > some other rule.
> 
> I don't understand how this will prevent official branding regressions. If
> the rule breaks we still won't notice on nightly/aurora because it's
> effectively a no-op there.

Because if I want to override this rule I need something more specific than if I want to override the existing nightly/aurora rule. So let's say in some cases we want to color the text red:

#identity-box.reallyred > #identity-icon-labels {
  color: red;
}

would be enough after your patch for nightly and aurora, but not enough on official branding builds. So I think having the rule be present (but idempotent) on aurora/nightly is better than not having it.

> > >  #urlbar[pageproxystate="valid"] > #identity-box.chromeUI > #connection-icon,
> > 
> > So, I was concerned this would change the color of the firefox/nightly icon.
> > It doesn't, because that's #identity-icon.
> > 
> > Under what circumstances does this icon show up when the identity-box has
> > the chromeUI class? Because I believe the answer to that is "never", which
> > means we might as well nuke this selector from orbit to reduce confusion and
> > useless CSS. 
> > 
> > If the answer isn't "never", can you clarify what effect this has in the
> > official branding case?
> 
> Seems orthogonal to this bug since I didn't touch the selector, but I think
> the #connection-icon selector can indeed be removed.

Might as well do it while we're here anyway, IMO.

Comment 50

[Dão Gottwald [:dao]]

Attachment: patch v2

I also removed #urlbar[pageproxystate="valid"] > #identity-box.verifiedIdentity > #connection-icon since we don't use a filter on that icon anyway, i.e. the green is already part of the icon itself.

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/58a5ab186197
Remove custom identity box color for chrome pages in non-release builds since it doesn't provide sufficient contrast with dark themes. r=gijs

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/58a5ab186197

Comment 53

[Petruta Horea [:phorea], Desktop Test Engineering]

Reproduced with Vertex-Dark theme using Nightly 2016-04-08 under Ubuntu 14.04 32-bit.
Verified as fixed using 51.0a1 2016-09-02.