Closed Bug 1163954 Opened 6 years ago Closed 6 years ago

[Gtk3] [Dark theme] text in preferences (about:config) is unreadable

Categories

(Firefox :: Preferences, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: stransky, Assigned: stransky)

References

Details

Attachments

(6 files, 3 obsolete files)

Attached image how it looks like
Fedora/Gtk3/Dark theme

Dark theme in Gtk3 means that system uses white text on black background. On the preferences pannel, only background color is picked up. Text color remains dark. So we get dark color on dark background.
Summary: [Gtk3] [Dark theme] text in preferences is unreadable → [Gtk3] [Dark theme] text in preferences (about:config) is unreadable
I just realized it affects all xul list boxes (for instance about:preferences#search).
Attached patch simple fix (obsolete) — Splinter Review
Comment on attachment 8609403 [details] [diff] [review]
simple fix

Jared, what do you think about this change, is that a correct way or should it be fixed in a different manner? Thanks!
Attachment #8609403 - Flags: feedback?(jaws)
Assignee: nobody → stransky
Status: NEW → ASSIGNED
Comment on attachment 8609403 [details] [diff] [review]
simple fix

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

I'm not so sure about this change, I'm redirecting the request to someone that might have more opinions on it. The one part for me is hardcoding background-color to white and I don't see where the text-color is defined to always have high contrast with a white background.
Attachment #8609403 - Flags: feedback?(jaws) → feedback?(gijskruitbosch+bugs)
Comment on attachment 8609403 [details] [diff] [review]
simple fix

All of these colors should be defined as variables at the top of the file, like all the other colors. In fact, for the background and foreground of listboxes, we already have variables, from what I can tell. You should be reusing these. 


I also don't understand why the existing rules for the (rich)listbox aren't enough here (they are right above the block your first hunk is changing). We should probably just ensure those are working on Linux instead of adding new rules/properties.

Also, the comment here is superfluous without more information. It's a bit like:

/* make variable be 5 */
var foo = 5;


It doesn't say why we're doing that, nor does it give context. It should do either/both of those, or not be there.
Attachment #8609403 - Flags: feedback?(gijskruitbosch+bugs) → feedback-
Attached patch patch (obsolete) — Splinter Review
Thanks for the feedback, there's the updated one. This bug affects Windows too so it's not Linux only (I don't have Mac). It happens when dark theme is selected, i.e. white text on dark background is chosen for windows.

There are three issues which are addressed in this patch:
- xul|listbox xul|listitem -> fixes wrong background in about:config, search engine list, favorite applications and so.

- xul|tree -> set background when no item is listed or the list does not fill whole listbox area. That's certificate exception listing for instance.

- xul|treechildren::-moz-tree-cell-text(hover) -> fixes background of active line in listbox.
Attachment #8609403 - Attachment is obsolete: true
Attachment #8616040 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8616040 [details] [diff] [review]
patch

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

::: toolkit/themes/shared/in-content/common.inc.css
@@ +704,5 @@
>  xul|tree {
>    -moz-appearance: none;
>    font-size: 1em;
>    border: 1px solid var(--in-content-box-border-color);
> +  background-color: var(--in-content-box-background);

Why is this necessary if it's already set on treechildren::-moz-tree-row ?
Attachment #8616040 - Flags: review?(gijskruitbosch+bugs)
Attached image back.png
> Why is this necessary if it's already set on treechildren::-moz-tree-row ?

When the tree is empty there's black background rendered. The same happens when the list is not filed completely - like on the attached image.
(In reply to Martin Stránský from comment #8)
> Created attachment 8616797 [details]
> back.png
> 
> > Why is this necessary if it's already set on treechildren::-moz-tree-row ?
> 
> When the tree is empty there's black background rendered. The same happens
> when the list is not filed completely - like on the attached image.

OK, but then, conversely, why do you need it on the treechildren if you set it on the tree anyway?
Flags: needinfo?(stransky)
Attached image back2.png
> OK, but then, conversely, why do you need it on the treechildren if you set
> it on the tree anyway?

As you can see some of the rows does have a background - which is dark.
Flags: needinfo?(stransky)
(In reply to Martin Stránský from comment #10)
> Created attachment 8616831 [details]
> back2.png
> 
> > OK, but then, conversely, why do you need it on the treechildren if you set
> > it on the tree anyway?
> 
> As you can see some of the rows does have a background - which is dark.

This is because of:

http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/tree.css#44
http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsLookAndFeel.cpp#397

and the various windows/osx equivalents of the same.

Which we should probably not *all* just override to a plain white. On OS X they are white (#fff) for odd rows and off-white (#f3f6fa) for even rows. We should make sure not to change that.
Attached patch patch v.2 (obsolete) — Splinter Review
Okay, thanks. So what about this one? It redefines the row background on Linux only.
Attachment #8616040 - Attachment is obsolete: true
Attachment #8617242 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8617242 [details] [diff] [review]
patch v.2

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

I tested this patch and it still breaks the even/odd colouring of the rows on my linux VM.

I expect that the right thing to do will be to hardcode both colors (so that's one selector with no atoms and one with (odd)) in the shared file, because all the other colours are hardcoded already, and otherwise we'll just run into the same or a similar bug on Windows (because the foreground colours are hardcoded but the background ones are OS-theme-based).
Attachment #8617242 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #13)
> I expect that the right thing to do will be to hardcode both colors (so
> that's one selector with no atoms and one with (odd)) in the shared file,
> because all the other colours are hardcoded already, and otherwise we'll
> just run into the same or a similar bug on Windows (because the foreground
> colours are hardcoded but the background ones are OS-theme-based).

Okay, and which color values we should use for that?
The MacOS ones or something different?
(In reply to Martin Stránský from comment #15)
> The MacOS ones or something different?

Let's ask UX. Philipp, we want to hardcode the even/odd colouring in trees (lists) in the in-content prefs. Can you please indicate what colours to use?
Flags: needinfo?(philipp)
Hm, we don't use any even/odd coloring on Windows (and we probably should keep it that way).

But I assume that this patch is for Linux only - we can use the same colors as on Mac there.
Flags: needinfo?(philipp)
Attached patch patch v.3Splinter Review
Thanks, there's the updated one.
Attachment #8617242 - Attachment is obsolete: true
Attachment #8617870 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8617870 [details] [diff] [review]
patch v.3

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

Almost there. Sorry for the numerous iterations here...

::: toolkit/themes/linux/global/in-content/common.css
@@ +95,5 @@
> +xul|treechildren::-moz-tree-row(multicol, odd) {
> +  background-color: var(--in-content-box-background-odd);
> +}
> +
> +xul|treechildren::-moz-tree-row(hover) {

This rule already exists in common.inc.css...

@@ +99,5 @@
> +xul|treechildren::-moz-tree-row(hover) {
> +  background-color: var(--in-content-item-hover);
> +}
> +
> +xul|treechildren::-moz-tree-row(selected) {

And so does this one.

::: toolkit/themes/shared/in-content/common.inc.css
@@ +763,5 @@
>  xul|treechildren::-moz-tree-cell-text {
>    color: var(--in-content-text-color);
>  }
>  
> +xul|treechildren::-moz-tree-cell-text(hover),

So, the background color for these rows and the listbox/tree is fixed to white/grey now, which means that black text is always legible, even on hover, isn't it? In fact, at least on my machine black-on-light-blue provides more contrast than white-on-light-blue, so I'd prefer not to do this.
Attachment #8617870 - Flags: review?(gijskruitbosch+bugs) → feedback+
(In reply to :Gijs Kruitbosch from comment #19)
> ::: toolkit/themes/linux/global/in-content/common.css
> @@ +95,5 @@
> > +xul|treechildren::-moz-tree-row(multicol, odd) {
> > +  background-color: var(--in-content-box-background-odd);
> > +}
> > +
> > +xul|treechildren::-moz-tree-row(hover) {
> 
> This rule already exists in common.inc.css...
> 
> @@ +99,5 @@
> > +xul|treechildren::-moz-tree-row(hover) {
> > +  background-color: var(--in-content-item-hover);
> > +}
> > +
> > +xul|treechildren::-moz-tree-row(selected) {
> 
> And so does this one.

Yes. because xul|treechildren::-moz-tree-row(multicol, odd) overides also hover/selected state. I tried to make up a css selector with -moz-tree-row + not(selected, hover) but without luck. xul|treechildren::-moz-tree-row(multicol, odd):not(hover):not(selected) does not work for me.

> ::: toolkit/themes/shared/in-content/common.inc.css
> @@ +763,5 @@
> >  xul|treechildren::-moz-tree-cell-text {
> >    color: var(--in-content-text-color);
> >  }
> >  
> > +xul|treechildren::-moz-tree-cell-text(hover),
> 
> So, the background color for these rows and the listbox/tree is fixed to
> white/grey now, which means that black text is always legible, even on
> hover, isn't it? 

Yes.

> In fact, at least on my machine black-on-light-blue
> provides more contrast than white-on-light-blue, so I'd prefer not to do
> this.

Okay, it makes sense.
Attached patch patch v.4Splinter Review
There's the patch with black text on hover.
Comment on attachment 8617883 [details] [diff] [review]
patch v.4

(In reply to Martin Stránský from comment #20)
> (In reply to :Gijs Kruitbosch from comment #19)
> > ::: toolkit/themes/linux/global/in-content/common.css
> > @@ +95,5 @@
> > > +xul|treechildren::-moz-tree-row(multicol, odd) {
> > > +  background-color: var(--in-content-box-background-odd);
> > > +}
> > > +
> > > +xul|treechildren::-moz-tree-row(hover) {
> > 
> > This rule already exists in common.inc.css...
> > 
> > @@ +99,5 @@
> > > +xul|treechildren::-moz-tree-row(hover) {
> > > +  background-color: var(--in-content-item-hover);
> > > +}
> > > +
> > > +xul|treechildren::-moz-tree-row(selected) {
> > 
> > And so does this one.
> 
> Yes. because xul|treechildren::-moz-tree-row(multicol, odd) overides also
> hover/selected state.

Ah.

> I tried to make up a css selector with -moz-tree-row +
> not(selected, hover) but without luck.
> xul|treechildren::-moz-tree-row(multicol, odd):not(hover):not(selected) does
> not work for me.

Ugh. Yeah, I am not surprised that doesn't work, a little more surprised that there seems to be no other way to do this (:not(::-moz-tree-row(hover)) also doesn't work, for instance).

OK, so then can you add a comment above these two rules explaining why we're copying them? Something like "These rules are duplicated from common.inc.css because otherwise the above rule for odd rows overrides them" ?

With that, r=me
Attachment #8617883 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9bcefcaf458c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Duplicate of this bug: 1183591
Duplicate of this bug: 1178603
Duplicate of this bug: 1174713
You need to log in before you can comment on or make changes to this bug.