Open Bug 1540515 Opened 6 years ago Updated 2 years ago

Default theme in dark mode does not apply compacttheme.css

Categories

(WebExtensions :: Themes, defect, P3)

68 Branch
defect

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

Has Regression Range: --- → yes
Keywords: regression

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

Component: Theme → Add-ons Manager
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(kmaglione+bmo)
Product: Firefox → Toolkit

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

Flags: needinfo?(ntim.bugs)

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

Flags: needinfo?(ntim.bugs)

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

Flags: needinfo?(ntim.bugs)
No longer blocks: 1525762
Regressed by: 1525762
Component: Add-ons Manager → Themes
Priority: -- → P3
Product: Toolkit → WebExtensions

[Tracking Requested - why for this release]: I expect UX won't be okay with losing the compacttheme.css tweaks in dark mode.

What's the status of this now that bug 1540387 has landed?

Flags: needinfo?(ntim.bugs)

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

Flags: needinfo?(ntim.bugs)
Version: unspecified → Firefox 68

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

Priority: P3 → P2

Dao, Tim, is this something one of you can get to before the end of 68 nightly?

Flags: needinfo?(kmaglione+bmo)

Per triage adding ni for the two folks in Comment 9.

Flags: needinfo?(ntim.bugs)
Flags: needinfo?(dao+bmo)

Is Kris available to work on this? It's his regression and it seems like he had ideas for how this should be addressed.

Flags: needinfo?(dao+bmo)

Kris is not available to work on this before the end of 68.

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.

Assignee: nobody → kmaglione+bmo
Flags: needinfo?(ntim.bugs)

Kris, did you get a chance to look at this?

Flags: needinfo?(kmaglione+bmo)

Tim told me he was planning to fix this

Assignee: kmaglione+bmo → nobody
Flags: needinfo?(kmaglione+bmo) → needinfo?(ntim.bugs)

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 :)

Flags: needinfo?(ntim.bugs) → needinfo?(kmaglione+bmo)

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

Kris, we're about to build 68 rc. I guess that means this is wontfix for that release?

This would be very unpleasant as this affects 68 ESR too which has support for around a year.

Flags: needinfo?(kmaglione+bmo)
Priority: P2 → P3

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

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

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.

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.

Is there some planning to fix this regression? Hopefully soon.

Flags: needinfo?(ntim.bugs)
Flags: needinfo?(kmaglione+bmo)
Regressions: 1571634

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.

Flags: needinfo?(ntim.bugs)

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.

Version: Firefox 68 → 68 Branch

Marking fix-optional for 73 to remove it from weekly regression triage.

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

Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.