863299 - Drop support for small icons mode in the toolbars

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

Attachment: Patch

For Australis, we are going to simplify our user interface so we can focus more of our time implementing core features. Small icons mode is not the default view and would be fulfilled better if it was implemented as an optional theme.

The attached patch removes 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 1

[Dão Gottwald [:dao]]

You completely removed @navbarLargeIcons@, but all those selectors were not only large-icon mode specific but also nav-bar specific. Same goes for '#navigator-toolbox[iconsize="large"] > #nav-bar' which you removed rather than replacing it with '#nav-bar'.

Some of your JS changes are about the toolbar mode rather than icon size.

General note: All the stuff supporting small icons mode seems pretty straightforward and mechanic. I don't think removing it frees up resources significantly.

Comment 2

[Richard Marti (:Paenglab)]

+++ b/toolkit/content/customizeToolbar.js
-    let modeList = document.getElementById("modelist");
-    let label = document.getElementById("modelistLabel");
-    label.hidden = true;
-    modeList.hidden = true;
+    for (let id of ["modelist", "modelistLabel", "smallicons"]) {
+      let element = document.getElementById(id);
+      element.hidden = true;
+    }

It looks like this hides this IDs for all applications like Thunderbird and Seamonkey too, is this true?

Please can't you add rules in content/browser.css to hide them? Or better would be in skin/browser.css then third party themes can still support the modes they want.

Comment 3

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

(In reply to Richard Marti [:Paenglab] from comment #2)
> +++ b/toolkit/content/customizeToolbar.js
> -    let modeList = document.getElementById("modelist");
> -    let label = document.getElementById("modelistLabel");
> -    label.hidden = true;
> -    modeList.hidden = true;
> +    for (let id of ["modelist", "modelistLabel", "smallicons"]) {
> +      let element = document.getElementById(id);
> +      element.hidden = true;
> +    }
> 
> It looks like this hides this IDs for all applications like Thunderbird and
> Seamonkey too, is this true?
> 
> Please can't you add rules in content/browser.css to hide them? Or better
> would be in skin/browser.css then third party themes can still support the
> modes they want.

Please disregard this change, it was supposed to be reverted but it slipped by.

Comment 4

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

Attachment: Patch v1.1

Rebased and fixed the issues that Dao mentioned.

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 754999 [details] [diff] [review]
Patch v1.1

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

I'm not comfortable reviewing the JS changes as I don't have experience with this code and haven't even read/followed bugs where other people are touching the modes. I didn't see obvious issues with the CSS but I was surprised that this doesn't remove much except for on Linux.

::: browser/base/content/browser-social.js
@@ -814,5 @@
>      iframe.setAttribute("origin", provider.origin);
>      iframe.setAttribute("src", shareEndpoint);
>  
>      let navBar = document.getElementById("nav-bar");
> -    let anchor = navBar.getAttribute("mode") == "text" ?

Is this change something that was missed in bug 573329? I'm not that familiar with how small icons and text/icons+text modes interact tbh.

::: browser/base/content/browser.js
@@ +3300,5 @@
>  
>  function BrowserToolboxCustomizeChange(aType) {
> +  gHomeButton.updatePersonalToolbarStyle();
> +  BookmarksMenuButton.customizeChange();
> +  allTabs.readPref();

Where did allTabs.readPref() come from?

::: browser/components/nsBrowserGlue.js
@@ +1370,5 @@
>        let modeResource = this._rdf.GetResource("mode");
>        let iconsizeResource = this._rdf.GetResource("iconsize");
>        for (let toolbarResource of toolbarResources) {
> +        this._setPersist(toolbarResource, modeResource, "icons");
> +        this._setPersist(toolbarResource, iconsizeResource, "large");

Is the reason we are setting values rather than clearing them because we aren't removing the toolkit capability to set the mode and the default is something other than large? IIUC the default is large.
Does some other code take care of not persisting default values ala pref service? Perhaps this doesn't matter in practice but it seems nicer to work like prefs so that changing the default changes the value for existing users with the old default. I'm not very familiar with how persistence works.

@@ +1385,5 @@
>      }
>  
> +    if (currentUIVersion < 15) {
> +
> +    }

I guess this isn't needed?

Comment 7

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

Attachment: Patch v1.2

I moved the changes in browser-social.js to a separate bug, bug 881385.

Thanks for the feedback. I checked my localstore.rdf and for custom toolbars we only store the collapsed, so this patch now only sets the "large" or "icons" if the toolbar had something different. I suppose we could also clear the attribute, although I don't see that being done elsewhere in migrateUI.

I've made the changes you requested and will request review from Blair.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 760523 [details] [diff] [review]
Patch v1.2

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

Only looked at the JS, as MattN looked at the CSS.

::: browser/components/nsBrowserGlue.js
@@ +1383,4 @@
>            this._setPersist(toolbarResource, modeResource, "icons");
> +        }
> +        let iconsize = this._getPersist(toolbarResource, iconsizeResource);
> +        if (iconsize && iconsize != "large") {

This seems to be incorrect. With the old customizeToolbar.js, the mode was locked for TabsToolbar (via the lockmode attribute) - it couldn't be changed. So toolbarMode was guaranteed to be "icons" for that toolbar, therefore the iconsize was never updated to "large". But, with this change, that toolbar will get iconsize set to "large" - which we don't want.

Comment 9

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

Attachment: Patch v1.3

Huh, I didn't know about lockmode or lockiconsize before. There doesn't appear to be any reference to these attributes outside of browser.xul.

It should also be noted that TabsToolbar isn't modified as it's not in the above array. I did notice that lockiconsize is used for PersonalToolbar.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 760722 [details] [diff] [review]
Patch v1.3

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

::: browser/components/nsBrowserGlue.js
@@ +1380,2 @@
>        let iconsizeResource = this._rdf.GetResource("iconsize");
> +      let lockiconsizeResource = this._rdf.GetResource("lockiconsize");

Er, I don't think the lock* attributes are persisted, are they? They're not in my localstore.rdf, and it didn't seem them get persisted in customizeToolbar.js (relevant code is in updateToolboxProperty, fwiw).

Comment 11

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

(In reply to Blair McBride [:Unfocused] from comment #10)
> Comment on attachment 760722 [details] [diff] [review]
> Patch v1.3
> 
> Review of attachment 760722 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/nsBrowserGlue.js
> @@ +1380,2 @@
> >        let iconsizeResource = this._rdf.GetResource("iconsize");
> > +      let lockiconsizeResource = this._rdf.GetResource("lockiconsize");
> 
> Er, I don't think the lock* attributes are persisted, are they? They're not
> in my localstore.rdf, and it didn't seem them get persisted in
> customizeToolbar.js (relevant code is in updateToolboxProperty, fwiw).

Thanks for pointing out updateToolboxProperty, the |"lock" + aProp| made it a lot harder to find.

But this makes it a bit harder, since during migrateUI we can't call getMostRecentWindow since it runs before any window has been opened. What do you suppose I do here?

Comment 12

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

I have a patch coming that will work around this issue.

Comment 13

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

Attachment: Patch v1.4

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 760978 [details] [diff] [review]
Patch v1.4

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

That works :)

Comment 15

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

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

Comment 16

[:Gijs (he/him)]

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