885066 - lightweight themes should not get applied to windows already in customization mode

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

lightweight theme applied in customization mode

this should have been fixed by https://bugzilla.mozilla.org/show_bug.cgi?id=870602 but maybe it doesn't work on XP or the build that was used to test was too old. needs to be verified.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

from ui-review etherpad,

this is because the harness applied the LWT after entering (see the order in the filename). The problem shouldn't occur when run with the order reversed. Note that this would be a problem if we ever allow customizing LWT from within customization mode.

Comment 2

[Matthew N. [:MattN]]

Yeah, that was my comment. The arguments to mozscreenshot were not in the right order to match how a user would hit this unless we allow enabling LWT from customization mode in the future. Probably fine to WONTFIX for now and remember to put "LightweighTheme" before "CustomizeMode" when using mozscreenshots.

Comment 3

[Dão Gottwald [:dao]]

(In reply to Matthew N. [:MattN] from comment #2)
> Yeah, that was my comment. The arguments to mozscreenshot were not in the
> right order to match how a user would hit this unless we allow enabling LWT
> from customization mode in the future.

It's quite easy to do manually if you have multiple windows.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Hm, this lightweight theme business looks like it's going to add quite a bit of complexity and ugliness in various parts. Is there a pre-existing way to disable lwthemes on a per-window basis?

Comment 6

[:Gijs (he/him)]

Attachment: don't apply LWTs to windows already in customization mode,

This WFM.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Jared Wein [:jaws] from comment #1)
> Note that this would be a problem if we ever allow customizing LWT from
> within customization mode.

If/when we ever do that, I think we'll need to figure out how to best *show* the LWT.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Dão Gottwald [:dao] from comment #3)
> It's quite easy to do manually if you have multiple windows.

Hm, this brings up an important issue: If you do something that would trigger this bug, and we fix this bug, then suddenly you're left with enabling a lightweight theme and it will look like it's not working in any window.

So, in light of that, I think bug 879994 should really have been a change to LightweightThemeConsumer - so it can be disabled per-window (à la comment 4). Would you mind making that change here too?

Will need to port bug 870602 too, to use LightweightThemeConsumer  (document.documentElement._lightweightTheme).

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 819755 [details] [diff] [review]
don't apply LWTs to windows already in customization mode,

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

As per comment 8.

Comment 10

[:Gijs (he/him)]

Attachment: don't apply LWTs to windows already in customization mode,

Alright, this keeps the LWT in the other windows. I'll do a straight backout of the temporary toggle thing (including its test) in a separate patch in a second.

Comment 11

[:Gijs (he/him)]

Attachment: Back out the original fix in bug 879994,

Backout patch.

Comment 12

[:Gijs (he/him)]

https://hg.mozilla.org/projects/ux/rev/81ef9027114f

Comment 13

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #12)
> https://hg.mozilla.org/projects/ux/rev/81ef9027114f

Uh, so that was the backout. The patch itself went in as https://hg.mozilla.org/projects/ux/rev/f4ec7c4100e2 . I blame mcMerge not dealing with this correctly for bugspam.

Comment 14

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/f4ec7c4100e2