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

VERIFIED FIXED in Firefox 51

Status

()

P4
normal
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: mikedeboer, Assigned: dao)

Tracking

(Blocks: 1 bug, {regression})

44 Branch
Firefox 51
All
Linux
regression
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox45 wontfix, firefox46 wontfix, firefox47 wontfix, firefox48 wontfix, firefox49 wontfix, firefox50 wontfix, firefox51 verified)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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 :)
Flags: qe-verify+
Flags: firefox-backlog+
Forgot the attachment?
Flags: needinfo?(mdeboer)
(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.
Flags: needinfo?(mdeboer)
(Assignee)

Comment 3

3 years ago
(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.
Flags: needinfo?(mdeboer)
Created attachment 8740903 [details]
Screenshot demonstrating the issue
Flags: needinfo?(mdeboer)
(Assignee)

Comment 5

3 years ago
So this only affects the "Nightly" label, not the "Firefox" label nor EV cert labels. Right?
(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

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

Comment 8

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

Comment 9

3 years ago
(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?
Flags: needinfo?(dao)
(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?
Marking 46 and 47 as unaffected as this should only be affecting nightly.
status-firefox46: affected → unaffected
status-firefox47: affected → unaffected
This does affect release, unfortunately - but only on dark GTK themes.
status-firefox46: unaffected → affected
status-firefox47: unaffected → affected

Comment 13

3 years ago
(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 ?
Flags: needinfo?(mdeboer)
You are right. Sorry for spamming this bug with my confusion.
status-firefox46: affected → unaffected
status-firefox47: affected → unaffected
Flags: needinfo?(mdeboer)
(Assignee)

Comment 15

3 years ago
(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.
Flags: needinfo?(dao)
(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?
Flags: needinfo?(shorlander)
I just noticed that on Firefox 45 the text inside the identity box is blue as well! So this affects Fx 44+.
status-firefox45: --- → wontfix
status-firefox46: unaffected → affected
status-firefox47: unaffected → affected
(Assignee)

Comment 18

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

3 years ago
(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?
Flags: needinfo?(mdeboer)
On Fx 45.0.1, Mozilla Firefox for Ubuntu, canonical - 1.0, visitin about:home.
Flags: needinfo?(mdeboer)

Comment 21

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

Comment 22

3 years ago
(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.
Chris, does it ring a bell? (comment #21)
Flags: needinfo?(sledru) → needinfo?(chrisccoulson)

Updated

3 years ago
status-firefox46: affected → wontfix
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.
Flags: needinfo?(mh+mozilla)

Comment 25

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

Comment 27

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

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

Comment 29

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

Comment 30

3 years ago
This is resolved in our 46.0 builds

Comment 31

2 years ago
dropping tracking.
Blocks: 627699
status-firefox47: affected → wontfix
status-firefox48: affected → wontfix
Depends on: 994754
Blocks: 1234805

Updated

2 years ago
No longer blocks: 1234805

Updated

2 years ago
Duplicate of this bug: 1234805

Updated

2 years ago
No longer depends on: 994754
Duplicate of this bug: 994754

Comment 34

2 years ago
Created attachment 8763905 [details]
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/
Attachment #8763905 - Flags: review?(shorlander)
Attachment #8763905 - Flags: review?(dao+bmo)
(Assignee)

Updated

2 years ago
Attachment #8763905 - Flags: review?(dao+bmo) → review+
Not gtk3, as reported in bug 994754.
No longer blocks: 627699
Summary: [GTK3] Identity box text unreadable on dark themes → Identity box text unreadable on dark themes
status-firefox49: --- → wontfix
status-firefox50: --- → affected
Version: Trunk → 44 Branch

Comment 36

2 years ago
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?
Flags: needinfo?(shorlander) → needinfo?(dao+bmo)
Attachment #8763905 - Flags: review?(shorlander)
(Assignee)

Comment 37

2 years ago
(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?
Flags: needinfo?(dao+bmo)

Comment 38

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

Comment 40

2 years ago
(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?
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 41

2 years ago
Sure.
Flags: needinfo?(dao+bmo)

Updated

2 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Blocks: 1022601
(Assignee)

Comment 42

2 years ago
Gijs, can you update the patch?
Blocks: 1016556
Flags: needinfo?(gijskruitbosch+bugs)
Priority: -- → P4
(Assignee)

Updated

2 years ago
Summary: Identity box text unreadable on dark themes → Identity box text for internal pages is unreadable on dark themes in Nightly/Aurora
Comment hidden (mozreview-request)

Updated

2 years ago
Attachment #8763905 - Attachment is obsolete: true

Updated

2 years ago
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 44

2 years ago
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.
Attachment #8786418 - Flags: review?(dao+bmo) → review-

Comment 45

2 years ago
(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.
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 46

2 years ago
Created attachment 8786522 [details] [diff] [review]
patch

Yes, it does mean updating devedition. (I don't think there was any other way to read my comment...)
Assignee: gijskruitbosch+bugs → dao+bmo
Attachment #8786418 - Attachment is obsolete: true
Flags: needinfo?(dao+bmo)
Attachment #8786522 - Flags: review?(gijskruitbosch+bugs)

Comment 47

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

Comment 48

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

Comment 49

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

Comment 50

2 years ago
Created attachment 8786678 [details] [diff] [review]
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.
Attachment #8786522 - Attachment is obsolete: true
Attachment #8786678 - Flags: review?(gijskruitbosch+bugs)

Updated

2 years ago
Attachment #8786678 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 51

2 years ago
Pushed by dgottwald@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/58a5ab186197
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
(Assignee)

Updated

2 years ago
status-firefox50: affected → wontfix
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.
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.