Closed Bug 1216658 Opened 9 years ago Closed 8 years ago

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

Categories

(Core :: Widget: Gtk, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
firefox47 --- verified
firefox48 --- verified
firefox49 --- verified

People

(Reporter: nical, Assigned: nical)

References

Details

Attachments

(3 files, 2 obsolete files)

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.
Same symptoms as bug 1158076, but this is proposing a different solution to the approach being investigated there.
Blocks: 1158076
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
(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!
Attached patch ignore global gtk dark theme (obsolete) — Splinter Review
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.
Attachment #8705718 - Flags: review?(karlt)
Attachment #8705718 - Flags: feedback?(stransky)
Thanks. You have a typo in your comment ("wdget" → "widget").
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 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.
Attachment #8705718 - Flags: feedback?(stransky)
Any chance to get this merged?
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.
Attachment #8705718 - Flags: review?(karlt) → review+
(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 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.
Attachment #8705718 - Flags: approval-mozilla-beta?
Attachment #8705718 - Flags: approval-mozilla-aurora?
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)
Flags: needinfo?(nical.bugzilla)
Comment on attachment 8705718 [details] [diff] [review]
ignore global gtk dark theme

Please re-nominate for uplift once reasons for backout are addressed.
Attachment #8705718 - Flags: approval-mozilla-beta?
Attachment #8705718 - Flags: approval-mozilla-aurora?
I suspect a similar leak existed before.

Perhaps there's an existing suppression that needs a more general call stack.
(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.
Flags: needinfo?(nical.bugzilla)
Green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=074584aa22b945baac697078024e3182982f3262
Assignee: nobody → nical.bugzilla
Attachment #8748653 - Flags: review?(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 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."
Attachment #8748653 - Flags: review?(karlt)
Looks good!
I folded the patch you pushed to try into the initial one, without valgrind suppression list modifications.
Attachment #8705718 - Attachment is obsolete: true
Attachment #8748653 - Attachment is obsolete: true
Attachment #8750780 - Flags: review?(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"
Attachment #8750780 - Flags: review?(karlt) → review+
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.
Attachment #8750780 - Flags: approval-mozilla-beta?
Attachment #8750780 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/833fadcf5004
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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. :(
Flags: needinfo?(nical.bugzilla)
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+
Attachment #8750780 - Flags: approval-mozilla-beta?
Attachment #8750780 - Flags: approval-mozilla-beta+
Attachment #8750780 - Flags: approval-mozilla-aurora?
Attachment #8750780 - Flags: approval-mozilla-aurora+
(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.
Flags: needinfo?(nical.bugzilla)
Depends on: 1272332
Flags: qe-verify+
(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.
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.
Flags: needinfo?(nical.bugzilla)
(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.
Flags: needinfo?(nical.bugzilla)
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
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.
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?
(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.
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.
(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.
> > - 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 ;)
(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?
Flags: needinfo?(nical.bugzilla)
Flags: needinfo?(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.
Flags: needinfo?(karlt)
(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.
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).
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.
Possible duplicate: 825578 (via 727869 which describes this exact scenario).
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/
Flags: needinfo?(nical.bugzilla)
You need to log in before you can comment on or make changes to this bug.