1323833 - Trim the list of recommended themes from 5 to 3

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Jeff Griffiths (:canuckistani) (:⚡︎)]

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.

Comment 1

[Matthew N. [:MattN]]

Attachment: 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	[e-mail]
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.

Comment 2

[Brian Grinstead [:bgrins]]

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.

Comment 3

[Brian Grinstead [:bgrins]]

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.

Comment 4

[Peter Dolanjski [:pdol]]

(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?

Comment 5

[Brian Grinstead [:bgrins]]

(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.

Comment 6

[Brian Grinstead [:bgrins]]

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

Comment 7

[Brian Grinstead [:bgrins]]

Attachment: clean-profile-with-patch.png

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

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: 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 9

[Brian Grinstead [:bgrins]]

Attachment: Bug 1323833 - Trim the list of recommended themes from 5 to 3;

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

Comment 10

[Brian Grinstead [:bgrins]]

Gijs I see you reviewed relevant work in Bug 1007336 so I flagged you for review, but feel free to redirect

Comment 11

[Brian Grinstead [:bgrins]]

(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.

Comment 12

[:Gijs (he/him)]

(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

[:Gijs (he/him)]

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. :-)

Comment 14

[:Gijs (he/him)]

(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. :-)

Comment 15

[Dão Gottwald [:dao]]

(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.

Comment 16

[Brian Grinstead [:bgrins]]

(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

Comment 17

[Brian Grinstead [:bgrins]]

(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 18

[Brian Grinstead [:bgrins]]

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

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06bced347abc
Trim the list of recommended themes from 5 to 3;r=Gijs

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/06bced347abc

Comment 21

[Azmina]

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]

Comment 22

[:Gijs (he/him)]

Thanks!

Comment 23

[:Gijs (he/him)]

Unfortunately, the string removal here broke these lookups:

https://dxr.mozilla.org/mozilla-central/rev/7d15bc419c6cd7e9f3b4d41370c3b0e5990c8d1b/browser/components/customizableui/CustomizeMode.jsm#1403-1404

which broke recommended themes for people whose list still included the old ones (the prefs get customized based on using those themes). I'll try to fix in bug 1402981.