940953 - Australis' navbar's iconsize=small gets overridden by localstore.rdf

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Nils Maier [:nmaier]]

Due to compatibility reasons and convention, the nav-bar (and other browser.xul toolbars) should always use iconsize=small, since the built-in themes are in fact using small icons, unless a third-party theme is installed and instructs to set iconsize=large.

This was implemented in bug 626382 for FX4, which has all the gory details such as using a css counter to set the icon mode.

The reason to do so is that a lot of add-ons essentially do either:
#mytoolbarbutton {
  list-style-image: url(large.png);
}
toolbar[iconsize="small"] #mytoolbarbutton {
  list-style-image: url(small.png);
}
or do it the other way round:
#mytoolbarbutton {
  list-style-image: url(small.png);
}
toolbar[iconsize="large"] #mytoolbarbutton {
  list-style-image: url(large.png);
}

Please note that the above used to be (mostly) compatible with Firefox OSX/Win (small icons), Firefox Linux (large icons) and Seamonkey (large icons).

After upgrading a long running profile to the first Australis nightly, some add-on toolbar buttons started to show up as large again (usually 24x24). DOM Inspector reveals that iconsize=large, which should never happen. I looked into localstore.rdf and sure enough it has nav-bar iconsize=large. There is no longer any way to toggle this from the UI.

Essentially, bug 626382 needs to be re-implemented for Australis, to not break extensions and themes.

Comment 1

[:Gijs (he/him)]

The whole iconsize stuff is a clusterfuck. Let me just note that we attempted to fix this before, but there's an attribute on both the toolbox and the toolbar, and the dialog only ever twiddles the toolbox's attribute. So there never was a way to toggle that attribute from the UI. AFAIK we restored the attribute on the navbar in bug 896008. I don't know why localstore is overriding it, or if we can sanely fix that (maybe with a migration path?). Dão?

Comment 2

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #1)
> maybe with a migration path?

Yep.

Comment 4

[:Gijs (he/him)]

Attachment: migrate iconsize correctly for Australis,

As discussed on IRC.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8336978 [details] [diff] [review]
migrate iconsize correctly for Australis,

We should straight-out remove the properties rather than changing their value. You can do this by calling _setPersist without a new value.

Comment 6

[:Gijs (he/him)]

Attachment: migrate iconsize correctly for Australis,

Like this, I presume?

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8337003 [details] [diff] [review]
migrate iconsize correctly for Australis,

>+      let updateToolbars = function (aToolbarIds, aResourceName, aResourceValue) {
>+        let resource = this._rdf.GetResource(aResourceName);
>+        for (toolbarId of aToolbarIds) {
>+          let toolbar = this._rdf.GetResource(BROWSER_DOCURL + toolbarId);
>+          let oldValue = this._getPersist(toolbar, resource);
>+          if (oldValue && oldValue != aResourceValue) {
>+            this._setPersist(toolbar, resource, aResourceValue);
>+          }
>+        }
>+      }.bind(this);
>+
>+      updateToolbars(["navigator-toolbox", "nav-bar", "PersonalToolbar", "addon-bar"], "mode");
>+      // Exclude PersonalToolbar and addon-bar since they have lockiconsize="true".

This comment doesn't apply anymore.

updateToolbars is never called with the third argument.

Comment 8

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #7)
> >+      updateToolbars(["navigator-toolbox", "nav-bar", "PersonalToolbar", "addon-bar"], "mode");
> >+      // Exclude PersonalToolbar and addon-bar since they have lockiconsize="true".
> 
> This comment doesn't apply anymore.

Ah, but this part I don't follow... we're only resetting the navbar and the toolbox, not the other ones because presumably they don't have anything set, right? Am I missing something? Do you think I should be resetting iconsize for the personaltoolbar and addon-bar as well?

Comment 9

[Nils Maier [:nmaier]]

Better reset all toolbars, and that includes "toolbar-menubar" and "TabsToolbar". Better be safe than sorry.

Comment 10

[:Gijs (he/him)]

Attachment: destroy all the mode and iconsize persistence for Australis,

So here's one option, then...

Comment 11

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #8)
> (In reply to Dão Gottwald [:dao] from comment #7)
> > >+      updateToolbars(["navigator-toolbox", "nav-bar", "PersonalToolbar", "addon-bar"], "mode");
> > >+      // Exclude PersonalToolbar and addon-bar since they have lockiconsize="true".
> > 
> > This comment doesn't apply anymore.
> 
> Ah, but this part I don't follow... we're only resetting the navbar and the
> toolbox, not the other ones because presumably they don't have anything set,
> right? Am I missing something? Do you think I should be resetting iconsize
> for the personaltoolbar and addon-bar as well?

The comment is at least confusing, as we dropped the lockiconsize attribute. Maybe it meant to say that those toolbars /had/ lockiconsize="true"?

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8337026 [details] [diff] [review]
destroy all the mode and iconsize persistence for Australis,

Just removing both attributes for all toolbars is the simplest approach, so I agree with comment 9. Let's add "toolbar-menubar" and "TabsToolbar".

Comment 13

[:Gijs (he/him)]

Landed with TabsToolbar and toolbar-menubar added (and a variable declaration for toolbarId, oops!)

remote:   https://hg.mozilla.org/integration/fx-team/rev/461585b69748

Comment 14

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/461585b69748