1405720 - [macOS] [Photon] Regression Theme selector door hanger is showing two highlighted themes

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Mehmet]

Attachment: Bildschirmfoto 2017-10-04 um 17.56.22.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3228.0 Safari/537.36

Steps to reproduce:

macOS 10.12.6
58.0a1 (2017-10-04) (64-Bit)

1.) Go to Customizing
2.) Click on the Theme selector


Actual results:

The door hanger shows two selected/highlighted themes.


Expected results:

Only the actual theme should be highlighted.

Comment 1

[Johann Hofmann [:johannh]]

Hm, on Windows 7 it's showing no item highlighted. Weird. Can you get a regression window with mozregression, please?

Comment 2

[Mehmet]

Attachment: screencast.mov

Okay, I'll take a look to find the regression ...

Just a note, it seems, that it depends, which theme is selected. I also see with the Light and dark theme that nothing is selected. Please see the enclosed screencast.

Comment 3

[Mehmet]

(In reply to Johann Hofmann [:johannh] from comment #1)
> Hm, on Windows 7 it's showing no item highlighted. Weird. Can you get a
> regression window with mozregression, please?

Looks like your patch is the culprit: https://hg.mozilla.org/integration/autoland/rev/074089753394

Comment 4

[Johann Hofmann [:johannh]]

Gah, what? I can't see how that code would cause this. Maybe some race condition? I'll try to debug.

[Tracking Requested - why for this release]:
Horrible experience switching themes.

Comment 5

[Johann Hofmann [:johannh]]

Are you sure that this was caused by bug 1390885? Bug 1402981 landed around the same time and looks like a more realistic regressor. I'll try to local backout both patches.

Comment 6

[Johann Hofmann [:johannh]]

Ok my local backouts confirm that this is indeed regressed by bug 1402981.

Gijs, can you take a look?

Comment 7

[:Gijs (he/him)]

Can someone suggest steps to reproduce? The ones in comment #0 don't produce the issue in the summary for me on a clean profile (tested on OS X), so something's missing here.

Comment 8

[Johann Hofmann [:johannh]]

I can reproduce comment 2 in a clean profile on Beta. I currently can't reproduce "two themes selected at once", maybe Mehmet knows more about this.

Comment 9

[:Gijs (he/him)]

STR:

1. open clean profile
2. select the 'light' or 'dark' theme from customize mode theme selector
3. restart firefox
4. reopen customize mode theme selector and select another lightweight theme (not light or dark)
5. reopen the theme selector

Comment 10

[:Gijs (he/him)]

Attachment: Bug 1405720 - ensure only 1 theme is ever shown as selected in customize mode,

The previous code here always set the `isActive` property on all themes. When writing the
patch for bug 1402981 I ran into issues because the default theme has an `isActive` property
anyway (it's a different type of object). So I tried to avoid setting `isActive` if it was
already present. Unfortunately, the result was that `isActive` values, once set, weren't
correctly updated. Worse, these values are (and were, prior to bug 1402981) serialized into
prefs.

There's no point serializing these values, all that will happen is that they'll start
mismatching the 'real' state of the world (LightweightThemeManager.currentTheme). So instead,
let's just not set the `isActive` property at all, and rely solely on the ID of the current
theme (or the default theme's ID, now that we no longer support non-lightweight-themes) to
establish whether any of the themes should appear selected or not.

Review commit: https://reviewboard.mozilla.org/r/186790/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186790/

Comment 11

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

Comment on attachment 8915593 [details]
Bug 1405720 - ensure only 1 theme is ever shown as selected in customize mode,

https://reviewboard.mozilla.org/r/186790/#review191890

It would be nice to have a test for this

Comment 12

[:Gijs (he/him)]

Comment on attachment 8915593 [details]
Bug 1405720 - ensure only 1 theme is ever shown as selected in customize mode,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186790/diff/1-2/

Comment 13

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

Comment on attachment 8915593 [details]
Bug 1405720 - ensure only 1 theme is ever shown as selected in customize mode,

https://reviewboard.mozilla.org/r/186790/#review191914

Test changes look good, thanks!

Comment 14

[Johann Hofmann [:johannh]]

Comment on attachment 8915593 [details]
Bug 1405720 - ensure only 1 theme is ever shown as selected in customize mode,

https://reviewboard.mozilla.org/r/186790/#review191922

Seems good and works for me! Thank you!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4099fa94e3ba
ensure only 1 theme is ever shown as selected in customize mode, r=jaws,johannh

Comment 16

[:Gijs (he/him)]

Comment on attachment 8915593 [details]
Bug 1405720 - ensure only 1 theme is ever shown as selected in customize mode,

Approval Request Comment
[Feature/Bug causing the regression]: bug 1402981 (which was also uplifted to 57)
[User impact if declined]: the theme dropdown in customize mode shows either no selected theme, or the wrong selected theme, in some cases.
[Is this code covered by automated tests?]: I said yes to this before, but clearly there weren't enough tests... I added some more in this change to make sure we don't break this again.
[Has the fix been verified in Nightly?]: not yet, but it's had 2 separate reviews including checks to see this is fixed, so I'm reasonably confident.
[Needs manual test from QE? If yes, steps to reproduce]: would be helpful. See comment 0 (to see 0 themes selected, when done on a clean profile) and comment 9 (to see 2 themes selected)
[List of other uplifts needed for the feature/fix]: nope
[Is the change risky?]: not really
[Why is the change risky/not risky?]: we're continuing to manipulate the picker we were manipulating before, so it's a small patch, we still have opportunities to find more issues and back all of it out if it comes to that, but with the extra reviews and the additional test coverage I'm reasonably confident this will be the end of things.
[String changes made/needed]: nope.

Comment 17

[Marcia Knous [:marcia]]

Tracking 57+ for this visible regression. We should also have QA verify the fix following the landing.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4099fa94e3ba

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8915593 [details]
Bug 1405720 - ensure only 1 theme is ever shown as selected in customize mode,

Recent regression in Photon code, Bet57+

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6081f387494b

Comment 21

[Ciprian Georgiu, Desktop QA]

I have repro this issue using STR from comment 2 and comment 9 using an affected Nightly (2017-10-04).

This is not reproducible anymore on Beta 57.0b7 and latest Nightly 58.0a1 under Windows 10 x64, Mac OS X 10.11 and Ubuntu 16.04 x64.