1472272 - Allow apps like Thunderbird to define a dark theme to use when the system appearance changes to dark mode

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Richard Marti (:Paenglab)]

FX uses "[e-mail]" as DARK_THEME_ID but TB uses "[e-mail]". So hard-coding it in https://hg.mozilla.org/mozilla-central/rev/24fe98c45aae#l3.12 makes it impossible to set the dark theme for TB. Renaming the TB to "[e-mail]" is also no real option as iut's a TB theme and not a FX one.

Stephen proposed to define DARK_THEME_ID during compile-time.

Comment 1

[Richard Marti (:Paenglab)]

Stephen, unfortunately my programming skills aren't enough to implement this. Would you be so kind to implement this? Many thanks in advance.

Comment 2

[Stephen A Pohl [:spohl]]

Attachment: Patch

This here looks like it would do the trick. Dao, is this the right way to achieve this in frontend code? Thank you!

Comment 3

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7cb1b45d690ff09afbc3a4bd0266dea0e86862bf
Bug 1472272: Allow apps like Thunderbird to switch to the dark theme as well when macOS 10.14+ switches to dark mode. r=dao

Comment 4

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52c64cf3183a
Backed out 1 changesets for xpcshell failures test_TelemetrySession.js CLOSED TREE

Comment 5

[Stephen A Pohl [:spohl]]

Attachment: Patch

Fixed telemetry test by adding null check.

Comment 6

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd57c1549f755550a9f43ada35652178a2a41193
Bug 1472272: Allow apps like Thunderbird to switch to the dark theme as well when macOS 10.14+ switches to dark mode. r=dao

Comment 7

[Andrei Ciure[:aciure]]

Backed out 1 changesets (bug 1472272) for xpcshell failures test_TelemetrySession.js

push that caused the backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/bd57c1549f755550a9f43ada35652178a2a41193

failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&group_state=expanded&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=185764954&filter-searchStr=Linux+opt+Xpcshell+tests+test-linux32%2Fopt-xpcshell-8+X%28X8%29

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

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/250aac27f0cbcbb3b72a93116bf5b33af2f84c30

Comment 8

[Dão Gottwald [:dao]]

How about we let nsBrowserGlue communicate to LightweightThemeManager that [e-mail] is the "dark" theme somewhere around here? https://searchfox.org/mozilla-central/rev/4074ba403219b7accdf00220b20dc492bfd4d83e/browser/components/nsBrowserGlue.js#755

Comment 9

[Jorg K (CEST = GMT+2)]

Many thanks guys for all the trouble to help Thunderbird!

Comment 10

[Stephen A Pohl [:spohl]]

(In reply to Dão Gottwald [::dao] from comment #8)
> How about we let nsBrowserGlue communicate to LightweightThemeManager that
> [e-mail] is the "dark" theme somewhere around here?
> https://searchfox.org/mozilla-central/rev/
> 4074ba403219b7accdf00220b20dc492bfd4d83e/browser/components/nsBrowserGlue.
> js#755

Before we do this I'd like to look into this test. The problem with the first patch was that Services.appinfo.name was null. But now, the test fails because the telemetry ping reports a buildId of "undefined" instead of the expected "20160315". This seems to be due to the way we load the addons manager for this test[1], and how we generate the gAppInfo. Simply accessing Services.appinfo in LightweightThemeManager.jsm will cause this test to fail.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/head.js#187
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/head.js#190,207-211

Comment 11

[Dão Gottwald [:dao]]

In my book doing this from nsBrowserGlue is preferable anyway. It would be a better separation of concerns without the need for LightweightThemeManager to pull clever tricks to know the id.

Comment 12

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d72f0721a55c06a6fa89e233673384ab0e702e0

Comment 13

[Stephen A Pohl [:spohl]]

Attachment: Patch

Comment 14

[Stephen A Pohl [:spohl]]

Comment on attachment 8989029 [details] [diff] [review]
Patch

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

Along these lines?

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8989029 [details] [diff] [review]
Patch

Looks good. I don't feel strongly about it but instead of a separate method I'd maybe extend addBuiltInTheme to support this directly. Something like this:

signature:
  addBuiltInTheme(theme, { useInDarkMode } = {}) {

call site:
  LightweightThemeManager.addBuiltInTheme({..}, { useInDarkMode: true });

Comment 16

[Stephen A Pohl [:spohl]]

Attachment: Patch

Comment 17

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15c9eddbb3312d7fc2a852cb6d9ec5ec12700f7f
Bug 1472272: Allow apps like Thunderbird to switch to the dark theme as well when macOS 10.14+ switches to dark mode. r=dao

Comment 18

[Richard Marti (:Paenglab)]

Many thanks for helping Thunderbird.

Comment 19

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/15c9eddbb331