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

RESOLVED FIXED in Firefox 28

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nmaier, Assigned: Gijs)

Tracking

(Blocks: 1 bug, {addon-compat, regression})

unspecified
Firefox 28
addon-compat, regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:P2])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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?
Flags: needinfo?(dao)
(Assignee)

Updated

5 years ago
Summary: Australis does not properly set iconsize=small → Australis' navbar's iconsize=small gets overridden by localstore.rdf
(In reply to :Gijs Kruitbosch from comment #1)
> maybe with a migration path?

Yep.
Flags: needinfo?(dao)
(Assignee)

Updated

5 years ago
Whiteboard: [Australis] → [Australis:P2]
(Assignee)

Updated

5 years ago
Duplicate of this bug: 942165
(Assignee)

Comment 4

5 years ago
Created attachment 8336978 [details] [diff] [review]
migrate iconsize correctly for Australis,

As discussed on IRC.
Attachment #8336978 - Flags: review?(jaws)
(Assignee)

Updated

5 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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.
Attachment #8336978 - Flags: review+ → review-
(Assignee)

Comment 6

5 years ago
Created attachment 8337003 [details] [diff] [review]
migrate iconsize correctly for Australis,

Like this, I presume?
Attachment #8337003 - Flags: review?(dao)
(Assignee)

Updated

5 years ago
Attachment #8336978 - Attachment is obsolete: true
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.

Updated

5 years ago
Attachment #8337003 - Flags: review?(dao)
Attachment #8337003 - Flags: review-
Attachment #8337003 - Flags: feedback+
(Assignee)

Comment 8

5 years ago
(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?
Flags: needinfo?(dao)
(Reporter)

Comment 9

5 years ago
Better reset all toolbars, and that includes "toolbar-menubar" and "TabsToolbar". Better be safe than sorry.
(Assignee)

Comment 10

5 years ago
Created attachment 8337026 [details] [diff] [review]
destroy all the mode and iconsize persistence for Australis,

So here's one option, then...
Attachment #8337026 - Flags: review?(dao)
(Assignee)

Updated

5 years ago
Attachment #8337003 - Attachment is obsolete: true
(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"?
Flags: needinfo?(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".
Attachment #8337026 - Flags: review?(dao) → review+
(Assignee)

Comment 13

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

remote:   https://hg.mozilla.org/integration/fx-team/rev/461585b69748
Whiteboard: [Australis:P2] → [Australis:P2][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/461585b69748
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P2][fixed-in-fx-team] → [Australis:P2]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.