Closed Bug 863299 Opened 10 years ago Closed 10 years ago

Drop support for small icons mode in the toolbars

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: jaws, Assigned: jaws)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat, Whiteboard: [Australis:M7])

Attachments

(1 file, 4 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
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 ?
Attachment #739109 - Flags: review?(mnoorenberghe+bmo)
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.
+++ 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.
(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.
Attachment #739109 - Attachment is obsolete: true
Attachment #739109 - Flags: review?(mnoorenberghe+bmo)
No longer blocks: 860814
Whiteboard: [Australis:M6]
Attached patch Patch v1.1 (obsolete) — Splinter Review
Rebased and fixed the issues that Dao mentioned.
Attachment #754999 - Flags: review?(mnoorenberghe+bmo)
Attachment #754999 - Flags: review?(felipc)
Attachment #754999 - Flags: review?(felipc) → review?(dao)
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?
Attachment #754999 - Flags: review?(mnoorenberghe+bmo) → feedback+
Whiteboard: [Australis:M6] → [Australis:M7]
Attachment #754999 - Flags: review?(dao)
Attached patch Patch v1.2 (obsolete) — Splinter Review
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.
Attachment #754999 - Attachment is obsolete: true
Attachment #760523 - Flags: review?(bmcbride)
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.
Attachment #760523 - Flags: review?(bmcbride) → review-
Attached patch Patch v1.3 (obsolete) — Splinter Review
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.
Attachment #760523 - Attachment is obsolete: true
Attachment #760722 - Flags: review?(bmcbride)
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).
(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?
Flags: needinfo?(bmcbride)
I have a patch coming that will work around this issue.
Flags: needinfo?(bmcbride)
Attached patch Patch v1.4Splinter Review
Attachment #760722 - Attachment is obsolete: true
Attachment #760722 - Flags: review?(bmcbride)
Attachment #760978 - Flags: review?(bmcbride)
Comment on attachment 760978 [details] [diff] [review]
Patch v1.4

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

That works :)
Attachment #760978 - Flags: review?(bmcbride) → review+
https://hg.mozilla.org/projects/ux/rev/d34b32544b06
Whiteboard: [Australis:M7] → [Australis:M7][fixed-in-ux]
Keywords: addon-compat
Depends on: 885065
Depends on: 885452
Blocks: 890356
No longer blocks: 890356
Depends on: 890356
Depends on: 896008
No longer depends on: 896008
Depends on: 896008
Depends on: 925338
https://hg.mozilla.org/mozilla-central/rev/d34b32544b06
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M7][fixed-in-ux] → [Australis:M7]
Target Milestone: --- → Firefox 28
Depends on: 940935
You need to log in before you can comment on or make changes to this bug.