1740089 - Centralize dark/light theme decisions and provide reasonable fallback when toolbar color isn't set.

Status: RESOLVED FIXED

(Firefox :: Theme, task)

Description

[Emilio Cobos Álvarez (:emilio)]

STR:

• Install a theme that doesn't provide a toolbar color like https://addons.mozilla.org/en-US/firefox/addon/a-n-i-m-a-t-e-d-kitty-cat/

ER:

• Toolbar-color should probably be considered light since it does provide a frame color.

AR:

• Toolbar color is not there, so we fall back to "system", which might be dark and look a bit off.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1740089 - Use CSS.escape to escape URIs in LightweightThemeConsumer. r=#desktop-theme-reviewers

The code caught my attention because it broke my editor's syntax
highlighting. This should be more correct.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1740089 - Centralize theme computation in LightweightThemeConsumer. r=#desktop-theme-reviewers

This shouldn't change behavior.

Depends on D130668

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1740089 - Fall back to frame (accentcolor) to determine toolbar theme. r=#desktop-theme-reviewers

Depends on D130669

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: comm-central cleanup

After the previous patches all this should be unnecessary.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(I ni?d because I can't r?)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c9f35164a4f
Use CSS.escape to escape URIs in LightweightThemeConsumer. r=desktop-theme-reviewers,harry
https://hg.mozilla.org/integration/autoland/rev/ac810bf14fd5
Centralize theme computation in LightweightThemeConsumer. r=desktop-theme-reviewers,harry
https://hg.mozilla.org/integration/autoland/rev/a4546a4b8991
Fall back to frame (accentcolor) to determine toolbar theme. r=desktop-theme-reviewers,harry

Comment 7

[Iulian Moraru]

Backed out for causing bc failures on browser_ext_browserAction_theme_icons.js.

The affected platforms are Linux 18.04 x64 WebRender opt and debug.

Push with failures

Failure log

Backout link

Comment 8

[Richard Marti (:Paenglab)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

Created attachment 9249840 [details] [diff] [review]
comm-central cleanup

After the previous patches all this should be unnecessary.

I'll let Alessandro make the review as it is his code that's changed.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5bbb34d94f7
Use CSS.escape to escape URIs in LightweightThemeConsumer. r=desktop-theme-reviewers,harry
https://hg.mozilla.org/integration/autoland/rev/9a5d45e4cf07
Centralize theme computation in LightweightThemeConsumer. r=desktop-theme-reviewers,harry

Comment 10

[Alessandro Castellani [:aleca]]

Maybe is due to D130670 not in m-c yet, but this patch doesn't work unfortunately.
Switching to dark theme doesn't properly update the in-content style (prefs, account settings, account setup, etc.). Partial sections of the UI and other pages are not properly updated with an hybrid state of light text and light bg.

Thanks for thinking about us and keeping us in the loop :D

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Well the other two patches aren't in central either so that's not too surprising. After that happens it should work :)

Comment 12

[Alessandro Castellani [:aleca]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #11)

Well the other two patches aren't in central either so that's not too surprising. After that happens it should work :)

I'm not a smart man, I didn't see it was on autoland but not on m-c.
I'll circle back to this and test it again, thanks and sorry for noise.

Comment 13

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/f5bbb34d94f7
https://hg.mozilla.org/mozilla-central/rev/9a5d45e4cf07

Comment 14

[Alessandro Castellani [:aleca]]

Comment on attachment 9249840 [details] [diff] [review]
comm-central cleanup

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

This is perfect!
Tested this on Linux, Windows, and macOS, and everything works as expected.
The dark/light theme variation properly updates both chrome and content, and the auto theme properly follows the OS themeing.

Thank you so much for doing this.

Comment 15

[Alessandro Castellani [:aleca]]

Emilio, can I mark this for checkin-needed-tb only for the patch you made for TB?

Comment 16

[Emilio Cobos Álvarez (:emilio)]

I suppose? I can push it otherwise I guess :)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30ef1f19b746
Fall back to frame (accentcolor) to determine toolbar theme. r=desktop-theme-reviewers,harry

Comment 18

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/30ef1f19b746

Comment 19

[Alessandro Castellani [:aleca]]

Can you push the comm-central cleanup patch attached to this bug?

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/928f80231a11
Remove now-unused code. r=aleca

Comment 21

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Attachment: screenshot

Hello Emilio! I'm having some trouble reproducing the issue with Firefox 96.0a1 (20211108214919) on Windows 10x64, macOS 10.15, and Ubuntu 21.1. For me when installing the theme provided in comment 0, the toolbar color is white as well on the affected build even if the system color is dark.

Am I doing something wrong here? Attached screenshot of Windows 10x64 with the affected and fixed build.

Or maybe could you verify the fix on the latest beta if you have the time? Thank you in advance.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

The toolbar is always, light, but before the patch we choose a dark color for content (erroneously). With the theme installed both the UI and the content color (i.e., about:support backgrounds etc) should be light.