Closed Bug 1560660 Opened 1 year ago Closed 9 months ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla72
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 72+ verified
firefox67 --- unaffected
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- wontfix
firefox72 --- verified

People

(Reporter: dholbert, Assigned: hiro)

References

(Regression)

Details

(Keywords: regression)

Attachments

(4 files)

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)

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

Flags: needinfo?(stransky)

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.

Flags: needinfo?(stransky)

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

Assignee: nobody → stransky
Priority: P3 → P2

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.

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.

Priority: P2 → P3
Assignee: stransky → nobody

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

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

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

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

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.

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

Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → WONTFIX
Duplicate of this bug: 1595037
Attached patch Does this work?Splinter Review
Assignee: nobody → hikezoe.birchill
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---

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)

Assignee: hikezoe.birchill → nobody
Flags: needinfo?(stransky)

I'll look at it, Thanks.

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

Flags: needinfo?(hikezoe.birchill)
Flags: needinfo?(hikezoe.birchill)
Pushed by hikezoe.birchill@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1398f3583623
Update mSystemUsesDarkTheme in the child process via caching machinery. r=stransky
Status: REOPENED → RESOLVED
Closed: 11 months ago9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
Assignee: nobody → hikezoe.birchill

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

Flags: needinfo?(stransky)

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

Flags: needinfo?(hikezoe.birchill)

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

Martin?

Flags: needinfo?(hikezoe.birchill) → needinfo?(stransky)

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

Flags: needinfo?(stransky)

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
Attachment #9110675 - Flags: approval-mozilla-esr68?
Attachment #9108883 - Flags: approval-mozilla-esr68?

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

Flags: qe-verify+
Attachment #9108883 - Flags: approval-mozilla-esr68?

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

Attachment #9110675 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Attached patch Patch for ESR68Splinter Review

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.

QA Whiteboard: [qa-triaged]

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

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

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.