I've made the CustomizableUI changes to my add-on, but when it is first installed, none of the buttons appear on the toolbar. They are all in the customize palette. This worked pre Australis. Extension attached.

What I've discovered is that toolbar.currentSet doesn't contain any of my toolbar items/buttons that have removable set to true. I'm deleting localstore.rdf at startup to test. I'm overlaying them directly like this (versus putting them in the pallette). <toolbar id="united-toolbar"> <toolbarbutton id="united-composeemail-button" label="&united.compose.label;" tooltiptext="&united.compose.tooltip;" oncommand="unitedinternet.webapps.startUsecase('new_mail');" orient="horizontal" class="chromeclass-toolbar-additional toolbarbutton-1" removable="true" /> </toolbar> This worked preAustralis.

So it appears that even though I'm overlaying these buttons onto my toolbar, when I look at the parentNode for these buttons, it's a toolbarpaletteitem (not my toolbar). So why are these buttons ending up in the toolbarpalette when they are overlaid onto the toolbar?

So even if I change my code to add these into my toolbar, if I can get Restore Default Set to work (it's currently pretty broke), then it removes everything off of my toolbar again, even though I've expicitly set the defaultset attribute on my toolbar to be the correct defaultset.

(In reply to Mike Kaply (:mkaply) from comment #1) > What I've discovered is that toolbar.currentSet doesn't contain any of my > toolbar items/buttons that have removable set to true. I'm deleting > localstore.rdf at startup to test. > > I'm overlaying them directly like this (versus putting them in the pallette). > > <toolbar id="united-toolbar"> > <toolbarbutton id="united-composeemail-button" > label="&united.compose.label;" > tooltiptext="&united.compose.tooltip;" > oncommand="unitedinternet.webapps.startUsecase('new_mail');" > orient="horizontal" > class="chromeclass-toolbar-additional toolbarbutton-1" > removable="true" > /> > </toolbar> > > This worked preAustralis. We're no longer using localstore.rdf for toolbar positions so that's not a useful test. If your code relies on that attribute directly in some way, that might be why it's broken.

I've reduced the number of buttons we add. The code is still here. In particular, the buttons are in: content/webapps/webapps-toolbaritem.xul The code that manipulates the toolbar is in content/main/toolbar.js Again, the issue is this code in content/main/toolbar.js: // If the item is not on our toolbar, we don't move it if (!tbitem || tbitem.parentNode != tb) { continue; } pre Australis, my items were on my toolbar. After australis, they are not.

It doesn't rely on localstore.rdf, but I am persisting an attribute. This issue happens with a clean install, so it's not localstore.rdf related.

Here is a very minimal testcase. The bug is that if you add a toolbar button as a child of a customizable toolbar in XUL, it doesn't appear on the toolbar. If you remove customizable or don't call registerArea, the button is still there.

I'll look at this tomorrow.

So, the issue is quite simple, it's your use of the API. That is partly our issue because we've not documented it sufficiently, clearly. You have two options: 1) call registerArea(toolbarID, {defaultPlacements: ["button1", "button2", ...]}); I tested this locally and this works for sure. 2) call registerArea(toolbarID, {legacy: true}); and set the defaultset attribute correctly on your toolbar (basically, the same as you would have done before) AFAICT this will also fix bug 941041. I'm resolving this as invalid because this is an issue with the add-on, not with the things CustomizableUI does - it basically just goes "oh, you specificed a default set of [], and there is stuff in there that shouldn't be there, so let me go and get rid of that".

Note that to get correct styling for buttons/icons, you should also pass {type: CustomizableUI.TYPE_TOOLBAR}. We should probably default to that value, though, so I'll file a new bug about that.

Sorry, I'm not going to let you close it until we get the whole thing figured out. Unfortunately your suggestion won't work in the full add-on because we get an invalid customization area in registerToolbarNode before we even call registerArea. Error: Unknown customization area: united-toolbar CustomizableUI.jsm:319 This is happening before our onLoad handler is called. It's happening completely outside of our code or there would be a stack. The customize code should be smart enough to ignore errors from registerToolbarNode until I call registerArea and then reregister the toolbar node.

(In reply to Mike Kaply (:mkaply) from comment #12) > Sorry, I'm not going to let you close it until we get the whole thing > figured out. It would have been better to file a new bug rather than morphing this one for each subsequent issue you discover in migrating your code. :-\ > Unfortunately your suggestion won't work in the full add-on because we get > an invalid customization area in registerToolbarNode before we even call > registerArea. > > Error: Unknown customization area: united-toolbar CustomizableUI.jsm:319 > > This is happening before our onLoad handler is called. It's happening > completely outside of our code or there would be a stack. > > The customize code should be smart enough to ignore errors from > registerToolbarNode until I call registerArea and then reregister the > toolbar node. So in principle the assumption is you should call registerArea before the toolbar is appended to the DOM. I am confused though - how did this work before but not now? That makes no sense, you're calling the same method at the same time, so this must have been broken before you tried adding stuff to the registerArea call. Is that correct?

> It would have been better to file a new bug rather than morphing this one for each subsequent issue you discover in migrating your code. :-\ No because they are all the same problem. registerArea is undocumented. > So in principle the assumption is you should call registerArea before the toolbar is appended to the DOM. I am confused though - how did this work before but not now? That makes no sense, you're calling the same method at the same time, so this must have been broken before you tried adding stuff to the registerArea call. Is that correct? When I first starting porting this add-on, registerArea didn't work in onLoad. So I put it outside onLoad. Now you're telling me I need to pass my defaultset to registerArea, so it has to be in onLoad, since that's where my defaultset is created. I'm creating defaultset dynamically. This is a reasonably common thing to do in an add-on because the defaultset stuff in XUL is a completely screwed up concept. To create the initial defaultset, I setup my toolbar the way I want it on firstrun and then query the items on the toolbar and dynamically set defaultset. My bet is this is what extension developer does.

(In reply to Mike Kaply (:mkaply) from comment #14) > > It would have been better to file a new bug rather than morphing this one for each subsequent issue you discover in migrating your code. :-\ > > No because they are all the same problem. registerArea is undocumented. That's orthogonal to the issue you're currently having. I agree that we should document CustomizableUI, but that's not what you filed a bug about. You're frustrated, and I get that, but I don't think filing bugs for things which are bugs rather than some kind of "help me fix my add-on within the bugzilla comments system" bug is such a terrible requirement. If you disagree, maybe you should just wait until we document. > > So in principle the assumption is you should call registerArea before the toolbar is appended to the DOM. I am confused though - how did this work before but not now? That makes no sense, you're calling the same method at the same time, so this must have been broken before you tried adding stuff to the registerArea call. Is that correct? > > When I first starting porting this add-on, registerArea didn't work in > onLoad. Define "didn't work". Same error? Different error? > Now you're telling me I need to pass my defaultset to registerArea, so it > has to be in onLoad, since that's where my defaultset is created. > > I'm creating defaultset dynamically. This is a reasonably common thing to do > in an add-on because the defaultset stuff in XUL is a completely screwed up > concept. > > To create the initial defaultset, I setup my toolbar the way I want it on > firstrun and then query the items on the toolbar and dynamically set > defaultset. > > My bet is this is what extension developer does. Actually, I don't understand this, nevermind my buying that it's what everyone does. You're setting the default contents of the toolbar at runtime, but you put stuff in the toolbar using an overlay, ie at design/compile time or however you want to call that. So you must know what goes into the toolbar at design/compile time. Why don't you set the defaultset then? You can do that in the XUL, and pass legacy: true in the options you give the registerArea call, and that should work. Can you explain why it doesn't? I'm not sure if we could make registerToolbar not error out for unknown areas and store stuff until you call registerArea. It might break some assumptions. Blair?

(In reply to :Gijs Kruitbosch from comment #15) > (In reply to Mike Kaply (:mkaply) from comment #14) > > > It would have been better to file a new bug rather than morphing this one for each subsequent issue you discover in migrating your code. :-\ > > > > No because they are all the same problem. registerArea is undocumented. > > That's orthogonal to the issue you're currently having. I agree that we > should document CustomizableUI, but that's not what you filed a bug about. > You're frustrated, and I get that, but I don't think filing bugs for things > which are bugs rather than some kind of "help me fix my add-on within the > bugzilla comments system" bug is such a terrible requirement. If you > disagree, maybe you should just wait until we document. No, because if I don't port it now, I might be stuck with the bugs. We're supposed to do this stuff on Nightly, remember? The whole point of rapid release is to get started as early as possible so we can find things that are broken as early as possible. You should have already documented this. The registerArea concept has been around for months. > > > > So in principle the assumption is you should call registerArea before the toolbar is appended to the DOM. I am confused though - how did this work before but not now? That makes no sense, you're calling the same method at the same time, so this must have been broken before you tried adding stuff to the registerArea call. Is that correct? > > > > When I first starting porting this add-on, registerArea didn't work in > > onLoad. > > Define "didn't work". Same error? Different error? > > > Now you're telling me I need to pass my defaultset to registerArea, so it > > has to be in onLoad, since that's where my defaultset is created. > > > > I'm creating defaultset dynamically. This is a reasonably common thing to do > > in an add-on because the defaultset stuff in XUL is a completely screwed up > > concept. > > > > To create the initial defaultset, I setup my toolbar the way I want it on > > firstrun and then query the items on the toolbar and dynamically set > > defaultset. > > > > My bet is this is what extension developer does. > > Actually, I don't understand this, nevermind my buying that it's what > everyone does. You're setting the default contents of the toolbar at > runtime, but you put stuff in the toolbar using an overlay, ie at > design/compile time or however you want to call that. So you must know what > goes into the toolbar at design/compile time. Why don't you set the > defaultset then? You can do that in the XUL, and pass legacy: true in the > options you give the registerArea call, and that should work. Can you > explain why it doesn't? No, it's more complicated then that. We don't use toolbarpaletteitems at all. And we don't use defaultset in the XUL. We wanted the ability to create defaultset on the fly and that's what we've done. We have an array that can be modified so that even though the items are on our toolbar, they aren't technically defaultset. Note, that what you've built here is actually BETTER for what we're doing, because you allow defaultset to be set in code versus the XUL attribute, so it might work better in the long run. I'm looking into how I can modify my code to work in your model. > > I'm not sure if we could make registerToolbar not error out for unknown > areas and store stuff until you call registerArea. It might break some > assumptions. Blair?

(In reply to Mike Kaply (:mkaply) from comment #16) > (In reply to :Gijs Kruitbosch from comment #15) > > (In reply to Mike Kaply (:mkaply) from comment #14) > > > > It would have been better to file a new bug rather than morphing this one for each subsequent issue you discover in migrating your code. :-\ > > > > > > No because they are all the same problem. registerArea is undocumented. > > > > That's orthogonal to the issue you're currently having. I agree that we > > should document CustomizableUI, but that's not what you filed a bug about. > > You're frustrated, and I get that, but I don't think filing bugs for things > > which are bugs rather than some kind of "help me fix my add-on within the > > bugzilla comments system" bug is such a terrible requirement. If you > > disagree, maybe you should just wait until we document. > > No, because if I don't port it now, I might be stuck with the bugs. We're > supposed to do this stuff on Nightly, remember? The whole point of rapid > release is to get started as early as possible so we can find things that > are broken as early as possible. We'll very likely be on Nightly at least until 29 (ie backout for Aurora 28), which will move to Aurora later because of Christmas, so there is time. > > You should have already documented this. The registerArea concept has been > around for months. And we've continued to refine stuff for months, and are still doing so. There aren't very many add-ons that add customizable toolbars wholesale. Most add-ons only add buttons, and that mostly works as it did before. We're still much too busy actually fixing bugs. > We don't use toolbarpaletteitems at all. And we don't use defaultset in the XUL. What do you mean by "we don't use toolbarpaletteitems"? Those are the wrapper thingies that wrap toolbar items when customizing. I'm assuming you mean something else... otherwise I don't see how that's related.

> There aren't very many add-ons that add customizable toolbars wholesale. Most add-ons only add buttons You're kidding, right? There are tons of toolbars out there, all customizable. > What do you mean by "we don't use toolbarpaletteitems"? Those are the wrapper thingies that wrap toolbar items when customizing. I'm assuming you mean something else... otherwise I don't see how that's related. No, that's exactly what I mean. You only need toolbarpaletteitems in your XUL if you're setting defaultset in XUL. The other way to create a customizable toolbar is to expicitly add a "removable" attribute to your toolbarbutton/toolbaritems and set the defaultset dynamically.

(In reply to Mike Kaply (:mkaply) from comment #18) > > There aren't very many add-ons that add customizable toolbars wholesale. Most add-ons only add buttons > > You're kidding, right? There are tons of toolbars out there, all > customizable. Fine, there aren't very many /popular/ add-ons that add toolbars wholesale, compared to how many add-ons just add a button or two separately. It's not the biggest factor in add-on compat, compared to single toolbarbuttons. > > What do you mean by "we don't use toolbarpaletteitems"? Those are the wrapper thingies that wrap toolbar items when customizing. I'm assuming you mean something else... otherwise I don't see how that's related. > > No, that's exactly what I mean. You only need toolbarpaletteitems in your > XUL if you're setting defaultset in XUL. You don't need to do anything with toolbarpaletteitems, period. The old/new customize stuff creates those if your toolbar is customizable, when entering the customize dialog/mode. It has nothing to do with defaultset. > The other way to create a customizable toolbar is to expicitly add a > "removable" attribute to your toolbarbutton/toolbaritems and set the > defaultset dynamically. This also doesn't make sense. Items in a toolbar will be removable=false, items in the toolbox's palette will be removable=true, by default, unless (in either case) you set the attribute yourself.

Sorry, I was being stupid. It's still early. I meant we don't use toolbarpalette. The documented way to add buttons is to put them in the toolbarpalette and then use defaultset. https://developer.mozilla.org/en-US/docs/XUL/School_tutorial/Adding_Toolbars_and_Toolbar_Buttons That's what most toolbars do. We simply add the items to our toolbar directly and set removable="true" (instead of using the palette).

(In reply to :Gijs Kruitbosch from comment #10) > 2) call registerArea(toolbarID, {legacy: true}); and set the defaultset > attribute correctly on your toolbar (basically, the same as you would have > done before) I'm sorry, because I just realized this is wrong. You probably still want to set legacy: true because it will mean we will honor the user's previous currentset attribute (ie previous customizations) and continue to store them in case they switch their profile (back) to non-Australis versions.

(In reply to :Gijs Kruitbosch from comment #21) > (In reply to :Gijs Kruitbosch from comment #10) > > 2) call registerArea(toolbarID, {legacy: true}); and set the defaultset > > attribute correctly on your toolbar (basically, the same as you would have > > done before) > I'm sorry, because I just realized this is wrong. You probably still want to > set legacy: true because it will mean we will honor the user's previous > currentset attribute (ie previous customizations) and continue to store them > in case they switch their profile (back) to non-Australis versions. We've already established that legacy won't work for us because there is no defaultset in our XUL. We set it dynamically, and we have to call registerArea before our DOM node is created. In what world is a user going to switch back and forth between Australis and non Australis? This is Rapid Release...

(In reply to Mike Kaply (:mkaply) from comment #22) > (In reply to :Gijs Kruitbosch from comment #21) > > (In reply to :Gijs Kruitbosch from comment #10) > > > 2) call registerArea(toolbarID, {legacy: true}); and set the defaultset > > > attribute correctly on your toolbar (basically, the same as you would have > > > done before) > > I'm sorry, because I just realized this is wrong. You probably still want to > > set legacy: true because it will mean we will honor the user's previous > > currentset attribute (ie previous customizations) and continue to store them > > in case they switch their profile (back) to non-Australis versions. > > We've already established that legacy won't work for us because there is no > defaultset in our XUL. We set it dynamically, and we have to call > registerArea before our DOM node is created. Hmm? You can pass {legacy: true, defaultPlacements: [...]}, that works. > In what world is a user going to switch back and forth between Australis and > non Australis? This is Rapid Release... Actually, you'd be surprised about the number of bugreports of people that use the same profile for release/beta/aurora/nightly(/ux, when this still lived on a branch). So yeah. We (try to) deal with it.

OK, so at this point, I think what this bug has morphed into is: TOOLBAR should be the default for registerArea. and can registerArea be called later, or does it have to be before the DOM Node is created. I've got my stuff mostly working via a combination of legacy, defaultPlacements and TOOLBAR as the type.

(In reply to Mike Kaply (:mkaply) from comment #24) > OK, so at this point, I think what this bug has morphed into is: > > TOOLBAR should be the default for registerArea. That's bug 941561. > can registerArea be called later, or does it have to be before the DOM Node > is created. Still waiting for Blair about whether we could fix this without breaking assumptions.

Hm, I think that should be doable. I don't think anything explicitly relies on registerToolbarNode() to not be lazy. So, in registerToolbarNode(), if the area isn't registered, store the information away somewhere and immediately return. Then in registerArea, check if any nodes have already been attempted to be registered for the new area - and register them then.

Should be fixed as part of bug 940013. Needinfo'ing so I don't forget to resolve when that gets merged.

Per bug 940013