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

RESOLVED FIXED in Firefox 28

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: mstange, Assigned: jaws)

Tracking

Trunk
Firefox 28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

7 years ago
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?
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.
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.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #694150 - Flags: feedback?(gavin.sharp)
Attachment #694150 - Flags: feedback?(dao)
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).
Attachment #694150 - Flags: feedback?(gavin.sharp) → feedback+
Created attachment 694162 [details] [diff] [review]
Moved migration code to _migrateUI

Thanks for the feedback Gavin, I moved the migration code to _migrateUI.
Attachment #694150 - Attachment is obsolete: true
Attachment #694150 - Flags: feedback?(dao)
Attachment #694162 - Flags: feedback?(dao)
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 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.
Attachment #694162 - Flags: feedback?(dao)
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)?
Attachment #694162 - Attachment is obsolete: true
Attachment #695846 - Flags: review?(gavin.sharp)
Attachment #695846 - Flags: review?(dao)
Comment on attachment 695846 [details] [diff] [review]
Patch v1.2

See my previous comment; I don't want to be the reviewer here.
Attachment #695846 - Flags: review?(dao)
(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.
Attachment #695846 - Flags: review?(mconley)
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.
Attachment #695846 - Flags: review?(gavin.sharp) → review-
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.
Attachment #695846 - Flags: review?(mconley)
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.
Attachment #695846 - Attachment is obsolete: true
Attachment #700010 - Flags: review?(mconley)
Attachment #700010 - Flags: review?(gavin.sharp)
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.
Attachment #700010 - Flags: review?(mconley) → review+
The plan is to hold this back until Australis Customization lands so that there are improvements that come with it, right?
(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?
Including it with Australis seems fine to me.
Depends on: 825852
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.
Attachment #700010 - Attachment is obsolete: true
Attachment #700010 - Flags: review?(gavin.sharp)
Attachment #702498 - Flags: review+
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).
(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.
Pushed to the UX branch, https://hg.mozilla.org/projects/ux/rev/78b8878ed4bf
Whiteboard: [fixed-in-ux]
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)
Depends on: 835956
Summary: Drop support for icons+text toolbar mode in the main browser window → Only support large icon toolbar mode in the main browser window
No longer depends on: 835956
Duplicate of this bug: 835956
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 :)
Attachment #708010 - Flags: review?(mnoorenberghe+bmo)
Why is the iconsize removal suddenly part of this patch?
With iconsize gone, will buttons continue to be in small size mode when placed in a toolbar other than the nav-bar?
(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 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.
Attachment #708010 - Attachment is obsolete: true
Attachment #708010 - Flags: review?(mnoorenberghe+bmo)

Updated

5 years ago
Summary: Only support large icon toolbar mode in the main browser window → Drop support for text and icons+text toolbar modes in the main browser window

Comment 28

4 years ago
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.
I have a question: will third-party themes still able to show small buttons?

Comment 30

4 years ago
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

4 years ago
Re comment #27:  What is the number of the other bug report?
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).

Updated

4 years ago
Blocks: 810028
Flags: needinfo?(jAwS)
Blocks: 860814
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.
Attachment #702498 - Attachment is obsolete: true
Attachment #738576 - Flags: review+
Flags: needinfo?(jaws)
(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.
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 ?
Attachment #738598 - Flags: review?(dao)
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.
I agree that the small icons mode patch should move to a separate bug as was planned in comment 27.
Attachment #738598 - Attachment is obsolete: true
Attachment #738598 - Flags: review?(dao)
Blocks: 863299

Updated

4 years ago
Blocks: 864355
Jared - am I OK to re-land this on the UX branch?
Whiteboard: [fixed-in-ux]

Updated

4 years ago
Flags: needinfo?(jaws)
Yes, land away. Sorry for not pushing this yesterday.
Flags: needinfo?(jaws)
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.
Attachment #738576 - Attachment is obsolete: true
Attachment #741042 - Flags: review+
https://hg.mozilla.org/projects/jamun/rev/f7c8d5bc8a86
Whiteboard: [fixed in jamun]
https://hg.mozilla.org/projects/ux/rev/f7c8d5bc8a86
Whiteboard: [fixed in jamun] → [fixed in jamun][fixed-in-ux]

Updated

4 years ago
Depends on: 880421
Depends on: 881385

Comment 43

4 years ago
@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.

Updated

4 years ago
Depends on: 925338

Comment 44

4 years ago
https://hg.mozilla.org/mozilla-central/rev/f7c8d5bc8a86
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in jamun][fixed-in-ux]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.