Closed Bug 1472272 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: Paenglab, Assigned: spohl)

References

Details

Attachments

(1 file, 3 obsolete files)

FX uses "firefox-compact-dark@mozilla.org" as DARK_THEME_ID but TB uses "thunderbird-compact-dark@mozilla.org". 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 "firefox-compact-dark@mozilla.org" 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.
Stephen, unfortunately my programming skills aren't enough to implement this. Would you be so kind to implement this? Many thanks in advance.
Flags: needinfo?(spohl.mozilla.bugs)
Attached patch Patch (obsolete) — Splinter Review
This here looks like it would do the trick. Dao, is this the right way to achieve this in frontend code? Thank you!
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Flags: needinfo?(spohl.mozilla.bugs)
Attachment #8988965 - Flags: review?(dao+bmo)
Attachment #8988965 - Flags: review?(dao+bmo) → review+
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
Summary: Define the DARK_THEME_ID during the compile time instead of hard-coding it in toolkit. → Make the definition of DARK_THEME_ID depend on Services.appinfo.name to support apps like Thunderbird
Backout by aciure@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52c64cf3183a
Backed out 1 changesets for xpcshell failures test_TelemetrySession.js CLOSED TREE
Attached patch Patch (obsolete) — Splinter Review
Fixed telemetry test by adding null check.
Attachment #8988965 - Attachment is obsolete: true
Attachment #8988997 - Flags: review+
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
How about we let nsBrowserGlue communicate to LightweightThemeManager that firefox-compact-dark@mozilla.org is the "dark" theme somewhere around here? https://searchfox.org/mozilla-central/rev/4074ba403219b7accdf00220b20dc492bfd4d83e/browser/components/nsBrowserGlue.js#755
Many thanks guys for all the trouble to help Thunderbird!
(In reply to Dão Gottwald [::dao] from comment #8)
> How about we let nsBrowserGlue communicate to LightweightThemeManager that
> firefox-compact-dark@mozilla.org 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
Flags: needinfo?(spohl.mozilla.bugs)
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.
Attached patch Patch (obsolete) — Splinter Review
Attachment #8989029 - Flags: review?(dao+bmo)
Comment on attachment 8989029 [details] [diff] [review]
Patch

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

Along these lines?
Attachment #8988997 - Attachment is obsolete: true
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 });
Attachment #8989029 - Flags: review?(dao+bmo) → review+
Attached patch PatchSplinter Review
Attachment #8989029 - Attachment is obsolete: true
Attachment #8989073 - Flags: review?(dao+bmo)
Attachment #8989073 - Flags: review?(dao+bmo) → review+
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
Summary: Make the definition of DARK_THEME_ID depend on Services.appinfo.name to support apps like Thunderbird → Allow apps like Thunderbird to define a dark theme to use when the system appearance changes to dark mode
Many thanks for helping Thunderbird.
https://hg.mozilla.org/mozilla-central/rev/15c9eddbb331
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.