1244500 - [firefox-gtk+3] On FF 44 current tab + bookmarks toolbar and extensions too light on dark themes

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[je-vv]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20160126104524

Steps to reproduce:

All this is by just running FF 44 with gtk+3...

You can run it without anything under:

gtk-3.0/settings.ini

Then adding to it:

gtk-application-prefer-dark-theme = true

And then adding a dark theme, such as:

gtk-theme-name = GnomishDark

All that with no DE in place, just a plain WM (in my case fluxbox).  All this just to make the dark theme changes take place.


Actual results:

On the gist images provided:

https://gist.github.com/je-vv/7a00601e9217cd9d3447

The bottom image corresponds to no gtk+3 settings, meaning a default light gtk+3 theme.  The middle image corresponds to setting "gtk-application-prefer-dark-theme" to true on gtk+3.  And the upper image corresponds to the same setting as on the middle image, plus using the dark theme "GnomishDark".

As you can see, in all cases, FF 44 has a new behavior (with regards to prior versions), where FF is creating contrast between current tab (but includes as well bookmarks toolbar and extensions, icons and URL toolbar) and other tabs.

FF creates this contrast by leaving the current tab and toolbars lighter than the other tabs.

In the case of turning the preference for dark themes on (with or without a dark theme used), makes the current tab an all toolbars just way too light.  So there's little contrast between background and fonts.  It's just as if all that area was selected.  See the middle and upper images.

In the case of the light theme (no gtk+3 settings and no theme specified) that is OK, because the fonts are supposed to be dark while the background light.


Expected results:

One thought to avoid the misbehavior on dark themes, is to create the contrast, but the other way around.  Leaving the current tab and toolbars dark, while keeping the other tabs lighter.  This would work for both, light and dark themes, while creating the intended contrast between current and other themes.

Another thought is to keep everything the normal back ground colour (dark or light, depending on the theme), and just changing colours on the tabs bar.  That way other toolbars and icons wouldn't be affected.

Or, just picking better colours and degradations used by FF, :-)

Notice this is new on FF 44 for gtk+3.

Comment 1

[:Gijs (he/him)]

Does this happen with Firefox 45 as well? ( https://beta.mozilla.org/ )

Are you using a distro build of Firefox 44, or a Mozilla-provided build?

Comment 2

[je-vv]

I'm using Arch gnu/linux on x86-64, not a mozilla built, and I haven't tried FF 45 yet as it's in beta.

Comment 3

[je-vv]

I've tried also the 44 built/released by mozilla, and the 45.0b1 beta built/released by mozilla.  Both experience the same behavior.  So it's not a distro issue, and it's not fixed on beta either.

Please remember I'm not using DE, but plain fluxbox instead, and I'm configuring gtk+3 to prefer dark themes (gtk-application-prefer-dark-theme = true).  Ont top of that, I'm using a gtk+3 dark theme (gtk-theme-name = GnomishDark), though just by setting the preferred application theme to dark will show the behavior, as you can see on the gist already provided...

Thanks!

Comment 4

[:Gijs (he/him)]

(In reply to Javier from comment #3)
> I've tried also the 44 built/released by mozilla, and the 45.0b1 beta
> built/released by mozilla.  Both experience the same behavior.  So it's not
> a distro issue, and it's not fixed on beta either.
> 
> Please remember I'm not using DE, but plain fluxbox instead, and I'm
> configuring gtk+3 to prefer dark themes (gtk-application-prefer-dark-theme =
> true).  Ont top of that, I'm using a gtk+3 dark theme (gtk-theme-name =
> GnomishDark), though just by setting the preferred application theme to dark
> will show the behavior, as you can see on the gist already provided...

Yes, but 44 as Mozilla ships it is built on a gtk2 toolkit, and you keep mentioning gtk3, which is why I asked you to test 45, which is built on gtk3.

As it is, I don't know off-hand why the behaviour would be any different between 43 and 44. I looked at the theme bugs that got fixed for 44, 45 and 46 (for uplift to 44, potentially), and found nothing that seemed relevant. What did this look like in 43? Any chance you could narrow down when this broke using mozregression? ( https://mozilla.github.io/mozregression/ )

Comment 5

[je-vv]

Ah, well I'm using also a dark theme for gtk+2 (gtk-theme-name="GnomishDark"), and actually it's the same one I use for gtk+3 (gtk-theme-name = GnomishDark).  So perhaps that's why in the end the mozilla released experienced the same thing.

I'll try looking for mozregression...

Comment 6

[je-vv]

Any chance to know which nightlies correspond to versions 43.0.4 and 44.0?  Thanks,

Comment 7

[:Gijs (he/him)]

(In reply to Javier from comment #6)
> Any chance to know which nightlies correspond to versions 43.0.4 and 44.0? 
> Thanks,

On the most recent versions of mozregression I believe just --good 43 --bad 44 should work.

Comment 8

[je-vv]

Hmm, I think several versions prior to the one I marked as good, they already experience the contrast between current and other tabs.  But on those versions the current was not so light, therefore I left that as good.  In my opinion, the other way around is still better (working for both light and dark themes), current dark and others lighter.

Any ways, the results from mozregression:

12:57.29 LOG: MainThread Bisector INFO Last good revision: 2a9c0cfc260c69b2ffb608f3332b3a03e13da9bf
12:57.29 LOG: MainThread Bisector INFO First bad revision: de4fc486564bf401bac7173cbf309df55470bb63

Both corresponding to 44.0a1.  As I said, both already have the new behavior, just that on the 1st bad it was too light...  As you mentioned this is gtk+2, it's kind of weird, the effect is on both gtk+2 and gtk+3, perhaps more visible on gtk+3.  It might be that I'm still using a gtk+2 dark theme...

Comment 9

[:Gijs (he/him)]

(In reply to Javier from comment #8)
> Hmm, I think several versions prior to the one I marked as good, they
> already experience the contrast between current and other tabs.  But on
> those versions the current was not so light, therefore I left that as good. 
> In my opinion, the other way around is still better (working for both light
> and dark themes), current dark and others lighter.

Can you provide a screenshot of the "good" behaviour? Or even the behaviour of 43?

Comment 10

[je-vv]

I added at the very bottom of the same gist:

https://gist.github.com/je-vv/7a00601e9217cd9d3447

A snapshot of the good old 43.0.4 behavior.  It's called "ff__43_0_4__good.gif".

You can notice how on 43.0.4 the current tab, plus toolbars, are not very light, and what gets lighter is just the current tab, not the toolbars.  Which works well on both light and dark themes.

There's some gradient, that also makes the change from darker in toolbars to lighter on current tab, or from darker at bottom to lighter at top and from darker on right to lighter on left, which makes the change smooth.

If the gradient is to be removed, I would suggest darker on current tab and toolbars, and lighter on other tabs.  As I said, that should work well on both dark and light themes...

Thanks.

Comment 11

[:Gijs (he/him)]

Philipp, can you take a look and suggest what to do? Keep in mind we can't fix things on a per-gtk-theme basis, we can only shade the toolbars and/or tabs and/or selected tabs darker or lighter. (though I guess in principle we could try to use the toolbar colour inference attributes to do two different things for dark/light themes... maybe... though we don't do that on any other OS and I'd be worried it might regress things)

It seems to me that it might just make sense to revert the two blocking bugs to get back the 43 appearance here, as that worked for both light and dark themes, but I don't know if we have better options. I suspect that making the selected tab darker as Javier suggested will not look so nice for lighter themes, which I would further suggest are probably a majority.

Comment 12

[je-vv]

Anye news?  Perhaps the easiest way is just reverting...

Comment 13

[:Gijs (he/him)]

Stephen, as Philipp is out this week, any chance you can comment on what you think we ought to do here?

Comment 14

[Stephen Horlander [:shorlander]]

(In reply to :Gijs Kruitbosch (away 24-29/3, incl.) from comment #13)
> Stephen, as Philipp is out this week, any chance you can comment on what you
> think we ought to do here?

The simplest solution would be to use a more transparent overlay color, like rgba(255,255,255,.1) instead of rgba(255,255,255,.4).

Probably only on Linux and probably *not* in the case when using a Persona.

Comment 15

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
User-visible regression of Firefox's visual appearance.

Comment 16

[:Gijs (he/him)]

Clearing needinfo's because Mike's got this, and because comment #14 has an approach.

Comment 17

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=906e0d9d2ed5

Comment 18

[Mike de Boer [:mikedeboer]]

baseline for comment 17: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9dfd01079c6

Comment 19

[Mike de Boer [:mikedeboer]]

Perfherder comparison link: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f9dfd01079c6&newProject=try&newRevision=05ab14af425c&framework=1&showOnlyImportant=0

Comment 20

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: add support for dark GTK themes when applied to toolbars and the tab strip

I think I have enough data now for at least a discussion: this patch fully implements what's requested, but shows a 2.31% regression for 'ts_paint opt e10s' on Linux64. Not on non-e10s, but I think I can explain why:
The default theme for Ubuntu - Ambiance - yields `brighttext=true`, which is not the default when a new window opens and the toolbars are initially constructed.
This patch reacts to the 'brighttext' attribute being present, because that's the only way we currently have to know if we're on a dark background or not. My guess is that the layout flush does happen for ts_paint on e10s but too late for ts_paint on non-e10s - I think I noticed a slight flicker when starting Fx as the lighter gradient(s) were applied later.
I'd like for you, Gijs, to look at the patch on your Linux to see if you can see the same behavior. And I'd be very happy if you could provide some feedback on this patch!

Comment 21

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #20)
> Created attachment 8739406 [details] [diff] [review]
> Patch 1: add support for dark GTK themes when applied to toolbars and the
> tab strip
> 
> I think I have enough data now for at least a discussion: this patch fully
> implements what's requested, but shows a 2.31% regression for 'ts_paint opt
> e10s' on Linux64. Not on non-e10s, but I think I can explain why:
> The default theme for Ubuntu - Ambiance - yields `brighttext=true`, which is
> not the default when a new window opens and the toolbars are initially
> constructed.
> This patch reacts to the 'brighttext' attribute being present, because
> that's the only way we currently have to know if we're on a dark background
> or not. My guess is that the layout flush does happen for ts_paint on e10s
> but too late for ts_paint on non-e10s - I think I noticed a slight flicker
> when starting Fx as the lighter gradient(s) were applied later.
> I'd like for you, Gijs, to look at the patch on your Linux to see if you can
> see the same behavior. And I'd be very happy if you could provide some
> feedback on this patch!

I haven't tried the patch, but what I'm a little confused about is that the way I read comment #14 was that we could just always use a more restrained highlight on Linux. Is there a reason not do that?

Comment 22

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #21)
> I haven't tried the patch, but what I'm a little confused about is that the
> way I read comment #14 was that we could just always use a more restrained
> highlight on Linux. Is there a reason not do that?

I think because of LWT, which Stephen mentioned in comment 14 too.

Comment 23

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.1: add support for dark GTK themes when applied to toolbars and the tab strip

With Stephen's help: a patch that's simpler and better (which tend to go hand-in-hand).

Comment 24

[:Gijs (he/him)]

Comment on attachment 8739427 [details] [diff] [review]
Patch 1.1: add support for dark GTK themes when applied to toolbars and the tab strip

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

::: browser/themes/linux/browser.css
@@ +998,5 @@
>    outline-offset: -3px;
>  }
>  
> +#nav-bar:not(:-moz-lwtheme)[brighttext] #identity-box:not(:hover):not([open=true]) {
> +  --identity-box-chrome-color: #fff;

r=me without this hunk as per IRC discussion; please do file a followup bug for this. I'm just not sure how to fix it "properly", so to speak... but I think we should probably find a way of doing that in identity-block.inc.css in coordination with different background and foreground (nightly vs. aurora vs. firefox) colors. We might even want to tweak the green we use for EV domains.

Comment 25

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/d7976fa32978a786a43ba4fa720f061bb096abcf
Bug 1244500: add support for dark GTK themes when applied to toolbars and the tab strip. r=Gijs

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d7976fa32978

Comment 27

[Richard Marti (:Paenglab)]

Attachment: FX-dark-theme.png

Is this really intended to have almost no difference between tabbar and toolbar? And also the top border has not the same color as the tab border.

Comment 28

[Mike de Boer [:mikedeboer]]

(In reply to Richard Marti (:Paenglab) from comment #27)
> Is this really intended to have almost no difference between tabbar and
> toolbar? And also the top border has not the same color as the tab border.

Yes, it's supposed to look like this - and personally, I think it's gorgeous.

Comment 29

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #28)
> (In reply to Richard Marti (:Paenglab) from comment #27)
> > Is this really intended to have almost no difference between tabbar and
> > toolbar? And also the top border has not the same color as the tab border.
> 
> Yes, it's supposed to look like this - and personally, I think it's gorgeous.

Shouldn't the light highlight continue along the top border of the nav bar?

Comment 30

[Mike de Boer [:mikedeboer]]

I will check it and open a fresh bug to address the issue.

Comment 31

[Mike de Boer [:mikedeboer]]

Comment on attachment 8739427 [details] [diff] [review]
Patch 1.1: add support for dark GTK themes when applied to toolbars and the tab strip

Approval Request Comment
[Feature/regressing bug #]: bug 1184651
[User impact if declined]: On dark Linux GTK themes, the active tab, navbar and other toolbars will look out-of-place, i.e. very light. This patch fixes this case.
[Describe test coverage new/current, TreeHerder]: landed on m-c, one follow-up to be filed (separate issue).
[Risks and why]: minor.
[String/UUID change made/needed]: n/a.

Comment 32

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8739427 [details] [diff] [review]
Patch 1.1: add support for dark GTK themes when applied to toolbars and the tab strip

Tweak for gtk3 theme contrast, please uplift to aurora and beta. This should end up in beta 11 later this week.

Comment 33

[Matthew N. [:MattN]]

This showed up on Nightly screenshot comparisons. You can review them at the following URL to ensure this had the desired impact: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=d9b1a9829c8ee2862955043f28183efa07de3d2b&newProject=try&newRev=90b66fc83a14834764e361edb3c4dc87ad9b9f12

(They'll be ready when the https://treeherder.mozilla.org/#/jobs?repo=try&revision=90b66fc83a14834764e361edb3c4dc87ad9b9f12&filter-tier=1&filter-tier=2&filter-tier=3&exclusion_profile=false push jobs are done)

Comment 34

[Mike de Boer [:mikedeboer]]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #33)
> This showed up on Nightly screenshot comparisons. You can review them at the
> following URL to ensure this had the desired impact:
> https://screenshots.mattn.ca/compare/?oldProject=mozilla-
> central&oldRev=d9b1a9829c8ee2862955043f28183efa07de3d2b&newProject=try&newRev
> =90b66fc83a14834764e361edb3c4dc87ad9b9f12
> 
> (They'll be ready when the
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=90b66fc83a14834764e361edb3c4dc87ad9b9f12&filter-
> tier=1&filter-tier=2&filter-tier=3&exclusion_profile=false push jobs are
> done)

Thanks Matt for pushing that! The changes that it detects are expected. So we're all good :)

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a779e41439d6

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a3b8f0bcd916

Comment 37

[je-vv]

Thanks a lot guys, :-)  Appreciated.

Comment 38

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to reproduce this issue on Firefox 47.0a1 (2016-01-30).
The issue is no longer reproducible on Firefox 46.0 RC, Firefox 47.0a2 (2016-04-19) and on Firefox 48.0a1 (2016-04-19). 
Tests were performed on Ubuntu 14.04 x64.
I am marking this issue Verified Fixed.