Martin Stránský [:stransky] (ni? me) Assignee
	Description • 9 years ago

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.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 1 • 9 years ago

I just realized it affects all xul list boxes (for instance about:preferences#search).

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 3 • 9 years ago

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!

	Jared Wein [:jaws] (please needinfo? me)
	Comment 4 • 9 years ago

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.

	:Gijs (he/him)
	Comment 5 • 9 years ago

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.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 6 • 9 years ago

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.

	:Gijs (he/him)
	Comment 7 • 9 years ago

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 ?

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 8 • 9 years ago

> 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.

	:Gijs (he/him)
	Comment 9 • 9 years ago

(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?

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 10 • 9 years ago

> 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.

	:Gijs (he/him)
	Comment 11 • 9 years ago

(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.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 12 • 9 years ago

Okay, thanks. So what about this one? It redefines the row background on Linux only.

	:Gijs (he/him)
	Comment 13 • 9 years ago

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).

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 14 • 9 years ago

(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?

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 15 • 9 years ago

The MacOS ones or something different?

	:Gijs (he/him)
	Comment 16 • 9 years ago

(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?

	(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo
	Comment 17 • 9 years ago

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.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 18 • 9 years ago

Thanks, there's the updated one.

	:Gijs (he/him)
	Comment 19 • 9 years ago

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.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 20 • 9 years ago

(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.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 21 • 9 years ago

There's the patch with black text on hover.

	:Gijs (he/him)
	Comment 22 • 9 years ago

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

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 23 • 9 years ago

Thanks, there's the one for check-in.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=83aec9c78288

	Pulsebot
	Comment 24 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/9bcefcaf458c

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 25 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/9bcefcaf458c