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

RESOLVED FIXED in Firefox 34

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bgrins, Assigned: dao)

Tracking

Trunk
Firefox 36
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox34 fixed, firefox35 fixed, firefox36 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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
(Reporter)

Updated

4 years ago
Component: Theme → Toolbars and Customization
(Assignee)

Updated

4 years ago
Blocks: 1007336
Points: --- → 2
Flags: firefox-backlog+
(Assignee)

Updated

4 years ago
Blocks: 1053434
(Assignee)

Comment 1

4 years ago
Created attachment 8518207 [details] [diff] [review]
patch
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #8518207 - Flags: review?(jaws)
(Assignee)

Updated

4 years ago
Iteration: --- → 36.2
Flags: qe-verify-

Comment 2

4 years ago
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-
(Assignee)

Comment 3

4 years ago
Created attachment 8518259 [details] [diff] [review]
patch

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 4

4 years ago
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)
(Assignee)

Comment 5

4 years ago
(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.
(Assignee)

Comment 6

4 years ago
(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.
(Assignee)

Updated

4 years ago
Attachment #8518259 - Flags: review?(jaws)

Comment 7

4 years ago
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-
(Assignee)

Comment 8

4 years ago
(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?
(Assignee)

Comment 10

4 years ago
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.
(Assignee)

Comment 12

4 years ago
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.
(Assignee)

Comment 13

4 years ago
Created attachment 8519124 [details] [diff] [review]
patch that attempts to keep working the pseudo-preview for the default theme

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)
(Assignee)

Comment 15

4 years ago
(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?
(Assignee)

Updated

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
(Assignee)

Comment 19

4 years ago
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?
status-firefox34: --- → affected
status-firefox35: --- → affected
status-firefox36: --- → fixed
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-
(Assignee)

Comment 21

4 years ago
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)
(Assignee)

Comment 23

4 years ago
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.