1216658 - Ignore Gt3 dark themes and use light theme's color scheme for native widgets

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect)

Description

[Nicolas Silva [:nical]]

Attachment: Screenshot from 2015-10-20 19-39-14.png

When the gnome desktop is set to use the dark theme by default, Firefox looks generally horrible. Rather than rant about aesthetics, I'll focus on the usability issues. Web devs don't expect widgets like drop down menus, check boxes, input fields and friends to be dark, so often when the widget is tweaked with some CSS, not all of the colors a set and we end up with black text on a dark background or white text on a white background. In either case it is hard to read the text or even know that it is there without selecting it (see the dark text on dark background in the attachment for example).

I am filing this under widget because I assume that it has something to do with how we ask gtk to draw the widgets for us. I assume there is some way to ask gtk3 for the light theme, otherwise we need to set the default colors so that they match the default light theme instead of  using the dark one.

Comment 1

[Karl Tomlinson (:karlt)]

Same symptoms as bug 1158076, but this is proposing a different solution to the approach being investigated there.

Comment 2

[Nicolas Silva [:nical]]

Attachment: Another one. Try to read the Prénom and Nom fields

Comment 3

[Christian Stadelmann]

Possible solution code, working fine for the 3.18 branch of gnome-calendar:

g_object_set (gtk_settings_get_default (), "gtk-application-prefer-dark-theme", FALSE, NULL);

Source: https://git.gnome.org/browse/gnome-calendar/tree/src/gcal-application.c?h=gnome-3-18#n266

Comment 4

[Nicolas Silva [:nical]]

(In reply to Christian Stadelmann from comment #3)
> Possible solution code, working fine for the 3.18 branch of gnome-calendar:
> 
> g_object_set (gtk_settings_get_default (),
> "gtk-application-prefer-dark-theme", FALSE, NULL);

Great, thanks!

Comment 5

[Nicolas Silva [:nical]]

Attachment: ignore global gtk dark theme

The patch is simple enough and makes firefox usable with global gtk dark themes. Even if we want to eventually go for a more elaborate solution like bug 1158076, I propose that we take (and uplift) this one-liner in the mean time and revert it later if need be.

Comment 6

[Christian Stadelmann]

Thanks. You have a typo in your comment ("wdget" → "widget").

Comment 7

[Christian Stadelmann]

A better solution might be found in torbrowser's source code. Torbrowser fixes these issues at least for Gtk2. See https://gitweb.torproject.org/tor-browser.git/

Comment 8

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8705718 [details] [diff] [review]
ignore global gtk dark theme

Typo wdget -> widget

Well, it looks misleading to me to set a global widget configuration in nsLookAndFeel::Init(), but nsLookAndFeel::Init() is actually called before moz_gtk_init()/gtk3drawing.c which may be more suitable. 

So IMHO there isn't any option for that unless we want it in nsAppRunner.cpp/nsNativeAppSupportUnix.cpp.

Comment 9

[Christian Stadelmann]

Any chance to get this merged?

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 8705718 [details] [diff] [review]
ignore global gtk dark theme

nsLookAndFeel is an OK place for now, I think.

It happens to be called before gtk3drawing is used, but things are not so bad
if that changes because widgets in gtk3drawing should automatically update for
theme changes.

nsLookAndFeel should not be used before moz_gtk_init, but its nice to keep
such code in widget/gtk where possible.

My biggest concern is there is no way to override this.
Are there so many sites that set foreground colors on native form controls,
that no one would want to use a dark theme?

If you think so, then we can try this and see if anyone complains.

I will look at bug 1158076, but the misdemeanors of GTK 3.20 need more urgent
attention.

>+    // Disable dark theme because it interracts poorly with wdget styling in
>+    // web content.
>+    g_object_set(gtk_settings_get_default (),
>+                 "gtk-application-prefer-dark-theme", FALSE, NULL);
>+
>     // Gtk manages a screen's CSS in the settings object so we
>     // ask Gtk to create it explicitly. Otherwise we may end up
>     // with wrong color theme, see Bug 972382
>     (void)gtk_settings_get_for_screen(gdk_screen_get_default());

Please use the settings returned by the existing call, instead of adding another call.  (Keep the comment.)

s/NULL/nullptr/
This is C++ and so NULL is not (void*)0.
Passing 0, which is an int, is depending on calling convention to expand the
4 byte int to 8 bytes in varargs.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2fd4339f19c1

Comment 12

[Nicolas Silva [:nical]]

(In reply to Karl Tomlinson (back 26 Apr, ni?:karlt) from comment #10)
> My biggest concern is there is no way to override this.
> Are there so many sites that set foreground colors on native form controls,
> that no one would want to use a dark theme?

Yeah it breaks a lot of sites including some of ours. Firefox is just not usable with gtk's dark theme. I tried really hard to use it for a week and ended up disabling the global dark theme (out of clearly biased love for firefox but I would have kept the theme and stopped using any other app that would have had the same issue).

Ideally we'd have a separate setting for the chrome and for web content. For the former we have enough control that we could make things work, but for the latter there's absolutely no content that is authored with the possibility of a default have dark theme in mind so things are at best ugly and more often just unreadable.

We should uplift this as far as possible.

Comment 13

[Nicolas Silva [:nical]]

Comment on attachment 8705718 [details] [diff] [review]
ignore global gtk dark theme

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Most sites with forms are broken when using GNOME's global dark theme on Linux (see attached screenshots for example).
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: low.
[String/UUID change made/needed]: None.

Comment 14

[Carsten Book [:Tomcat]]

sorry had to back this out for valgrind failures like :

https://treeherder.mozilla.org/logviewer.html#?job_id=26872312&repo=mozilla-inbound

 18:25.76 TEST-UNEXPECTED-FAIL | valgrind-test | 33,652 (64 direct, 33,588 indirect) bytes in 1 blocks are definitely lost at malloc / g_malloc / g_slice_alloc / g_slice_alloc0
 18:25.76 ==28529== 33,652 (64 direct, 33,588 indirect) bytes in 1 blocks are definitely lost in loss record 9,123 of 9,128
 18:25.76 ==28529==    at 0x4C293D5: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
 18:25.76 ==28529==    by 0xB33CE08: g_malloc (gmem.c:159)
 18:25.76 ==28529==    by 0xB3519CF: g_slice_alloc (gslice.c:1003)
 18:25.76 ==28529==    by 0xB351F25: g_slice_alloc0 (gslice.c:1029)
 18:25.76 ==28529==    by 0xB0D27C9: g_type_create_instance (gtype.c:1870)
 18:25.77 ==28529==    by 0xB0BB9B8: g_object_constructor (gobject.c:1855)
 18:25.77 ==28529==    by 0xB0BB3A9: g_object_newv (gobject.c:1638)
 18:25.77 ==28529==    by 0xB0BB98B: g_object_new (gobject.c:1548)
 18:25.77 ==28529==    by 0x7B7DDC4: gtk_css_provider_get_named (gtkcssprovider.c:2664)
 18:25.77 ==28529==    by 0x7C71AF2: settings_update_theme (gtksettings.c:2873)
 18:25.77 ==28529==    by 0x7C7233C: settings_init_style (gtksettings.c:1557)
 18:25.77 ==28529==    by 0x7C7233C: gtk_settings_get_for_screen (gtksettings.c:1595)
 18:25.77 ==28529==    by 0xEAC09BD: nsLookAndFeel::Init() (nsLookAndFeel.cpp:1099)
 18:25.77 ==28529==    by 0xEA9A1A3: nsXPLookAndFeel::GetInstance() (nsXPLookAndFeel.cpp:264)
 18:25.77 ==28529==    by 0xEA9A670: mozilla::LookAndFeel::GetInt(mozilla::LookAndFeel::IntID, int*) (nsXPLookAndFeel.cpp:911)
 18:25.77 ==28529==    by 0xD729A24: GetInt (LookAndFeel.h:561)
 18:25.77 ==28529==    by 0xD729A24: nsChromeRegistryChrome::CheckForOSAccessibility() (nsChromeRegistryChrome.cpp:160)
 18:25.77 ==28529==    by 0xF0BAEF8: ScopedXPCOMStartup::SetWindowCreator(nsINativeAppSupport*) (nsAppRunner.cpp:1603)
 18:25.77 ==28529==    by 0xF0BEEDF: XREMain::XRE_mainRun() (nsAppRunner.cpp:4103)
 18:25.77 ==28529==    by 0xF0BFD88: XREMain::XRE_main(int, char**, nsXREAppData const*) (nsAppRunner.cpp:4451)
 18:25.77 ==28529==    by 0xF0C000D: XRE_main (nsAppRunner.cpp:4559)
 18:25.77 ==28529==    by 0x404FD9: do_main(int, char**, char**, nsIFile*) (nsBrowserApp.cpp:220)
18:25.77 ==28529== by 0x4046AD: main (nsBrowserApp.cpp:360)

Comment 15

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/896b9e2e58d6

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8705718 [details] [diff] [review]
ignore global gtk dark theme

Please re-nominate for uplift once reasons for backout are addressed.

Comment 17

[Karl Tomlinson (:karlt)]

I suspect a similar leak existed before.

Perhaps there's an existing suppression that needs a more general call stack.

Comment 18

[Nicolas Silva [:nical]]

(In reply to Karl Tomlinson (ni?:karlt) from comment #17)
> I suspect a similar leak existed before.
> 
> Perhaps there's an existing suppression that needs a more general call stack.

Yeah I looked into that but as far as I understand our code is legit. I added an entry to valgrind's suppression list and pushed to try, we'll see.

Comment 19

[Nicolas Silva [:nical]]

Attachment: Update valgrind's suppression list

Green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=074584aa22b945baac697078024e3182982f3262

Comment 20

[Karl Tomlinson (:karlt)]

(In reply to Carsten Book [:Tomcat] from comment #14)
> sorry had to back this out for valgrind failures like :

https://treeherder.mozilla.org/logviewer.html#?job_id=26867399&repo=mozilla-inbound#L24702

Comment 21

[Karl Tomlinson (:karlt)]

Comment on attachment 8748653 [details] [diff] [review]
Update valgrind's suppression list

I don't think we need this.  The leak indicates that "changing" the setting to
its current value is triggering unnecessary work.  Looking at the source, I
haven't found the cause of the leak, but settings_update_theme looks like it
is re-reading files.  I think we should avoid that by only setting when a
change is required.  That avoids the leak and re-reading files.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a306a95be12d&selectedJob=20433202

Please declare |settings| at the initialization.

https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html#GObject-notify
"Note that getting this signal doesn't guarantee that the value of the property has actually changed, it may also be emitted when the setter for the property is called to reinstate the previous value."

Comment 22

[Nicolas Silva [:nical]]

Attachment: Disable the dark theme (only when the setting is actually set to dark by default)

Looks good!
I folded the patch you pushed to try into the initial one, without valgrind suppression list modifications.

Comment 23

[Karl Tomlinson (:karlt)]

Comment on attachment 8750780 [details] [diff] [review]
Disable the dark theme (only when the setting is actually set to dark by default)

Thanks!

>+    // Disable dark theme because it interracts poorly with widget styling in

"interacts"

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/833fadcf5004

Comment 25

[Nicolas Silva [:nical]]

Comment on attachment 8750780 [details] [diff] [review]
Disable the dark theme (only when the setting is actually set to dark by default)

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Firefox on Linux is not usable if gnome/gtk is set to use the dark theme by default.
[Describe test coverage new/current, TreeHerder]: None.
[Risks and why]: low. the patch does not affect users that don't use the default dark theme, and for those who do, firefox is not usable without the patch.
[String/UUID change made/needed]: None.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/833fadcf5004

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Nical, did you get a chance to manually test this change? Test coverage "None" does not give me too much confidence about the accuracy of this patch at addressing the issue. :(

Comment 28

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8750780 [details] [diff] [review]
Disable the dark theme (only when the setting is actually set to dark by default)

Fixes gtk dark theme mode in Firefox on linux, Aurora48+, Beta47+

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1b2854d057e7

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/93a53170dedf

Comment 31

[Nicolas Silva [:nical]]

(In reply to Ritu Kothari (:ritu) from comment #27)
> Nical, did you get a chance to manually test this change? Test coverage
> "None" does not give me too much confidence about the accuracy of this patch
> at addressing the issue. :(

Yes, I tested it locally and now I can set my system back to use the global dark theme so I'll be running in this configuration which is some form of passive testing I guess. None of our automated tests run in this config though, so strictly speaking the test coverage is non-existent.
That said, this patch has the effect of making an "odd" configuration behave more like the default one so the absence of testing was more problematic before than it is now (we ended up shipping a release in which a gnome setting can make firefox display white text on white backgrounds in editable text areas after all).
Thanks for the approval.

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Nicolas Silva [:nical] from comment #31)
> (In reply to Ritu Kothari (:ritu) from comment #27)
> > Nical, did you get a chance to manually test this change? Test coverage
> > "None" does not give me too much confidence about the accuracy of this patch
> > at addressing the issue. :(
> 
> Yes, I tested it locally and now I can set my system back to use the global
> dark theme so I'll be running in this configuration which is some form of
> passive testing I guess. 

That helps! :) Thanks a lot for your due diligence.

Comment 33

[Mihai Boldan, Desktop QA [:mboldan]]

I was not able to reproduce this issue on Firefox 46.0.1 RC and on Ubuntu 14.04 x64, using Gnomishdark theme.
Nicolas, can you please provide me more information about this issue, as STR, OS, or what changes I have to make in order to reproduce this issue.

Comment 34

[Nicolas Silva [:nical]]

(In reply to Mihai Boldan, QA [:mboldan] from comment #33)
> I was not able to reproduce this issue on Firefox 46.0.1 RC and on Ubuntu
> 14.04 x64, using Gnomishdark theme.
> Nicolas, can you please provide me more information about this issue, as
> STR, OS, or what changes I have to make in order to reproduce this issue.

To reproduce this on a GNOME linux desktop, install gnome-tweak-tools and use it to enable the global dark theme option.
Then open firefox and browse around to see if native controls look off. Native controls typically include input text areas, check boxes, drop-down menus, etc.

Comment 35

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to reproduce this issue on Firefox 45.0.2 and on Ubuntu 14.04 x64.
The issue is no longer reproducible on Firefox 47.0b9, Firefox 48.0a2 (2016-05-30), Firefox 49.0a1 (2016-05-30).
I'm marking this issue Verified Fixed.

Comment 36

[Tiago Peixoto]

Well, this "fix" is not really nice at all. Now firefox sticks out like a sore thumb when dark themes are being used. I have been using firefox with dark themes for quite some time without any issue, and now it quite unusable. There should be at least a way to force activation of dark themes.

Comment 37

[Omar Pakker]

Ok so this has completely broken dark theme support right now..
Is there a config option to force Firefox back to honouring the dark theme switch?

Comment 38

[Christian Stadelmann]

(In reply to Omar Pakker from comment #37)
> Ok so this has completely broken dark theme support right now..
> Is there a config option to force Firefox back to honouring the dark theme
> switch?

Do you mean dark Firefox themes or dark Gtk+ themes?

You can't get dark Gtk+ themes back until either all websites stop assuming light widget themes (will never happen) or until there is a way to set different theme variants for XUL/chrome and website content. The latter is possible with Gtk+, but only on a multi-process architecture like e10s, where the main process could run with dark themes and the web renderer processes could run with light theme. Please have a look at the discussion for bug #1272332.

Comment 39

[Omar Pakker]

There were several ways in which users of dark themes could've worked around any problems they may have had with site fields that would not have had to impact users that use the dark theme normally.
For example:
- A very small userContent.css file that just sets the input field background to white.
- Setting GTK_THEME before starting Firefox.

The GTK dark theme support would not have had to be broken with this patch to 'fix' something that wasn't a bug in Firefox.

Anyhow, I just reverted this change and recompiled Firefox in the mean time. Let's hope this is fixed by the time Firefox 48 is released.

Comment 40

[Christian Stadelmann]

(In reply to Omar Pakker from comment #39)
> There were several ways in which users of dark themes could've worked around
> any problems they may have had with site fields that would not have had to
> impact users that use the dark theme normally.
> For example:
> - A very small userContent.css file that just sets the input field
> background to white.

This is true. But it should have been shipped (built-in) in Firefox, no user interaction required. Tor Browser (based on Firefox ESR) does theme normalization and probably works that way.

> - Setting GTK_THEME before starting Firefox.

This is no fix, but only a bad workaround. “bad” because Gtk+ developers discourage using this switch. And because it also changes the theme for all applications started by firefox.

> 
> The GTK dark theme support would not have had to be broken with this patch
> to 'fix' something that wasn't a bug in Firefox.
> 
> Anyhow, I just reverted this change and recompiled Firefox in the mean time.
> Let's hope this is fixed by the time Firefox 48 is released.

You probably should have a look at bug #1272332 where they try to find a useful fix.

Comment 41

[Omar Pakker]

> > - Setting GTK_THEME before starting Firefox.
> 
> This is no fix, but only a bad workaround. “bad” because Gtk+ developers
> discourage using this switch. And because it also changes the theme for all
> applications started by firefox.
I didn't call it a work around for nothing. But in my personal opinion I still think completely breaking dark theme support for something that's not a bug in Firefox itself was a very odd decision.

> > 
> > The GTK dark theme support would not have had to be broken with this patch
> > to 'fix' something that wasn't a bug in Firefox.
> > 
> > Anyhow, I just reverted this change and recompiled Firefox in the mean time.
> > Let's hope this is fixed by the time Firefox 48 is released.
> 
> You probably should have a look at bug #1272332 where they try to find a
> useful fix.
I've been following this one since you first mentioned it ;)

Comment 42

[Dão Gottwald [:dao]]

(In reply to Christian Stadelmann from comment #38)
> You can't get dark Gtk+ themes back until either all websites stop assuming
> light widget themes (will never happen) or until there is a way to set
> different theme variants for XUL/chrome and website content. The latter is
> possible with Gtk+, but only on a multi-process architecture like e10s,
> where the main process could run with dark themes and the web renderer
> processes could run with light theme. Please have a look at the discussion
> for bug #1272332.

It looks like bug 1272332 took a different direction and just added support for a hidden pref...? Clearly this isn't ideal. We should reasonably integrate with the host OS by default and not push users towards hidden prefs that end up breaking web content. Is there another followup bug for the chrome / content separation that's apparently needed here?

Comment 43

[Karl Tomlinson (:karlt)]

(In reply to Dão Gottwald [:dao] from comment #42)
> It looks like bug 1272332 took a different direction and just added support
> for a hidden pref...? Clearly this isn't ideal. We should reasonably
> integrate with the host OS by default and not push users towards hidden
> prefs that end up breaking web content.

FWIW dark and light variants of themes are designed so that applications that use gtk-application-prefer-dark-theme do integrate.

I agree that the ideal is to use the preferred variant for chrome.

I don't think anyone is encouraging use of this pref, but it is available for those who want to try it.

> Is there another followup bug for
> the chrome / content separation that's apparently needed here?

Bug 1158076 is still open.

So far momentum there has been in the direction of separation by chrome/content caller.  Separation by chrome/content process instead of chrome/content caller would be a much simpler solution if that can work.

Comment 44

[Dão Gottwald [:dao]]

(In reply to Karl Tomlinson (ni?:karlt) from comment #43)
> (In reply to Dão Gottwald [:dao] from comment #42)
> > It looks like bug 1272332 took a different direction and just added support
> > for a hidden pref...? Clearly this isn't ideal. We should reasonably
> > integrate with the host OS by default and not push users towards hidden
> > prefs that end up breaking web content.
> 
> FWIW dark and light variants of themes are designed so that applications
> that use gtk-application-prefer-dark-theme do integrate.
> 
> I agree that the ideal is to use the preferred variant for chrome.

I find it hard to argue that attachment 8751748 [details] looks integrated. It seems fairly jarring and as an affected user this would motivate me to look for another browsers. Luckily enough for us, Chrome isn't any better and alternatives are sparse on Linux. (The flipside is that we could use this as a competitive advantage over Chrome. I don't want to exaggerate, though; the number of affected users won't be huge.)

> I don't think anyone is encouraging use of this pref, but it is available
> for those who want to try it.

The subpar user experience encourages the use of the pref. We can't assume that users will make an informed decision either. They'll just find out about the pref in a forum or from a friend and (not unwarrantedly) blame Firefox when encountering broken content. Even if they do make a fully informed decision to flip the pref, they still won't be happy about broken content. We're giving them a choice where both options suck.

Comment 45

[Serhii Tereshchenko]

Hi all. Firefox user here.

I'm using dark theme for my gnome, and i'm not happy firefox using only light theme.

I remember when some inputs on some web sites was dark, and was not happy with it either.

Probably right solution is to use dark theme for window border/user interface and light for web page content. (Or have some default colors for web content, probaly it is better solution, becouse if my light theme is pink i still want to see white textarea in browser, not pink).

Comment 46

[Kevin Brodsky]

I think that the chrome/content separate handling for dark themes will have to wait for Electrolysis to be deployed. In the meantime, there will be a way to restore the old behaviour in Firefox 50, see bug 1272332.

Comment 47

[Joshua Hudson]

Possible duplicate: 825578 (via 727869 which describes this exact scenario).

Comment 48

[Scott Palmer]

No need to modify .desktop files and download the beta builds!

I was able to get just the dark window controls (Gnome 3.20) using:
https://addons.mozilla.org/en-us/firefox/addon/darkwdec/

It also works alongside:
https://addons.mozilla.org/en-US/firefox/addon/adwaita-dark/