Last Comment Bug 573329 - Drop support for text and icons+text toolbar modes in the main browser window
: Drop support for text and icons+text toolbar modes in the main browser window
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Toolbars and Customization (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Firefox 28
Assigned To: Jared Wein [:jaws] (please needinfo? me)
:
Mentors:
: 835956 (view as bug list)
Depends on: 825852 880421 881385 925338
Blocks: 559033 810028 860814 863299 864355
  Show dependency treegraph
 
Reported: 2010-06-20 02:43 PDT by Markus Stange [:mstange]
Modified: 2016-04-25 11:20 PDT (History)
26 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Simple patch that hides non-icon modes (9.81 KB, patch)
2012-12-19 16:35 PST, Jared Wein [:jaws] (please needinfo? me)
gavin.sharp: feedback+
Details | Diff | Splinter Review
Moved migration code to _migrateUI (10.88 KB, patch)
2012-12-19 17:09 PST, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch v1.2 (11.77 KB, patch)
2012-12-26 14:46 PST, Jared Wein [:jaws] (please needinfo? me)
gavin.sharp: review-
Details | Diff | Splinter Review
Patch v1.3 (29.09 KB, patch)
2013-01-09 12:40 PST, Jared Wein [:jaws] (please needinfo? me)
mconley: review+
Details | Diff | Splinter Review
Patch for checkin to UX branch (28.94 KB, patch)
2013-01-15 13:33 PST, Jared Wein [:jaws] (please needinfo? me)
jaws: review+
Details | Diff | Splinter Review
Remove small icons and remove any references that were checking for different modes or iconsizes (part 2) (36.05 KB, patch)
2013-01-29 22:50 PST, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Rebased patch for UX branch (24.63 KB, patch)
2013-04-17 09:47 PDT, Jared Wein [:jaws] (please needinfo? me)
jaws: review+
Details | Diff | Splinter Review
Part 2, remove small icons mode (34.49 KB, patch)
2013-04-17 10:22 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Rebased on top of 748894 (26.26 KB, patch)
2013-04-23 14:58 PDT, Jared Wein [:jaws] (please needinfo? me)
jaws: review+
Details | Diff | Splinter Review

Description Markus Stange [:mstange] 2010-06-20 02:43:21 PDT
As discussed in bug 559033 starting around bug 559033 comment 10. It's not completely clear we want to do this yet, as far as I can tell.

We need this questions answered from UX:
 - Should we do this at all?
 - Should we drop support for text-only mode, too?

Dão, do you want to create the patch?
Comment 1 Dão Gottwald [:dao] 2010-06-20 10:40:59 PDT
I'm not yet convinced this would be a wise move, see bug 559033 comment 35. I guess we should drop it if it really gets in our way, like in a redesign of the customization UI. Right now I don't really see the need. Granted, getting rid of the mode switch would simplify the current customization UI, but only marginally.
Comment 2 Jared Wein [:jaws] (please needinfo? me) 2012-12-19 16:35:58 PST
Created attachment 694150 [details] [diff] [review]
Simple patch that hides non-icon modes

As we discussed on IRC today, this patch doesn't actually remove the ability for text on Toolkit toolbarbuttons, but does hide the ability in Firefox.

I still need to figure out if my migration code will conflict with retrieveToolbarIconsizesFromTheme.

Tested on Windows 7 and Ubuntu.
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-12-19 16:40:54 PST
Comment on attachment 694150 [details] [diff] [review]
Simple patch that hides non-icon modes

The migration probably belongs in nsBrowserGlue's _migrateUI rather than delayedStartup. The rest of the patch looks sound, but I have not looked into whether it is complete (Dao may have a better sense of this anyhow).
Comment 4 Jared Wein [:jaws] (please needinfo? me) 2012-12-19 17:09:33 PST
Created attachment 694162 [details] [diff] [review]
Moved migration code to _migrateUI

Thanks for the feedback Gavin, I moved the migration code to _migrateUI.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-12-20 11:31:53 PST
Comment on attachment 694162 [details] [diff] [review]
Moved migration code to _migrateUI

There is no gNavToolbox in nsBrowserGlue, so you can't just copy the code as-is. You need to adjust the code to modify the localstore.rdf values as the other UI migration blocks do.
Comment 6 Dão Gottwald [:dao] 2012-12-20 11:41:42 PST
Comment on attachment 694162 [details] [diff] [review]
Moved migration code to _migrateUI

This looks like a reasonable approach. There's more code to touch in winstripe/browser.css at least, I haven't looked at pinstripe and gnomestripe.

This will probably upset a number of people, among them some who use the labels as an accessibility feature and who I'd rather keep supporting, like my grandparents who can't make sense of the Firefox UI without the labels.
Comment 7 Jared Wein [:jaws] (please needinfo? me) 2012-12-26 14:46:34 PST
Created attachment 695846 [details] [diff] [review]
Patch v1.2

Fixed the nsBrowserGlue code. I removed the last remaining bit that I could find in winstripe that mentioned mode="icons". What do you think we should do with the styles in content/browser.css that mention mode="icons" (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.css#140)?
Comment 8 Dão Gottwald [:dao] 2012-12-26 15:12:59 PST
Comment on attachment 695846 [details] [diff] [review]
Patch v1.2

See my previous comment; I don't want to be the reviewer here.
Comment 9 Justin Dolske [:Dolske] 2012-12-26 18:58:02 PST
(In reply to Dão Gottwald [:dao] from comment #6)

> This will probably upset a number of people, among them some who use the
> labels as an accessibility feature and who I'd rather keep supporting, like
> my grandparents who can't make sense of the Firefox UI without the labels.

We did discuss this a bit somewhere... The irony of it is that the people most likely to be helped by the text labels are also least likely to be able to find it. At least without assistance. We've still got tooltips, which are much more discoverable (and more descriptive, since they have less space constraints). So it feels adequately addressed to me.
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-12-29 07:28:16 PST
Comment on attachment 695846 [details] [diff] [review]
Patch v1.2

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js

>+    if (currentUIVersion < 9) {

>+        // Update the nav-bar as well.

Do we need to update all of the toolbars (i.e. to handle the presence of custom toolbars?).

>diff --git a/browser/themes/gnomestripe/browser.css b/browser/themes/gnomestripe/browser.css

There are two more selectors in this file that specify [mode="icons"] and that are now redundant.

>diff --git a/browser/themes/pinstripe/browser.css b/browser/themes/pinstripe/browser.css

Similarly, there are many  toolbar:not([mode="icons"]) and toolbar[mode="icons"] selectors in this file that could be removed.

>diff --git a/browser/themes/winstripe/browser.css b/browser/themes/winstripe/browser.css

>-/* Prevent [mode="icons"] from hiding the label */
>-.bookmark-item > .toolbarbutton-text {
>-  display: -moz-box !important;
>-}

This rule is still needed (though you could adjust the comment).

There are also some mode=icons selectors left in this file.

>diff --git a/toolkit/content/customizeToolbar.js b/toolkit/content/customizeToolbar.js

>   else if (window.frameElement &&
>            "toolbox" in window.frameElement) {

>+    InitWithToolbox(window.frameElement.toolbox, window.options);

Doesn't this need to be window.frameElement.options?

>+function InitWithToolbox(aToolbox, aOptions)

>+    var modeList = document.getElementById("modelist");
>+    var label = modeList.previousElementSibling;
>+    label.hidden = true;
>+    modeList.hidden = true;

You should just give this label an ID rather than referring to it via previousElementSibling.
Comment 11 Mike Conley (:mconley) - (needinfo me!) 2013-01-02 11:04:28 PST
Comment on attachment 695846 [details] [diff] [review]
Patch v1.2

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

Not much to add to gavin's review - just two nits.

::: browser/components/nsBrowserGlue.js
@@ +1471,5 @@
> +        // Update the nav-bar as well.
> +        let navbarResource = this._rdf.GetResource(BROWSER_DOCURL + "nav-bar");
> +        this._setPersist(navbarResource, modeResource, "icons");
> +        this._setPersist(navbarResource, iconsizeResource, "large");
> +        

Nit: you can drop this line

::: toolkit/content/customizeToolbar.js
@@ +33,5 @@
>      toolbar.setAttribute("customizing", "true");
>    });
>    gPaletteBox = document.getElementById("palette-box");
> +  if (aOptions && aOptions.hideToolbarModes) {
> +    var modeList = document.getElementById("modelist");

I think we prefer let over var now.
Comment 12 Jared Wein [:jaws] (please needinfo? me) 2013-01-09 12:40:25 PST
Created attachment 700010 [details] [diff] [review]
Patch v1.3

Thanks for the feedback. Does this cover enough of the toolbars? I have it now updating navigator-toolbox, nav-bar, PersonalToolbar, and addon-bar.
Comment 13 Mike Conley (:mconley) - (needinfo me!) 2013-01-09 12:55:47 PST
Comment on attachment 700010 [details] [diff] [review]
Patch v1.3

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

I can't find anything wrong with this. I did some mxr'ing to see if you missed any rules, and I couldn't find any obvious ones.

My only note is about the migration - see below.

::: browser/components/nsBrowserGlue.js
@@ +1437,5 @@
>          Services.prefs.clearUserPref("browser.startup.homepage");
>        }
>      }
>  
> +    if (currentUIVersion < 9) {

I assume this is targeting FF >= 21. If that's so, you're going to conflict with bug 825852 which adds a migration version 9 to add the downloads button.

That patch is likely to land in Nightly by the end of the week, and be uplifted to Aurora soon after. I think you might want to build off of that patch, and use a UI version of 10 instead.
Comment 14 Matthew N. [:MattN] (In Taipei until Sep. 23) 2013-01-09 15:38:59 PST
The plan is to hold this back until Australis Customization lands so that there are improvements that come with it, right?
Comment 15 Justin Dolske [:Dolske] 2013-01-09 21:55:04 PST
(In reply to Jared Wein [:jaws] from comment #12)
> Created attachment 700010 [details] [diff] [review]

Aww, you just missed attachment 700000 [details] [diff] [review]. Maybe next time!

(In reply to Matthew N. [:MattN] from comment #14)
> The plan is to hold this back until Australis Customization lands so that
> there are improvements that come with it, right?

A fine question. I don't feel strongly about it either way, but that might be a good idea. Any downside to rolling this into Australis, other than time?
Comment 16 Jared Wein [:jaws] (please needinfo? me) 2013-01-11 10:27:42 PST
Including it with Australis seems fine to me.
Comment 17 Jared Wein [:jaws] (please needinfo? me) 2013-01-15 13:33:56 PST
Created attachment 702498 [details] [diff] [review]
Patch for checkin to UX branch

Carrying over r+ from mconley, waiting to land until bug 825852 gets merged to UX branch.
Comment 18 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-01-15 13:35:56 PST
I had one concern with the current patch: the migration doesn't handle custom toolbars, which seems like it could leave some users in the dust with messed up UIs (after we've removed the styling).
Comment 19 Jared Wein [:jaws] (please needinfo? me) 2013-01-22 09:44:07 PST
(In reply to :Gavin Sharp (away Jan 16-23) from comment #18)
> I had one concern with the current patch: the migration doesn't handle
> custom toolbars, which seems like it could leave some users in the dust with
> messed up UIs (after we've removed the styling).

I looked in my localstore.rdf file and didn't see the attributes for iconsize or mode being persisted for custom toolbars. http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/toolbar.xml#83 shows that when custom toolbars are appended they are given the mode and iconsize of the toolbox.

I tested out my patch with custom toolbars, where I had mode=full before the patch. After applying the patch and running through the migration code, the toolbars show the toolbar buttons as icons with no text.
Comment 20 Jared Wein [:jaws] (please needinfo? me) 2013-01-22 10:54:15 PST
Pushed to the UX branch, https://hg.mozilla.org/projects/ux/rev/78b8878ed4bf
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-01-25 11:08:04 PST
Bug 829416 made me think that we need to check for other people checking the toolbar's "mode" attribute. The code touched in that bug's patch, at least, but also:

http://hg.mozilla.org/mozilla-central/annotate/a207f33adc1a/browser/base/content/browser.js#l4426
http://hg.mozilla.org/mozilla-central/annotate/a207f33adc1a/browser/base/content/browser-fullScreen.js#l506
and potentially:
http://hg.mozilla.org/mozilla-central/annotate/a207f33adc1a/browser/base/content/browser.js#l3687 (not sure about this one)
Comment 22 Jared Wein [:jaws] (please needinfo? me) 2013-01-29 11:54:57 PST
*** Bug 835956 has been marked as a duplicate of this bug. ***
Comment 23 Jared Wein [:jaws] (please needinfo? me) 2013-01-29 22:50:29 PST
Created attachment 708010 [details] [diff] [review]
Remove small icons and remove any references that were checking for different modes or iconsizes (part 2)

This is in addition to the previous patch. I don't have a Mac with me at the moment, so if you can test it out on Mac that would be cool :)
Comment 24 Dão Gottwald [:dao] 2013-01-30 09:23:08 PST
Why is the iconsize removal suddenly part of this patch?
Comment 25 Siddhartha Dugar [:sdrocking] 2013-01-30 09:25:52 PST
With iconsize gone, will buttons continue to be in small size mode when placed in a toolbar other than the nav-bar?
Comment 26 Dão Gottwald [:dao] 2013-01-30 09:58:31 PST
(In reply to Siddhartha Dugar [:sdrocking] from comment #25)
> With iconsize gone, will buttons continue to be in small size mode when
> placed in a toolbar other than the nav-bar?

Third-party icons would be large.
Comment 27 Jared Wein [:jaws] (please needinfo? me) 2013-01-30 14:47:35 PST
Comment on attachment 708010 [details] [diff] [review]
Remove small icons and remove any references that were checking for different modes or iconsizes (part 2)

I'll move this to another bug so the merits of it can be discussed there and not get mixed with this bug.
Comment 28 Greg 2013-02-03 07:17:47 PST
I think removing support for small icons is a terrible idea. The better move would be to remove support for large icons as I do not know a single user of firefox that uses the large icons. 

Everyone I know both at work and personally uses the small icons. Before switching to Firefox, my friend complained to Google about Chrome's lack of a small-icon setting. (Personally, I only use google-chrome on occasions when I need h.264 to work). Small-icons and the ability to use an extension to put tabs on the side are two of the greatest things about Firefox because of the vertical real-estate one can save. Please don't take small-icons away. The Firefox Large-icon setting takes up so much vertical space I'd rather forgo icons altogether (and just have the URL and SEARCH boxes) than HAVE to use large-icons. When I set up a computer for my elderly mother and presented the two choices to her, even she chose the small icons and she has terrible eyes. The overall resolution of the display etc is what made a difference for her viewing. The large icons are just too big and out of sync with the rest of firefox design. The current "small icons" are the best layout and should be preserved.

I propose a patch to remove LARGE icons. Large-icons are what should be removed if you want to remove something.
Comment 29 Pardal Freudenthal (:ShareBird) 2013-02-04 11:03:23 PST
I have a question: will third-party themes still able to show small buttons?
Comment 30 Ronald Hunter 2013-02-05 09:03:13 PST
I don't understand this.  What is the rationale behind this suggestion?  Does it improve the user experience?  Does anyone think this will cause users to be more satisfied with the product?  Does it improve the function?  If all of these questions can't be answered 'yes', and explained, then WHY do it?
Comment 31 David E. Ross 2013-02-05 14:46:45 PST
Re comment #27:  What is the number of the other bug report?
Comment 32 Matthew N. [:MattN] (In Taipei until Sep. 23) 2013-02-26 16:29:07 PST
Comment on attachment 702498 [details] [diff] [review]
Patch for checkin to UX branch

Could you update this for trunk and push it to UX? Make sure not to introduce new heads (e.g. if you have the old UX branch commits there).
Comment 33 Jared Wein [:jaws] (please needinfo? me) 2013-04-17 09:47:24 PDT
Created attachment 738576 [details] [diff] [review]
Rebased patch for UX branch

Rebased the patch, I'll need to write a followup that will remove the small icons mode.

I'd like to still do it on the same UI_VERSION for migration, unless we feel a need to land this right away. I'll fold the two patches when I get that finished if this hasn't landed already.
Comment 34 Jared Wein [:jaws] (please needinfo? me) 2013-04-17 09:49:07 PDT
(In reply to Jared Wein [:jaws] from comment #33)

One thing to note about this rebased patch is that it no longer includes the changes to the toolkit customize dialog, since Firefox isn't going to use that dialog anymore.
Comment 35 Jared Wein [:jaws] (please needinfo? me) 2013-04-17 10:22:18 PDT
Created attachment 738598 [details] [diff] [review]
Part 2, remove small icons mode

I removed all references to small icons mode that I could find, but I'm not sure if we need to change http://hg.mozilla.org/mozilla-central/annotate/ef0432d35332/browser/base/content/browser.js#l3451 or keep it?

Also, do we need to keep smallicons mode for fullscreen, http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-fullScreen.js#519 ?
Comment 36 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-04-17 13:35:34 PDT
So does this bug need to be re-summarized now, to include removing support for small icons? Seems like maybe it would be better to do that in a separate bug/patch, and keep this one focused on the original patch.
Comment 37 Matthew N. [:MattN] (In Taipei until Sep. 23) 2013-04-17 17:04:42 PDT
I agree that the small icons mode patch should move to a separate bug as was planned in comment 27.
Comment 38 Mike Conley (:mconley) - (needinfo me!) 2013-04-23 06:49:12 PDT
Jared - am I OK to re-land this on the UX branch?
Comment 39 Jared Wein [:jaws] (please needinfo? me) 2013-04-23 08:02:09 PDT
Yes, land away. Sorry for not pushing this yesterday.
Comment 40 Jared Wein [:jaws] (please needinfo? me) 2013-04-23 14:58:19 PDT
Created attachment 741042 [details] [diff] [review]
Rebased on top of 748894

I rebased on top of bug 748894 (on m-c). Hopefully there won't be much churn between m-c and ux/jamun for the landing. Once bug 748894 gets merged to m-c, we can merge m-c -> UX and then UX -> jamun. After all those merges, this patch can land on jamun.
Comment 41 Jared Wein [:jaws] (please needinfo? me) 2013-04-24 10:36:42 PDT
https://hg.mozilla.org/projects/jamun/rev/f7c8d5bc8a86
Comment 42 Mike Conley (:mconley) - (needinfo me!) 2013-05-02 15:53:07 PDT
https://hg.mozilla.org/projects/ux/rev/f7c8d5bc8a86
Comment 43 rexyrexy2 2013-08-18 21:02:51 PDT
@comment 30
There is no reason, Mozilla just LOVE to RUIN Firefox for everyone in the whole world just to make it "EASIER TO USE" (they are in fact making it harder to use, that is nullified), and "EASIER TO UNDERSTAND FOR N00BS" (unless you're a five year old trying to customize it, then you will be able to diagnose and fix things, that is nullified). Which leaves no reason, other than to PURPUSFULLY MAKE THEIR BROWSER LESS CUSTOMIZABLE IN A DESPERATE ATTEMPT TO FURTHER MIMICK CHROME (hence the removal of the option to toggle tabs on top off, and the removal of app menu). they want to LOOK LIKE CHROME, COPY THE REMOVAL OF THE BACK FORWARD DROPDOWN LIKE CHROME, NOT HAVE A BOTTOM BAR LIKE CHROME, AND BE UN-FLEXIBLE LIKE CHROME!!! ALL OF THE OTHER REASONS FOR ANY OF THOSE CHANGES ARE EXCUSES TO JUSTIFY THEM.

Note You need to log in before you can comment on or make changes to this bug.