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

RESOLVED FIXED in Firefox 57

Status

()

P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: abenson, Assigned: Gijs)

Tracking

57 Branch
Firefox 58
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox57 fixed, firefox58 fixed)

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(2 attachments)

(Reporter)

Description

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

Updated

2 years ago
Whiteboard: [photon-structure] → [photon-structure][triage]
(Assignee)

Comment 1

2 years ago
(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?
Flags: needinfo?(abenson)
(Assignee)

Updated

2 years ago
status-firefox57: --- → affected
status-firefox58: --- → affected
Flags: in-testsuite?
Priority: -- → P3
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
(Assignee)

Updated

2 years ago
Flags: qe-verify-
(Assignee)

Comment 2

2 years ago
(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.
Flags: needinfo?(abenson)
(Assignee)

Updated

2 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Priority: P3 → P1
(Assignee)

Updated

2 years ago
Blocks: 1323833
(Assignee)

Comment 4

2 years ago
mozreview-review
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 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?
Attachment #8912939 - Flags: review?(jaws) → review+
(Assignee)

Comment 6

2 years ago
(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?
Flags: needinfo?(jaws)
(Assignee)

Comment 7

2 years ago
Chatted with Jared on IRC, we're good.
Flags: needinfo?(jaws)

Comment 8

2 years ago
Pushed by gijskruitbosch@gmail.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ed2a1dff7a2e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
(Assignee)

Comment 10

2 years ago
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
Attachment #8912939 - Flags: approval-mozilla-beta?
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
Attachment #8912939 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 12

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/0ceab1767911
status-firefox57: affected → fixed
Flags: in-testsuite? → in-testsuite+
Depends on: 1405720
(Assignee)

Updated

a year ago
Duplicate of this bug: 1351666
You need to log in before you can comment on or make changes to this bug.