Default theme in dark mode does not apply compacttheme.css
Categories
(WebExtensions :: Themes, defect, P3)
Tracking
(firefox-esr60 unaffected, firefox-esr68 wontfix, firefox66 unaffected, firefox67 unaffected, firefox68+ wontfix, firefox69 wontfix, firefox70 wontfix, firefox71 wontfix, firefox72 wontfix, firefox73 wontfix)
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox-esr68 | --- | wontfix |
firefox66 | --- | unaffected |
firefox67 | --- | unaffected |
firefox68 | + | wontfix |
firefox69 | --- | wontfix |
firefox70 | --- | wontfix |
firefox71 | --- | wontfix |
firefox72 | --- | wontfix |
firefox73 | --- | wontfix |
People
(Reporter: ntim, Unassigned, NeedInfo)
References
(Regression)
Details
(Keywords: regression)
Regression of bug 1525762.
Kris suggested we load compacttheme.css regardless of whether we're in dark mode and use :-moz-lwtheme selectors to make sure it doesn't apply the compacttheme.css styles on the default theme in light mode.
EDIT: clarified a bit
Reporter | ||
Updated•6 years ago
|
Comment 1•6 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #0)
Regression of bug 1525762.
Kris suggested we load compacttheme.css regardless of whether we're in dark mode and use :-moz-lwtheme selectors to make sure it doesn't apply the default theme in light mode.
What does this even mean? Technically, compacttheme.css is always loaded (but disabled), and the default theme is always applied, with other themes sitting on top.
Reporter | ||
Comment 2•6 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #1)
(In reply to Tim Nguyen :ntim from comment #0)
Regression of bug 1525762.
Kris suggested we load compacttheme.css regardless of whether we're in dark mode and use :-moz-lwtheme selectors to make sure it doesn't apply the default theme in light mode.
What does this even mean? Technically, compacttheme.css is always loaded (but disabled), and the default theme is always applied, with other themes sitting on top.
Enabling compacttheme.css regardless of whether we're in dark mode and prefix everything in compacttheme.css with :-moz-lwtheme selectors to make sure it doesn't apply on the default theme.
Comment 3•6 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #2)
(In reply to Dão Gottwald [::dao] from comment #1)
(In reply to Tim Nguyen :ntim from comment #0)
Regression of bug 1525762.
Kris suggested we load compacttheme.css regardless of whether we're in dark mode and use :-moz-lwtheme selectors to make sure it doesn't apply the default theme in light mode.
What does this even mean? Technically, compacttheme.css is always loaded (but disabled), and the default theme is always applied, with other themes sitting on top.
Enabling compacttheme.css regardless of whether we're in dark mode
The code enabling compacttheme.css doesn't care about dark mode, so I guess what you mean is enabling it for the default theme? https://searchfox.org/mozilla-central/rev/201450283cddc9e409cec707acb65ba6cf6037b1/browser/base/content/browser-compacttheme.js#29
Also, how does bug 1540387 play into this?
Reporter | ||
Comment 4•6 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #3)
so I guess what you mean is enabling it for the default theme?
Yes, that's what Kris was suggesting at least. There might be better alternative solutions.
Also, how does bug 1540387 play into this?
Depends on the fix that we're planning to use here.
Reporter | ||
Updated•6 years ago
|
Updated•6 years ago
|
Comment 5•6 years ago
|
||
[Tracking Requested - why for this release]: I expect UX won't be okay with losing the compacttheme.css tweaks in dark mode.
Comment 6•6 years ago
|
||
What's the status of this now that bug 1540387 has landed?
Reporter | ||
Comment 7•6 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #6)
What's the status of this now that bug 1540387 has landed?
This is still relevant. Kris was suggesting on IRC to specify the experiment as part of the theme, rather than a separate manifest field so you can have 2 different experiments for theme and dark_theme in manifest.json, which is a solution I'm fine with.
Updated•6 years ago
|
Comment 8•6 years ago
|
||
Changing the priority to p2 as the bug is tracked by a release manager for the current nightly.
See How Do You Triage for more information
Comment 9•6 years ago
|
||
Dao, Tim, is this something one of you can get to before the end of 68 nightly?
Updated•6 years ago
|
Comment 10•6 years ago
|
||
Per triage adding ni for the two folks in Comment 9.
Comment 11•6 years ago
|
||
Is Kris available to work on this? It's his regression and it seems like he had ideas for how this should be addressed.
Comment 12•6 years ago
|
||
Kris is not available to work on this before the end of 68.
Comment 13•6 years ago
|
||
My next best suggestion would be aswan or ntim then. I was barely involved with bug 1525762 and am generally not very familiar with the webextension theme infrastructure.
Updated•5 years ago
|
Updated•5 years ago
|
Comment 14•5 years ago
|
||
Kris, did you get a chance to look at this?
Comment 15•5 years ago
|
||
Tim told me he was planning to fix this
Reporter | ||
Comment 16•5 years ago
|
||
I know I said I could take this, but I have less time than expected at the moment and I'm quite sick right now. I don't have time to take this until at least the start of my internship on July 8th. And this needs to be fixed before Firefox 68 (released on July 8th) since it would regress bug 1504766, which many users were frustrated about.
One of things you could do quickly is manually enable/disable or load/unload the stylesheet in https://searchfox.org/mozilla-central/rev/b3b401254229f0a26f7ee625ef5f09c6c31e3949/toolkit/modules/LightweightThemeConsumer.jsm#228-232 . I think this is acceptable for now and I can clean that up next month if that sounds ok to you. Kris, do you mind taking this and take care of uplifting to beta ? This way bug 1504766 won't be regressed.
Thanks :)
Comment 17•5 years ago
|
||
Coming here from bug 1504840 which was apparently regressed by whatever bug caused this one to be filed. Concrete example of the side effects of the tweaks not being applied is that the blue "unseen" coloring on folder names is not readable in dark mode when using the default theme. This was previously fixed in bug 1504840 and the fix was lost with the latest nightly build (and I was pointed at this bug for the potential fix because if you explicitly set the dark theme it's readable again).
Comment 18•5 years ago
|
||
Kris, we're about to build 68 rc. I guess that means this is wontfix for that release?
Comment 19•5 years ago
|
||
This would be very unpleasant as this affects 68 ESR too which has support for around a year.
Updated•5 years ago
|
Comment 20•5 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #16)
And this needs to be fixed before Firefox 68 (released on July 8th) since it would regress bug 1504766, which many users were frustrated about.
Actually the fix for that bug lives in browser-aero.css so I think / hope it's not affected?
After having done a full review of the compacttheme[.inc].css files, I think the following rules are the only affected ones.
- cross-platform: https://searchfox.org/mozilla-central/rev/f91bd38732d4a330eba4e780812274b98eb81274/browser/themes/shared/compacttheme.inc.css#9-13 (suboptimal but not critical)
- Mac: https://searchfox.org/mozilla-central/rev/f91bd38732d4a330eba4e780812274b98eb81274/browser/themes/osx/compacttheme.css#12-15 (might be critical?)
Reporter | ||
Comment 21•5 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #20)
(In reply to Tim Nguyen :ntim from comment #16)
And this needs to be fixed before Firefox 68 (released on July 8th) since it would regress bug 1504766, which many users were frustrated about.
Actually the fix for that bug lives in browser-aero.css so I think / hope it's not affected?
Ah you're right, sorry :)
After having done a full review of the compacttheme[.inc].css files, I think the following rules are the only affected ones.
- cross-platform: https://searchfox.org/mozilla-central/rev/f91bd38732d4a330eba4e780812274b98eb81274/browser/themes/shared/compacttheme.inc.css#9-13 (suboptimal but not critical)
This does change the appearance, but yeah, it's probably not critical. Bug 1550090 would make it easier to fix this bug, but I haven't had time to look into the failures so far. I guess this can wait until Firefox 69.
This rule affects more specifically the light theme (so dark mode won't necessarily be affected), but the dark theme rule looks fishy too. I'm going to fix this in bug 1562220.
Comment 22•5 years ago
|
||
I'm going to go ahead and mark this wontfix for 68.0, and we can revisit for a future 68.x update on ESR.
Comment 23•5 years ago
|
||
Is there some planning to fix this regression? Hopefully soon.
Reporter | ||
Comment 24•5 years ago
|
||
Since compacttheme.css doesn't contain that much more tweaks, we decided to leave it there for now.
The only tweaks are: https://searchfox.org/mozilla-central/source/browser/themes/shared/compacttheme.inc.css
The Windows ones are Windows 7/8 specific, which don't have a system dark mode, so this is not particularly relevant.
The MacOS ones are specific to the light theme.
The plan is to just eventually get rid of compacttheme.css.
Happy to take a patch for 70 or beyond.
Since we are getting close to the end of the 69 beta cycle and this is set to P3, I'm marking it fix-optional for 69 and 70 to remove it from weekly triage.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Marking fix-optional for 73 to remove it from weekly regression triage.
Comment 29•3 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #3)
(In reply to Tim Nguyen :ntim from comment #2)
(In reply to Dão Gottwald [::dao] from comment #1)
(In reply to Tim Nguyen :ntim from comment #0)
Regression of bug 1525762.
Kris suggested we load compacttheme.css regardless of whether we're in dark mode and use :-moz-lwtheme selectors to make sure it doesn't apply the default theme in light mode.
What does this even mean? Technically, compacttheme.css is always loaded (but disabled), and the default theme is always applied, with other themes sitting on top.
Enabling compacttheme.css regardless of whether we're in dark mode
The code enabling compacttheme.css doesn't care about dark mode, so I guess what you mean is enabling it for the default theme? https://searchfox.org/mozilla-central/rev/201450283cddc9e409cec707acb65ba6cf6037b1/browser/base/content/browser-compacttheme.js#29
Also, how does bug 1540387 play into this?
Hello there, I am also facing the same issue with my project https://x8spedersapk.id/ and plz tell me the axect soloution.
Updated•2 years ago
|
Updated•2 years ago
|
Description
•