Closed Bug 1093368 Opened 5 years ago Closed 5 years ago

Customizable theme picker fires a lightweight-theme-styling-update with the default theme ID

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 36
Iteration:
36.2
Tracking Status
firefox34 --- fixed
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: bgrins, Assigned: dao)

References

Details

Attachments

(2 files, 1 obsolete file)

A lightweight-theme-styling-update comes through with '{972ce4c6-7e08-4474-a285-3208198ce6fd}' as the theme ID when following these steps:

Open customizable UI
Open theme dropdown
Hover over a lightweight theme from the menu
Select the default theme from the menu

I wasn't expecting this, since the default theme is not a lightweight theme
Component: Theme → Toolbars and Customization
Blocks: 1007336
Points: --- → 2
Flags: firefox-backlog+
Blocks: 1053434
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #8518207 - Flags: review?(jaws)
Iteration: --- → 36.2
Flags: qe-verify-
Comment on attachment 8518207 [details] [diff] [review]
patch

Stealing - this breaks hovering over the default theme and un-applying the lightweight theme in that case, if you're already using a lightweight theme.
Attachment #8518207 - Flags: review?(jaws) → review-
Attached patch patchSplinter Review
Amazing how many bugs are layered on top of each other here.
Attachment #8518207 - Attachment is obsolete: true
Attachment #8518259 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8518259 [details] [diff] [review]
patch

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

(In reply to Dão Gottwald [:dao] from comment #3)
> Created attachment 8518259 [details] [diff] [review]
> patch
> 
> Amazing how many bugs are layered on top of each other here.

I don't actually understand what you mean here. In any case, this doesn't address comment 2.

To be clear, here's my STR:

0. apply patch, rebuild browser/base, browser/components
1. run browser, go into customize mode
2. click themes, then click one of the themes. This applies the theme (rather than just previewing it) and closes the popup.
3. click the themes button again, hover over the "Default" item.

ER:
see the result of using the default theme

AR:
no change from the applied lwt
Attachment #8518259 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #4)
> Comment on attachment 8518259 [details] [diff] [review]
> patch
> 
> Review of attachment 8518259 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Dão Gottwald [:dao] from comment #3)
> > Created attachment 8518259 [details] [diff] [review]
> > patch
> > 
> > Amazing how many bugs are layered on top of each other here.
> 
> I don't actually understand what you mean here.

I expected the previewTheme call for the default theme to be an oversight, but apparently it was expected to do something, which is even worse. (The other unexpected bug I ran into is that browser-devedition.js working around CustomizeMode.jsm's broken behavior is baked into browser-devedition.js.)

> In any case, this doesn't
> address comment 2.
> 
> To be clear, here's my STR:
> 
> 0. apply patch, rebuild browser/base, browser/components
> 1. run browser, go into customize mode
> 2. click themes, then click one of the themes. This applies the theme
> (rather than just previewing it) and closes the popup.
> 3. click the themes button again, hover over the "Default" item.
> 
> ER:
> see the result of using the default theme
> 
> AR:
> no change from the applied lwt

This is pretty much expected behavior. LightweightThemeManager.jsm doesn't intend to provide means for "previewing" something that isn't a lightweight theme.
(In reply to Dão Gottwald [:dao] from comment #5)
> (The
> other unexpected bug I ran into is that browser-devedition.js working around
> CustomizeMode.jsm's broken behavior is baked into browser-devedition.js.)

... baked into browser_devedition.js, I meant. I.e. the test counts on the tested code being broken.
Attachment #8518259 - Flags: review?(jaws)
Comment on attachment 8518259 [details] [diff] [review]
patch

(In reply to Dão Gottwald [:dao] from comment #5)
> (In reply to :Gijs Kruitbosch from comment #4)
> > Comment on attachment 8518259 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 8518259 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > (In reply to Dão Gottwald [:dao] from comment #3)
> > > Created attachment 8518259 [details] [diff] [review]
> > > patch
> > > 
> > > Amazing how many bugs are layered on top of each other here.
> > 
> > I don't actually understand what you mean here.
> 
> I expected the previewTheme call for the default theme to be an oversight,
> but apparently it was expected to do something, which is even worse.

Why? You've provided no rationale as to why previewing the default state when using a LWT is a bad thing.

> > In any case, this doesn't
> > address comment 2.
> > 
> > To be clear, here's my STR:
> > 
> > 0. apply patch, rebuild browser/base, browser/components
> > 1. run browser, go into customize mode
> > 2. click themes, then click one of the themes. This applies the theme
> > (rather than just previewing it) and closes the popup.
> > 3. click the themes button again, hover over the "Default" item.
> > 
> > ER:
> > see the result of using the default theme
> > 
> > AR:
> > no change from the applied lwt
> 
> This is pretty much expected behavior. LightweightThemeManager.jsm doesn't
> intend to provide means for "previewing" something that isn't a lightweight
> theme.

It's a regression from a user POV. That LWTManager doesn't (currently) "intend to provide" this doesn't mean that we shouldn't. In the list of themes, it makes no sense if all themes have hover previews, except the default. If the current way that the preview works is not "correct" from a code POV, then the patch should adjust LWTManager to provide a "correct" way to do this.
Attachment #8518259 - Flags: review-
(In reply to :Gijs Kruitbosch from comment #7)
> > I expected the previewTheme call for the default theme to be an oversight,
> > but apparently it was expected to do something, which is even worse.
> 
> Why? You've provided no rationale as to why previewing the default state
> when using a LWT is a bad thing.

Because it's a gross misuse of the API. It only kind of works accidentally.
(In reply to Dão Gottwald [:dao] from comment #8)
> ... It only kind of works accidentally.

What ways does it not work?
It's causing this bug, for instance, which browser-devedition.js had to work around.
(In reply to Dão Gottwald [:dao] from comment #10)
> It's causing this bug, for instance, which browser-devedition.js had to work
> around.

I don't think we should do this code cleanup without keeping the default-theme preview working though.
It's cleanup as far as browser-devedition.js is concerned, but the CustomizeMode.jsm part is just out of whack and could similarly confuse other observers of the lightweight theme notifications.
this might work, but I'm on the run and can't test this right now
Attachment #8519124 - Flags: review?(jaws)
Comment on attachment 8518259 [details] [diff] [review]
patch

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

I tested out the patch with devedition and it still has the regression where hovering over a default theme after a lightweight theme has been selected doesn't preview any theme.

However, I did come across another bug and I filed it as bug 1095797.
Attachment #8518259 - Flags: review?(jaws)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #14)
> Comment on attachment 8518259 [details] [diff] [review]
> patch
> 
> Review of attachment 8518259 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I tested out the patch

Which one?
Flags: needinfo?(jaws)
Comment on attachment 8519124 [details] [diff] [review]
patch that attempts to keep working the pseudo-preview for the default theme

(In reply to Dão Gottwald [:dao] from comment #15)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #14)
> > Comment on attachment 8518259 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 8518259 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I tested out the patch
> 
> Which one?

Huh, I could have sworn I was testing out the patch that you requested I review. But I double-checked and I was testing out the patch that Gijs had r-'d. With *this* patch applied, the preview for the default theme now works. Sorry about the earlier comment, and thank for the follow-up needinfo.
Flags: needinfo?(jaws)
Attachment #8519124 - Flags: review?(jaws) → review+
https://hg.mozilla.org/mozilla-central/rev/c30143c7a4cc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Comment on attachment 8519124 [details] [diff] [review]
patch that attempts to keep working the pseudo-preview for the default theme

Approval Request Comment
[Feature/regressing bug #]: bug 1007336
[User impact if declined]: faulty lightweight-theme-styling-update notification that might break add-ons
[Describe test coverage new/current, TBPL]: https://tbpl.mozilla.org/?tree=Fx-Team&rev=c30143c7a4cc
[Risks and why]: low
[String/UUID change made/needed]: none
Attachment #8519124 - Flags: approval-mozilla-beta?
Attachment #8519124 - Flags: approval-mozilla-aurora?
Comment on attachment 8519124 [details] [diff] [review]
patch that attempts to keep working the pseudo-preview for the default theme

I'm hesitant to take this change in beta8 (only one beta left to deal with fallout) on the chance that it will impact add-ons. If this was known to impact add-ons, that would be a stronger case. I'm going to mark beta- and ni Dao in case he wants to come back again today with a case to take this change in Beta.
Flags: needinfo?(dao)
Attachment #8519124 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
I don't have a throughout analysis and think we should take this patch opportunistically. However from a quick look it seems that this add-on is affected: https://addons.mozilla.org/en-US/firefox/addon/ymobactus/
Judging by this code: https://addons.mozilla.org/en-US/firefox/files/browse/253388/file/chrome/content/ymobactus.class.js#L213
Flags: needinfo?(dao)
Is there any way that this change will impact/break themes other than wrt add-ons?
Flags: needinfo?(dao)
I see no way how it could break lightweight themes. Full themes it doesn't affect at all.
Flags: needinfo?(dao)
Comment on attachment 8519124 [details] [diff] [review]
patch that attempts to keep working the pseudo-preview for the default theme

Taking this on Aurora at least, so we can get more user feedback (esp with the new bump thanks to DevEd).
Attachment #8519124 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8519124 [details] [diff] [review]
patch that attempts to keep working the pseudo-preview for the default theme

Reversing my earlier decision based on comment 21 and comment 23. Let's take this in beta8.
Attachment #8519124 - Flags: approval-mozilla-beta- → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.