1060623 - Theme switcher in customize mode can't handle many themes

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Screenshot of overflow

When lots of themes are installed, the theme switcher bubble in customize mode grows out of the screen, making some of the items impossible to access.

Instead it should become scrollable at a certain point.

Comment 1

[:Gijs (he/him)]

[Tracking Requested - why for this release]: Shouldn't ship this. :-)

Comment 2

[jaramat]

Long lists are never nice. Instead of becoming scrollable, the bubble should either pick five-or-so themes at random (or by times used) and just don't display the rest (or display a "...and {n} more" ellipsis button).

Comment 3

[:Gijs (he/him)]

(In reply to jaramat from comment #2)
> Long lists are never nice. Instead of becoming scrollable, the bubble should
> either pick five-or-so themes at random

Really? That's not very user-friendly. How do we justify the random selection? If people don't want the list to scroll, they can uninstall some themes by clicking "Manage".

> (or by times used)

We don't have this information (and its definition is unclear - sessions? session length? number of times you switch to it?), and starting to store it would be tracking of a proportion that is entirely unnecessary for the feature at hand.

> and just don't
> display the rest (or display a "...and {n} more" ellipsis button).

We have a manage button for this already - I think we should go with the scrolling list, and if people find it jarring they can click that option and remove some themes.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I admit I agree with comment 2

(In reply to :Gijs Kruitbosch from comment #3)
> We don't have this information (and its definition is unclear - sessions?
> session length? number of times you switch to it?)

the last 5 used themes would work nicely.

> We have a manage button for this already - I think we should go with the
> scrolling list, and if people find it jarring they can click that option and
> remove some themes.

There's the themes section in the addons manager, this UI should stay simple and not try to duplicate that functionality imo.

Comment 5

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #4)
> I admit I agree with comment 2
> 
> (In reply to :Gijs Kruitbosch from comment #3)
> > We don't have this information (and its definition is unclear - sessions?
> > session length? number of times you switch to it?)
> 
> the last 5 used themes would work nicely.

Again, I'm 99% sure we don't have that information, and I don't think keeping it around just to have a nicer list here is worth doing. I can already see all the extra code in the add-on manager code that we'd need to keep track of it, plus dealing with things like safe mode, not counting the instant previews, not keeping track if you're in permanent private browsing mode, etc. etc.

It just seems way overkill in order to have a non-scrolling UI for people who install >5 themes, which is already going to be a tiny section of our userbase.

Comment 6

[Dão Gottwald [:dao]]

LightweightThemeManager.usedThemes should be in recently-used order.

Comment 7

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

On second thought, I agree that we should keep this simple.
So if possible, let's show the five most recently used themes here.

Also (and this might be happening already), the default theme should be sticky and always be shown in the same spot so that people have an easy path back.

Comment 8

[Dão Gottwald [:dao]]

Attachment: patch

Comment 9

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

Comment on attachment 8490929 [details] [diff] [review]
patch

Review of attachment 8490929 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/CustomizeMode.jsm
@@ +1308,1 @@
>        let doc = this.window.document;

Please move this line above `function buildToolbarButton(aTheme) {` so it is move obvious that `doc` is closed over.

Comment 10

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/2bebc67aa52c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2bebc67aa52c

Comment 12

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:35.0) Gecko/20100101 Firefox/35.0
Mozilla/5.0 (X11; Linux i686; rv:35.0) Gecko/20100101 Firefox/35.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0

Verified fixed using Nightly 35.0a1, build ID: 20140918030202.

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

This has been verified on m-c and has baked for several days. Can you please request Aurora uplift approval?

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 8490929 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1007336
[User impact if declined]: see comment 0
[Describe test coverage new/current, TBPL]: bug 1007336 landed with a basic test which still passes with this change
[Risks and why]: isolated fix, low risk
[String/UUID change made/needed]: none

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8490929 [details] [diff] [review]
patch

Aurora+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8d06ed011aaf

Comment 17

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:34.0) Gecko/20100101 Firefox/34.0
Mozilla/5.0 (X11; Linux i686; rv:34.0) Gecko/20100101 Firefox/34.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0

Also verified using latest Aurora, build ID: 20140930004006.