573329 - Drop support for text and icons+text toolbar modes in the main browser window

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Markus Stange [:mstange]]

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]]

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)]

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

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)]

Attachment: Moved migration code to _migrateUI

Thanks for the feedback Gavin, I moved the migration code to _migrateUI.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

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]]

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)]

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

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]]

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

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) (:⚙️)]

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)]

Attachment: 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) (:⚙️)]

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]]

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]]

(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)]

Including it with Australis seems fine to me.

Comment 17

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

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

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)]

(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)]

Pushed to the UX branch, https://hg.mozilla.org/projects/ux/rev/78b8878ed4bf

Comment 21

[:Gavin Sharp [email: [e-mail]]]

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 23

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

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

Why is the iconsize removal suddenly part of this patch?

Comment 25

[Siddhartha Dugar [:sdrocking]]

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]]

(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)]

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]

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)]

I have a question: will third-party themes still able to show small buttons?

Comment 30

[Ronald Hunter]

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]

Re comment #27:  What is the number of the other bug report?

Comment 32

[Matthew N. [:MattN]]

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)]

Attachment: 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)]

(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)]

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

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]]

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) (:⚙️)]

Jared - am I OK to re-land this on the UX branch?

Comment 39

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

Yes, land away. Sorry for not pushing this yesterday.

Comment 40

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

Attachment: 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)]

https://hg.mozilla.org/projects/jamun/rev/f7c8d5bc8a86

Comment 42

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/projects/ux/rev/f7c8d5bc8a86

Comment 43

[rexyrexy2]

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

Comment 44

[:Gijs (out for now; he/him)]

https://hg.mozilla.org/mozilla-central/rev/f7c8d5bc8a86