1560660 - prefers-color-scheme media query doesn't reflect user's preference, after the theme has changed away from dark

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect, P3)

Description

[Daniel Holbert [:dholbert]]

STR:

1. Visit some page whose rendering depends on prefers-color-scheme dark (e.g. the bugzilla.m.o homepage, or this page

2. Switch to a dark OS theme, and then a light theme, and then a dark theme (checking the page rendering at each point). I'm using "Yaru" vs "Yaru-Dark" on Ubuntu 19.04.

EXPECTED RESULTS:
The page should change to reflect the dark theme choice when the user is using a dark theme.

ACTUAL RESULTS:
The page changes to reflect the dark theme the first time, but after I've switched away from dark mode, the page doesn't change anymore when I reactivate a dark theme (e.g. at the very end of my STR, the page is still in light mode even though I'm using a dark theme)

This is kind of a multi-part regression.
Before bug 1527048 landed, I got EXPECTED RESULTS.
After that bug landed, dark theme stopped being respected entirely, which was filed as bug 1555565.
Now that bug 1555565's been fixed, I see ACTUAL RESULTS (where dark theme gets respected for a single light-to-dark transition but not after multiple light-dark-light-dark transitions)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: screencast

Here's a screencast of the bug in action.

At 0:03, you can see us getting the light-to-dark transition correct (changing to the word "dark" in the testcase), and then getting the transition back to light correct as well.

But at 0:07, you can see that we don't react correctly from that point onwards -- we don't honor subsequent changes back into dark mode.

Also, reloading does not help (not shown in the screencast, just FYI). Basically this only seems to work on the first transition to dark mode during a Firefox session, and then it doesn't work at all after that.

Comment 2

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

Yes, I understand. We cache the value now as it's set before we update theme for content process. I wonder why

g_signal_connect_after(default_settings, "notify::gtk-theme-name", G_CALLBACK(settings_changed_cb), this);

does not re-cofigures it as we call nsWindow::ThemeChanged() from it.

Comment 3

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

I'll look at it as we should fix that in 68.

Comment 4

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

It seems to affects only e10s pages. Pages which are displayed in chrome process like about: works fine but pages running in content process stop responding to theme change after while.

Comment 5

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

I'm afraid there's no simple fix for that because when we once set gtk-theme-name in the content process, Gtk+ won't change it. So we don't get any notification that the system theme was changed and AFAIK there's no way how to get the system-wide theme easily. Only way how to fix that is to create IPC bridge for nsLookAndFeel and ask parent nsLookAndFeel component for mSystemUsesDarkTheme value.

Comment 6

[Dão Gottwald [:dao]]

Is this bug present on Windows and Mac? How is this handled there?

Comment 7

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

(In reply to Dão Gottwald [::dao] from comment #6)

Is this bug present on Windows and Mac? How is this handled there?

This si Gtk specific and it's caused by way how Gtk sets its theme. I think browser restart is an acceptable workaround here.

Comment 8

[Dão Gottwald [:dao]]

(In reply to Martin Stránský [:stransky] from comment #7)

I think browser restart is an acceptable workaround here.

Probably good enough when manually changing the theme. If and when OSes offer toggling dark mode automatically depending on the time of the day, I imagine this might become a more serious issue.

Comment 9

[Asif Youssuff]

(In reply to Dão Gottwald [::dao] from comment #8)

Probably good enough when manually changing the theme. If and when OSes offer toggling dark mode automatically depending on the time of the day, I imagine this might become a more serious issue.

FYI, iOS 13 offers this - for example sunrise to sunset.

https://www.macrumors.com/guide/ios-dark-mode/

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Happy to take a patch for 70 but since this is triaged and set to P3 priority I'm setting it as fix-optional.
That will remove the bug from weekly regression triage.

Comment 11

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

We're not going to fix this one, browser restart is required.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Does this work?

Comment 14

[Hiroyuki Ikezoe (:hiro)]

I haven't played the patch in comment 13 not so much, but it seems to work on my Linux box (ubuntu 18.04). To be honest I don't recall how our caching machinery works exactly, and I don't know what ConfigureContentGtkTheme is supposed to work, so I might be missing something there, but the patch should work. Martin?

(I wasn't going to take this)

Comment 15

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

I'll look at it, Thanks.

Comment 16

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

Hiroyuki, it looks good, do you mind to file a phabricator review request for it?
Thanks.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1560660 - Update mSystemUsesDarkTheme in the child process via caching machinery. r?stransky

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1398f3583623
Update mSystemUsesDarkTheme in the child process via caching machinery. r=stransky

Comment 19

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/1398f3583623

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

I'm not sure how prevalent this bug is for Linux ESR users - is this something we should consider uplifting there?

Comment 22

[Hiroyuki Ikezoe (:hiro)]

I am not sure how ESR commonly is used in distros. If it's common, we should uplift to ESR.

Martin?

Comment 23

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

AFAIK ESR is used for some stable distros like RHEL/Ubuntu LTS and also for Tor browser so it makes sense to uplift it.

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 9110675 [details]
Bug 1560660 - Update mSystemUsesDarkTheme in the child process via caching machinery. r?stransky

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Given that Firefox ESR is used in stable distros (as per comment 23) and bug 1527048 which regressed this has been uplifted to ESR, it would be nice to uplift this fix to ESR.
• User impact if declined: Theme change doesn't affect properly in content documents, it affects properly on browser process, user will be very confused and probably the inconsistency between browser and contents would be very annoying.
• Fix Landed on Version: Firefox 72
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change is pretty simple and it's only for Linux.
• String or UUID changes made by this patch: None

Comment 25

[Hiroyuki Ikezoe (:hiro)]

(In reply to Martin Stránský [:stransky] from comment #23)

AFAIK ESR is used for some stable distros like RHEL/Ubuntu LTS and also for Tor browser so it makes sense to uplift it.

Thanks Martin!

I couldn't find QE request in the ESR request and I don't quite follow what changed in recent release cycle changes. Do we still need to uplift to beta first before uplifting to ESR? I don't think it's worth uplifting this to beta but I DO think it's worth uplifting to ESR.

Anyways, the steps to reproduce is in comment 0.

Comment 26

[Julien Cristau [:jcristau]]

Comment on attachment 9110675 [details]
Bug 1560660 - Update mSystemUsesDarkTheme in the child process via caching machinery. r?stransky

fix for dark theme on gtk, approved for 68.4esr

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Attachment: Patch for ESR68

NarcisB noticed me on slack that the patch can't be applied to ESR cleanly. Here is the one to be able to be applied to ESR.

Comment 28

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/befd99804abb

Comment 29

[Cristian Fogel [:cfogel]]

Confirmed issue with 71.0 on Ubuntu 19.
Fix verified with 72.0b4.

Comment 30

[Cristian Fogel [:cfogel]]

Issue still present on 68.3.0 esr.
Managed to verify with the taskCluster build 68.4.0esr(20191209231253).