Closed Bug 1060623 Opened 6 years ago Closed 6 years ago

Theme switcher in customize mode can't handle many themes

Categories

(Firefox :: Toolbars and Customization, defect)

x86
All
defect
Not set
normal
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 35
Iteration:
35.2
Tracking Status
firefox34 + verified
firefox35 --- verified

People

(Reporter: phlsa, Assigned: dao)

References

Details

Attachments

(2 files)

Attached image 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.
Flags: firefox-backlog+
[Tracking Requested - why for this release]: Shouldn't ship this. :-)
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).
(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.
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
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.
(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.
LightweightThemeManager.usedThemes should be in recently-used order.
Assignee: nobody → dao
Status: NEW → ASSIGNED
Iteration: --- → 35.2
Points: --- → 5
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.
Attached patch patchSplinter Review
Attachment #8490929 - Flags: review?(jaws)
Version: 33 Branch → Trunk
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.
Attachment #8490929 - Flags: review?(jaws) → review+
https://hg.mozilla.org/mozilla-central/rev/2bebc67aa52c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
QA Contact: cornel.ionce
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.
Status: RESOLVED → VERIFIED
This has been verified on m-c and has baked for several days. Can you please request Aurora uplift approval?
Flags: needinfo?(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
Attachment #8490929 - Flags: approval-mozilla-aurora?
Flags: needinfo?(dao)
Comment on attachment 8490929 [details] [diff] [review]
patch

Aurora+
Attachment #8490929 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
You need to log in before you can comment on or make changes to this bug.