Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Trim the list of recommended themes from 5 to 3

VERIFIED FIXED in Firefox 53

Status

()

Firefox
Toolbars and Customization
VERIFIED FIXED
7 months ago
2 months ago

People

(Reporter: canuckistani, Assigned: bgrins)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 53
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox53 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

The list in the customization view is getting a little long with the addition of compact themes and 5 recommended themes. Let's cut it to a top 3, sourced from AMO. It seems like we've just hard-coded 5 specific themes here. 

Happy to just hard code 3 of them for now.
(Reporter)

Updated

7 months ago
Blocks: 1314091
Created attachment 8819511 [details]
Current recommended themes

Top 20 lightweight themes based on the current theme reported to FHR in longitudinal: https://sql.telemetry.mozilla.org/queries/313
percent	persona
=======	============
98.680	none
0.083	recommended-2						Space Fantasy
0.061	firefox-devedition@mozilla.org
0.045	https://addons.mozilla.org/en-US/firefox/addon/18066	Dark Fox
0.021	recommended-4						Pastel Gradient
0.021	https://addons.mozilla.org/en-US/firefox/addon/60179	little flowers
0.020	recommended-1						a web browser renaissance
0.014	https://addons.mozilla.org/en-US/firefox/addon/381047	(Not on AMO anymore?)
0.011	recommended-5						Carbon light
0.011	https://addons.mozilla.org/en-US/firefox/addon/165163	Three Wolf Moon Shirt
0.009	recommended-3						Linen Light
0.009	https://addons.mozilla.org/en-US/firefox/addon/64769	Rainy day 2 (disabled by an AMO administrator)
0.009	https://addons.mozilla.org/en-US/firefox/addon/15131	Groovy Blue
0.007	https://addons.mozilla.org/en-US/firefox/addon/7610	Youlicit Toolbar
0.006	https://addons.mozilla.org/en-US/firefox/addon/25991	FIR3FOX
0.006	https://addons.mozilla.org/en-US/firefox/addon/80276	The Good Shephered
0.006	https://addons.mozilla.org/en-US/firefox/addon/46852	Sunset Over Water by MaDonna
0.006	https://addons.mozilla.org/en-US/firefox/addon/171543	Stylus Blue
0.006	https://addons.mozilla.org/en-US/firefox/addon/143671	Firefox Carbon
0.006	https://addons.mozilla.org/en-US/firefox/addon/24500	Glass - Black

As a sanity check, I compared the data to https://addons.mozilla.org/en-US/firefox/themes/?sort=popular and it seems to be in the same relative order for the top 3 listed ones but then differs a bit.

Updated

7 months ago
Component: Theme → Toolbars and Customization
(Assignee)

Comment 2

6 months ago
Based on Matt's analysis I'm thinking we keep recommended-2, recommended-4, and recommended-1 in the pref (possibly reordering?).

We'd delete the others from the pref but make sure the assets are still in tree so that if you previously installed it then it'd still work. Need to do some testing to confirm it still works even when not present in the pref.
(Assignee)

Comment 3

6 months ago
Peter, any feedback on Comment 2?  This would mean "Space Fantasy", "Pastel Gradient", and "A web browser renaissance" would stay while "Linen Light" and "Carbon Light" would go.
Flags: needinfo?(pdolanjski)
(In reply to Brian Grinstead [:bgrins] from comment #3)
> Peter, any feedback on Comment 2?  This would mean "Space Fantasy", "Pastel
> Gradient", and "A web browser renaissance" would stay while "Linen Light"
> and "Carbon Light" would go.

I would say that generally makes sense, but the only question I have is why not "little flowers" instead of "A web browser renaissance" if it is, indeed, more popular?
Flags: needinfo?(pdolanjski)
(Assignee)

Comment 5

6 months ago
(In reply to Peter Dolanjski [:pdol] from comment #4)
> (In reply to Brian Grinstead [:bgrins] from comment #3)
> > Peter, any feedback on Comment 2?  This would mean "Space Fantasy", "Pastel
> > Gradient", and "A web browser renaissance" would stay while "Linen Light"
> > and "Carbon Light" would go.
> 
> I would say that generally makes sense, but the only question I have is why
> not "little flowers" instead of "A web browser renaissance" if it is,
> indeed, more popular?

The recommended themes are different than "Dark Fox" / "little flowers" since they are shipped with the browser and show up in Customize Mode by default.  It may not be a bad idea to reach out to authors of top lwts about somehow surfacing them, but for this bug the idea is to remove 2 of the existing recommended themes.
(Assignee)

Comment 6

6 months ago
From local testing, it seems OK to remove the recommended themes from the pref even for people who have the theme applied already.  Steps I used to test:

./mach build && ./mach run --profile /tmp/test-themes
Go to customize mode and select "Carbon Light"
Apply patch that removes "Linen Light" and "Carbon Light"
./mach build && ./mach run --profile /tmp/test-themes

In this case, the "Carbon Light" theme is still applied.  I think what happens is that the lightweightThemes.recommendedThemes pref becomes modified once a theme is chosen from customize mode (it removes the selected theme from recommendedThemes), which means that the default pref value change doesn't affect this user.

So, anyone who has selected a recommended theme (or maybe even opened the menu in customize mode [0]) will continue to see all 5 recommended along with compact themes.  For people who haven't entered customize mode they will see only the 3 recommended along with compact themes.

[0]: https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizeMode.jsm#1442
(Assignee)

Comment 7

6 months ago
Created attachment 8828084 [details]
clean-profile-with-patch.png

This is what it will look like for users who haven't applied a theme through customize mode before
(Assignee)

Comment 8

6 months ago
Created attachment 8828086 [details]
had-applied-a-removed-recommended-theme-previously.png

This is what it will look like for someone who had already applied one of the recommended themes being removed.  It'll look basically the same for someone who had applied one of the recommended themes that are staying, except "Carbon Light" will swap positions with whatever is applied
Comment hidden (mozreview-request)
(Assignee)

Comment 10

6 months ago
Gijs I see you reviewed relevant work in Bug 1007336 so I flagged you for review, but feel free to redirect
(Assignee)

Comment 11

6 months ago
(In reply to Brian Grinstead [:bgrins] from comment #8)
> Created attachment 8828086 [details]
> had-applied-a-removed-recommended-theme-previously.png
> 
> This is what it will look like for someone who had already applied one of
> the recommended themes being removed.  It'll look basically the same for
> someone who had applied one of the recommended themes that are staying,
> except "Carbon Light" will swap positions with whatever is applied

Since there will still be cases where people see all the current recommended themes and the compact themes it might make sense to also tighten up the padding among these popup items to make it less likely to overflow the browser window and cause bad panel behavior.  That could be done in a follow-up if so.
(Assignee)

Updated

6 months ago
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED

Comment 12

6 months ago
(In reply to Brian Grinstead [:bgrins] from comment #6)
> So, anyone who has selected a recommended theme (or maybe even opened the
> menu in customize mode [0]) will continue to see all 5 recommended along
> with compact themes.  For people who haven't entered customize mode they
> will see only the 3 recommended along with compact themes.
> 
> [0]:
> https://dxr.mozilla.org/mozilla-central/source/browser/components/
> customizableui/CustomizeMode.jsm#1442

That's an oncommand handler, so it'll only happen if you select the theme from customize mode.

Comment 13

6 months ago
mozreview-review
Comment on attachment 8828089 [details]
Bug 1323833 - Trim the list of recommended themes from 5 to 3;

https://reviewboard.mozilla.org/r/105604/#review106616

r=me BUT ... can we remove the now-unused strings from https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/lightweightThemes.properties#5 ? Or does that break displaying these themes for users that will still see them? From code inspection, it *looks* to me like those should be OK, because once set we will have saved these names/descriptions into the other pref and they will persist without us having the strings, but it'd be good to check that that's really true. :-)
Attachment #8828089 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 14

6 months ago
(In reply to Brian Grinstead [:bgrins] from comment #11)
> (In reply to Brian Grinstead [:bgrins] from comment #8)
> > Created attachment 8828086 [details]
> > had-applied-a-removed-recommended-theme-previously.png
> > 
> > This is what it will look like for someone who had already applied one of
> > the recommended themes being removed.  It'll look basically the same for
> > someone who had applied one of the recommended themes that are staying,
> > except "Carbon Light" will swap positions with whatever is applied
> 
> Since there will still be cases where people see all the current recommended
> themes and the compact themes it might make sense to also tighten up the
> padding among these popup items to make it less likely to overflow the
> browser window and cause bad panel behavior.  That could be done in a
> follow-up if so.

Can you file and needinfo someone from UX about this? I'm assuming someone's been involved with the changes to compact themes, hopefully they have cycles to give some advice here. :-)
Flags: needinfo?(bgrinstead)

Updated

6 months ago
Depends on: 1332237
(In reply to :Gijs from comment #14)
> (In reply to Brian Grinstead [:bgrins] from comment #11)
> > (In reply to Brian Grinstead [:bgrins] from comment #8)
> > > Created attachment 8828086 [details]
> > > had-applied-a-removed-recommended-theme-previously.png
> > > 
> > > This is what it will look like for someone who had already applied one of
> > > the recommended themes being removed.  It'll look basically the same for
> > > someone who had applied one of the recommended themes that are staying,
> > > except "Carbon Light" will swap positions with whatever is applied
> > 
> > Since there will still be cases where people see all the current recommended
> > themes and the compact themes it might make sense to also tighten up the
> > padding among these popup items to make it less likely to overflow the
> > browser window and cause bad panel behavior.  That could be done in a
> > follow-up if so.
> 
> Can you file and needinfo someone from UX about this? I'm assuming someone's
> been involved with the changes to compact themes, hopefully they have cycles
> to give some advice here. :-)

The "My Themes" list is limited to five entries plus the default theme. It's always been pretty easy to hit that limit while also seeing the five recommended themes, so I'd hope that we handle that case reasonably and don't need to change anything for the addition of the compact themes.
(Assignee)

Comment 16

6 months ago
(In reply to Dão Gottwald [:dao] from comment #15)
> (In reply to :Gijs from comment #14)
> > (In reply to Brian Grinstead [:bgrins] from comment #11)
> > > (In reply to Brian Grinstead [:bgrins] from comment #8)
> > > > Created attachment 8828086 [details]
> > > > had-applied-a-removed-recommended-theme-previously.png
> > > > 
> > > > This is what it will look like for someone who had already applied one of
> > > > the recommended themes being removed.  It'll look basically the same for
> > > > someone who had applied one of the recommended themes that are staying,
> > > > except "Carbon Light" will swap positions with whatever is applied
> > > 
> > > Since there will still be cases where people see all the current recommended
> > > themes and the compact themes it might make sense to also tighten up the
> > > padding among these popup items to make it less likely to overflow the
> > > browser window and cause bad panel behavior.  That could be done in a
> > > follow-up if so.
> > 
> > Can you file and needinfo someone from UX about this? I'm assuming someone's
> > been involved with the changes to compact themes, hopefully they have cycles
> > to give some advice here. :-)
> 
> The "My Themes" list is limited to five entries plus the default theme. It's
> always been pretty easy to hit that limit while also seeing the five
> recommended themes, so I'd hope that we handle that case reasonably and
> don't need to change anything for the addition of the compact themes.

At least in my case it's filling up the vertical space available in the window with a couple of lw themes installed.  On top of it not looking great, the popup begins to behave very badly once that happens (the mouse events don't seem to fire on the correct item once it's overflowing the window).  I filed 1332406 so we can discuss further there
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 17

6 months ago
(In reply to :Gijs from comment #13)
> Comment on attachment 8828089 [details]
> Bug 1323833 - Trim the list of recommended themes from 5 to 3;
> 
> https://reviewboard.mozilla.org/r/105604/#review106616
> 
> r=me BUT ... can we remove the now-unused strings from
> https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/
> browser/lightweightThemes.properties#5 ? Or does that break displaying these
> themes for users that will still see them? From code inspection, it *looks*
> to me like those should be OK, because once set we will have saved these
> names/descriptions into the other pref and they will persist without us
> having the strings, but it'd be good to check that that's really true. :-)

I've tested this scenario and I still see the strings properly in both addons manager and customize mode, so I'll remove the strings
Comment hidden (mozreview-request)

Comment 19

6 months ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/06bced347abc
Trim the list of recommended themes from 5 to 3;r=Gijs
https://hg.mozilla.org/mozilla-central/rev/06bced347abc
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
(Assignee)

Updated

6 months ago
Flags: qe-verify+

Comment 21

6 months ago
I have reproduced this bug with Nightly 53.0a1 (2016-12-15) on Windows 8.1(64-bit).

This bug's fix is verified on Aurora 53.0a2.

Build ID : 20170126004004
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0

[bugday-20170125]

Updated

6 months ago
Status: RESOLVED → VERIFIED

Comment 22

6 months ago
Thanks!
status-firefox53: fixed → verified

Updated

2 months ago
Blocks: 1363853
You need to log in before you can comment on or make changes to this bug.