947410 - Add-on Bar shim always thinks add-on bar is collapsed even if it wasn't

Status: VERIFIED FIXED

(Firefox :: Extension Compatibility, defect)

Description

[Jorge Villalobos [:jorgev] (he/him)]

Attachment: aus-addonbar-test.xpi

I created the attached add-on to test add-ons on Australis. It adds a button in the Add-on Bar using fairly standard code (overlaying the palette and then adding it to the currentset). It works in current versions of Firefox.

According to the docs, the button is somehow migrated to the main toolbar. However, the current result is that the button isn't migrated after installation. I see an attribute in the addon-bar node: migratedset="my-button-id", but that's about it.

Comment 1

[:Gijs (he/him)]

(In reply to Jorge Villalobos [:jorgev] from comment #1)
> Created attachment 8343946 [details]
> aus-addonbar-test.xpi
> 
> I created the attached add-on to test add-ons on Australis. It adds a button
> in the Add-on Bar using fairly standard code (overlaying the palette and
> then adding it to the currentset). It works in current versions of Firefox.
> 
> According to the docs, the button is somehow migrated to the main toolbar.
> However, the current result is that the button isn't migrated after
> installation. I see an attribute in the addon-bar node:
> migratedset="my-button-id", but that's about it.

I can't reproduce. I just tried your add-on, and it adds a button with a greyed out Firefox logo and tooltip "Toolbar test" to my navbar automagically once I've restarted (seems it isn't a restartless add-on?), so it seems it works here.

I'm testing on OS X on a relatively clean profile. What OS are you testing on, can you reproduce reliably, can you give more details? (e.g. can you check if the item ends up in the palette instead? What version of Nightly did you use, etc.? Did you install this add-on on the same profile before? Seems you have prefs code that won't insert the item more than once...)

Comment 2

[Kris Maglione [:kmag]]

It doesn't work for me in the latest nightly on Linux. It appears in the customize pane, but not in any toolbar.

Comment 3

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] from comment #2)
> It doesn't work for me in the latest nightly on Linux. It appears in the
> customize pane, but not in any toolbar.

Fascinating. Clean profile? If not, can you check the iconsize and the mode attributes set on the toolbox and the add-on toolbar? We have migration code to nuke the persisted values, but if you use the profile in both Australis and non-Australis, it's possible that you changed it again and then the migrateUI stuff (nsBrowserGlue.js) didn't re-run. We've never had a good story for migrating back and forth between releases.

Anyway, I'm hypothesizing after midnight and I'm tired. Still need a bit more info, I guess... Generally, if it puts it in the toolbox it is considered too wide (id est > 100px). See the code in http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/toolbar.xml#461

Comment 4

[Kris Maglione [:kmag]]

For certain values of "clean profile". My testing scripts use a clean copy of a very basic skeleton profile for every run, and I use a different skeleton for each Firefox version. I'll try creating a new profile, though.

Comment 5

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] from comment #2)
> the customize pane

fwiw, this is what I meant when I said "palette". As noted, we migrate items there if we believe they are too wide to go into the navbar. Shouldn't happen with ordinary icons like this, though, so something fishy is still going on. See also bug

Oh, and also, if your add-on bar was collapsed on the profile before going to Australis, we will migrate to the palette rather than the navbar (so users who had the addonbar hidden aren't suddenly surprised to find 20 icons crowding up their navbar, or something).

Comment 6

[Kris Maglione [:kmag]]

Hm. Yes, with a brand new profile, it does work.

Comment 7

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #5)
> going on. See also bug

Ugh. Need sleep. bug 942051.

Comment 8

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] from comment #6)
> Hm. Yes, with a brand new profile, it does work.

OK. While, as I wrote that code, that is somewhat gratifying :-), it'd still be good if we could figure out what your skeleton profile does that trips up the migration... this not working is not good.

Comment 9

[Jorge Villalobos [:jorgev] (he/him)]

I can confirm that testing in a brand new Nightly profile, the problem doesn't occur.

I can reproduce doing this:
1) Run latest Aurora in a new profile.
2) Install add-on.
3) Open same profile on Nightly.

So, this would break the majority of cases, where users will be migrating their existing profiles to Australis.

Comment 10

[:Gijs (he/him)]

(In reply to Jorge Villalobos [:jorgev] from comment #9)
> I can confirm that testing in a brand new Nightly profile, the problem
> doesn't occur.
> 
> I can reproduce doing this:
> 1) Run latest Aurora in a new profile.
> 2) Install add-on.
> 3) Open same profile on Nightly.
> 
> So, this would break the majority of cases, where users will be migrating
> their existing profiles to Australis.

Yeah, that's no good. I'll try to make time to look at this later today.

Comment 11

[:Gijs (he/him)]

So at least in some cases, if you unhide the add-on bar, it'll have collapsed="false", not no attribute. We should make that check stricter. Patch in a second.

Comment 12

[:Gijs (he/him)]

Attachment: Australis' addonbar shim doesn't check collapsed state correctly,

d'oh.

Comment 13

[Mike de Boer [:mikedeboer]]

Comment on attachment 8347968 [details] [diff] [review]
Australis' addonbar shim doesn't check collapsed state correctly,

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

LGTM!

Comment 14

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/08e0383e7428

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/08e0383e7428

Comment 16

[:Gijs (he/him)]

Jorge, for my peace of mind, can you confirm this is fixed now?

(sadly automated tests for this are somewhat tricky because they'd require restarts and mochitest can't do that)

Comment 17

[Jorge Villalobos [:jorgev] (he/him)]

I can still reproduce the bug on the latest Nightly:

1) Run latest Aurora in a new profile.
2) Install add-on.
3) Enable Add-on Bar.
4) Open same profile on Nightly.

Comment 18

[:Gijs (he/him)]

(In reply to Jorge Villalobos [:jorgev] from comment #17)
> I can still reproduce the bug on the latest Nightly:
> 
> 1) Run latest Aurora in a new profile.
> 2) Install add-on.
> 3) Enable Add-on Bar.
> 4) Open same profile on Nightly.

Argh. I will try to look at this while I'm on end-of-year leave, then. :-\

Comment 19

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #18)
> (In reply to Jorge Villalobos [:jorgev] from comment #17)
> > I can still reproduce the bug on the latest Nightly:
> > 
> > 1) Run latest Aurora in a new profile.
> > 2) Install add-on.
> > 3) Enable Add-on Bar.
> > 4) Open same profile on Nightly.
> 
> Argh. I will try to look at this while I'm on end-of-year leave, then. :-\

Jorge, I'm sorry this took a bit longer, but I just checked and at least for me, with latest aurora and fx-team as of an hour or two ago, those STR work. Not sure what's different for you - I take it you can still reproduce?

Comment 20

[Jorge Villalobos [:jorgev] (he/him)]

I just tried it again and now it works as expected.