1402981 - Dark and light themes should always be in the themes list in customize mode

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

[Aaron Benson]

Attachment: Screen Shot 2017-09-25 at 5.11.04 PM.png

Shorlander says the items in this list are sorted by recency, but our dark and light themes should always be shown in the top two positions.

Comment 1

[:Gijs (he/him)]

(In reply to Aaron Benson from comment #0)
> Created attachment 8911999 [details]
> Screen Shot 2017-09-25 at 5.11.04 PM.png
> 
> Shorlander says the items in this list are sorted by recency, but our dark
> and light themes should always be shown in the top two positions.

I mean, I assume this includes the default theme as well such that we take up 3 of the 7 positions, right?

Comment 2

[:Gijs (he/him)]

(In reply to :Gijs from comment #1)
> (In reply to Aaron Benson from comment #0)
> > Created attachment 8911999 [details]
> > Screen Shot 2017-09-25 at 5.11.04 PM.png
> > 
> > Shorlander says the items in this list are sorted by recency, but our dark
> > and light themes should always be shown in the top two positions.
> 
> I mean, I assume this includes the default theme as well such that we take
> up 3 of the 7 positions, right?

Aaron says yes.

Comment 3

[:Gijs (he/him)]

Attachment: Bug 1402981 - add light and dark themes to the list of themes in customize mode by default,

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

Comment 4

[:Gijs (he/him)]

Comment on attachment 8912939 [details]
Bug 1402981 - add light and dark themes to the list of themes in customize mode by default,

https://reviewboard.mozilla.org/r/184262/#review189692

::: browser/components/customizableui/CustomizeMode.jsm:1417
(Diff revision 1)
> +          // If finding strings for this failed, just don't build it. This can
> +          // happen for users with 'older' recommended themes lists, some of which
> +          // have since been removed from Firefox.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1323833#c23 for context.

Comment 5

[(Away until May 24) Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8912939 [details]
Bug 1402981 - add light and dark themes to the list of themes in customize mode by default,

https://reviewboard.mozilla.org/r/184262/#review190136

::: browser/components/customizableui/CustomizeMode.jsm:1417
(Diff revision 1)
> +          // If finding strings for this failed, just don't build it. This can
> +          // happen for users with 'older' recommended themes lists, some of which
> +          // have since been removed from Firefox.

Do we know if the strings will exist for users who have one of these 'older' recommended themes as their current theme?

Comment 6

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Comment on attachment 8912939 [details]
> Bug 1402981 - add light and dark themes to the list of themes in customize
> mode by default,
> 
> https://reviewboard.mozilla.org/r/184262/#review190136
> 
> ::: browser/components/customizableui/CustomizeMode.jsm:1417
> (Diff revision 1)
> > +          // If finding strings for this failed, just don't build it. This can
> > +          // happen for users with 'older' recommended themes lists, some of which
> > +          // have since been removed from Firefox.
> 
> Do we know if the strings will exist for users who have one of these 'older'
> recommended themes as their current theme?

So what happens is:

1) user uses Firefox pre-bug 1323833
2) user uses one of the recommended themes. This saves a modified copy of the recommended themes list to their profile
3) user uses Firefox post-bug 1323833.

In bug 1323833, we removed some of the recommended themes, and their strings. If the user had one of the removed recommended themes in their list of themes in that pref, we still try to show it here. These string fetches now fail, because the strings are gone: https://hg.mozilla.org/mozilla-central/rev/06bced347abc#l2.12 .

Does that help clarify why we need a try...catch?

Comment 7

[:Gijs (he/him)]

Chatted with Jared on IRC, we're good.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed2a1dff7a2e
add light and dark themes to the list of themes in customize mode by default, r=jaws

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 10

[:Gijs (he/him)]

Comment on attachment 8912939 [details]
Bug 1402981 - add light and dark themes to the list of themes in customize mode by default,

Approval Request Comment
[Feature/Bug causing the regression]: the dark/light themes are more important and polished than before with photon, and we need to make sure users have easy access to them
[User impact if declined]: the dark/light themes can be 'lost' in the themes picker in customize mode
[Is this code covered by automated tests?]: yep
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: 
1. open AMO on a clean profile
2. install (not preview) 7 lightweight themes, one after the other.
3. open customize mode
4. open the theme picker in customize mode

Expected:
default, light and dark theme are all visible in the picker
Actual (pre-patch):
only the default theme is visible in the picker

[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: it's a pretty straightforward code change, and we have good automated test coverage and still quite a lot of beta time.

The only minor risk here is that I'm not sure what the current state of the developer edition is. I don't expect problems with the feature or the patch, but if we run automated tests against the devedition beta builds it's possible the expectations of the automated test aren't valid (because devedition uses the dark theme by default). I don't know how to check if this is the case, though, short of trying to uplift (that is, I'm not aware of how to run test builds on try that produce 'devedition' builds and tests either locally or on try). And maybe it's fine - just something for which to keep an eye out when uplifting. If this does break, it should be a 1 or 2-line change to the automated test to fix it.

[String changes made/needed]: none

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8912939 [details]
Bug 1402981 - add light and dark themes to the list of themes in customize mode by default,

Let's take it now and see what happens with devedition in the CI.
Should be in 57b5

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0ceab1767911