Closed Bug 573329 Opened 14 years ago Closed 11 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

Product:
Firefox
Component:
Toolbars and Customization
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 28

People

(Reporter: mstange, Assigned: jaws)

References

Details

Attachments

(1 file, 8 obsolete files)

Rebased on top of 748894
26.26 KB, patch
jaws
: review+
Details | Diff | Splinter Review 
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.
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+
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)
Attached patch Patch v1.2 (obsolete) — — Splinter Review 
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)
Attached patch Patch v1.3 (obsolete) — — Splinter Review 
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
Attached patch Patch for checkin to UX branch (obsolete) — — Splinter Review 
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]
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
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)
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
I have a question: will third-party themes still able to show small buttons?
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).
Blocks: 810028
Flags: needinfo?(jAwS)
Blocks: 860814
Attached patch Rebased patch for UX branch (obsolete) — — Splinter Review 
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.
Attached patch Part 2, remove small icons mode (obsolete) — — Splinter Review 
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
Jared - am I OK to re-land this on the UX branch?
Whiteboard: [fixed-in-ux]
Flags: needinfo?(jaws)
Yes, land away. Sorry for not pushing this yesterday.
Flags: needinfo?(jaws)
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/ux/rev/f7c8d5bc8a86
Whiteboard: [fixed in jamun] → [fixed in jamun][fixed-in-ux]
Depends on: 880421
Depends on: 881385
Depends on: 925338
https://hg.mozilla.org/mozilla-central/rev/f7c8d5bc8a86
Status: ASSIGNED → RESOLVED
Closed: 11 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.

Attachment

General

Creator:
Created:
Updated:
Size: