973698 - Austalis - addWidgetToArea inserting button at the wrong location

Status: RESOLVED INVALID

(Firefox :: Toolbars and Customization, defect)

Description

[Mike Kaply [:mkaply]]

Attachment: Extension to demonstrate problem

Our extension has a "minimized mode" where we reduce the number of button. When you minimize then maximized, the button is put in the wrong place.

The code is calling CustomizableUI.addWidgetToArea against our toolbar with the correct button and ID with a position of 2, but it is being placed at the end.

Unfortunately any effort I put into making this a reduced testcase causes it to work. It also sometimes simply starts working after a while.

But in my testing, it always fails with a new profile.

To recreate:

Install XPI.
Click the arrow next to the Email button. Select Minimize Mailcheck.
Now click the arrow next to the Email button again. Select Maximize Mailcheck.
The button goes to the far right of the toolbar. It should be back in its original place.

Comment 1

[:Gijs (he/him)]

(In reply to Mike Kaply (:mkaply) from comment #0)
> Created attachment 8377267 [details]
> Extension to demonstrate problem
> 
> Our extension has a "minimized mode" where we reduce the number of button.
> When you minimize then maximized, the button is put in the wrong place.
> 
> The code is calling CustomizableUI.addWidgetToArea against our toolbar with
> the correct button and ID with a position of 2, but it is being placed at
> the end.
> 
> Unfortunately any effort I put into making this a reduced testcase causes it
> to work. It also sometimes simply starts working after a while.
> 
> But in my testing, it always fails with a new profile.
> 
> To recreate:
> 
> Install XPI.
> Click the arrow next to the Email button. Select Minimize Mailcheck.
> Now click the arrow next to the Email button again. Select Maximize
> Mailcheck.
> The button goes to the far right of the toolbar. It should be back in its
> original place.

Have you tried just setting a breakpoint in addWidgetToArea and figuring out what's happening? I'm not sure why you're filing a bug with uncertain STR that sounds like something that could just be debugged...

(open the toolbox, open the options, tick 'enable remote debugging' and 'enable chrome debugging'; developer tools > Browser Toolbox, filter for "CustomizableUI.jsm", set a breakpoint in CustomizableUIInternal::addWidgetToArea)

Comment 2

[Mike Kaply [:mkaply]]

So the problem is that the separator in our toolbar has no ID when it is first installed. I've verified this with the DOM inspector.

 In findInsertionPoints.

This code:
    if (aNextNodeId) {
      nextNode = aAreaNode.customizationTarget.getElementsByAttribute("id", aNextNodeId)[0];
    }
    return [aAreaNode.customizationTarget, nextNode];
  },

is called with aNextNodeId ofcustomizableui-special-separator1 and it returns null.

After restart, using the DOM inspector, the separator now has an ID of customizableui-special-separator1.

The CustomizableUI.jsm code is assuming that this separator got some custom ID somehow.

Comment 3

[:Gijs (he/him)]

(In reply to Mike Kaply (:mkaply) from comment #2)
> So the problem is that the separator in our toolbar has no ID when it is
> first installed. I've verified this with the DOM inspector.
> 
>  In findInsertionPoints.
> 
> This code:
>     if (aNextNodeId) {
>       nextNode = aAreaNode.customizationTarget.getElementsByAttribute("id",
> aNextNodeId)[0];
>     }
>     return [aAreaNode.customizationTarget, nextNode];
>   },
> 
> is called with aNextNodeId ofcustomizableui-special-separator1 and it
> returns null.
> 
> After restart, using the DOM inspector, the separator now has an ID of
> customizableui-special-separator1.
> 
> The CustomizableUI.jsm code is assuming that this separator got some custom
> ID somehow.

How do you insert the separator?

Comment 4

[Mike Kaply [:mkaply]]

(In reply to :Gijs Kruitbosch from comment #3)
> How do you insert the separator?

It's passed it into defaultPlacements:

function initAustralis() {
  var defaultset = [];
  for (let i in brand.toolbar.items) {
    if (i == "separator") {
      defaultset.push("separator");
    } else if (i == "spring") {
      defaultset.push("spring");
    } else {
      defaultset.push("united-" + i);
    }
  }

  CustomizableUI.registerArea("united-toolbar",
      { type: CustomizableUI.TYPE_TOOLBAR,
         legacy: true, // support defaultset XUL attribute etc.
         defaultPlacements: defaultset});
}

Comment 5

[:Gijs (he/him)]

Yes, we change the IDs of separator, spring, and spacer elements, because obviously they can't all have the same ID... this happened in pre-Australis versions as well, except they didn't update default/currentset, which led to other bugs...

However, you don't rely on us inserting the separator, as far as I can tell - you insert them yourself in fixToolbarSets. I don't know why, but that's certainly the cause of the problem: the separator doesn't have the right ID (in fact, it has no ID at all), and so CustomizableUI can't find it and inserts in the wrong place.

The fact that we don't insert in front of the next node but at the end is bug 976792. However, the root cause here is that you create nodes yourself without IDs and/or with IDs that don't match the things which are in the default placements. I'm resolving this as INVALID because of this.


While I was here, I noticed this bit of code:

function addButton(toolbarID, button, insertAfterID) {
  var toolbar = E(toolbarID);
  if (toolbar.id == "united-toolbar") {
    // Go through the brand list and figure out a location for the button
    // We work backwards from the buttons location in the array
    var brandItems = [];
    for (let i in brand.toolbar.items) {
      brandItems.push("united-" + i);
    }

Which seems to be wrong, too, because unlike the initAustralis function, it's prefixing all the items, including the separators, springs etc. (even if that is of no consequence for this particular issue, it may bite you later)

Comment 6

[Mike Kaply [:mkaply]]

(In reply to :Gijs Kruitbosch from comment #5)
> Yes, we change the IDs of separator, spring, and spacer elements, because
> obviously they can't all have the same ID... this happened in pre-Australis
> versions as well, except they didn't update default/currentset, which led to
> other bugs.
>
> However, you don't rely on us inserting the separator, as far as I can tell
> - you insert them yourself in fixToolbarSets.

I can't rely on you to insert them because I want code that works preAustralis.

> I don't know why, but that's
> certainly the cause of the problem: the separator doesn't have the right ID
> (in fact, it has no ID at all), and so CustomizableUI can't find it and
> inserts in the wrong place.

That's not how defaultset worked from the beginning. defaultset contains the IDs of toolbar items, mixed with the names of separators, springs and spacers. You can see this in the Firefox code and Thunderbird code and SeaMonkey code.

http://mxr.mozilla.org/mozilla-release/source/browser/base/content/browser.xul#1135
http://mxr.mozilla.org/comm-release/source/mail/base/content/mailWindowOverlay.xul#3295
http://mxr.mozilla.org/comm-release/source/mail/components/addrbook/content/addressbook.xul#590
http://mxr.mozilla.org/comm-release/source/mail/components/compose/content/messengercompose.xul#794
http://mxr.mozilla.org/comm-release/source/suite/browser/navigator.xul#191
http://mxr.mozilla.org/comm-release/source/suite/mailnews/compose/messengercompose.xul#439

No one in the past ever put IDs on their spacers and separators, not even Firefox. IDs got randomly assigned on second invocation.

> The fact that we don't insert in front of the next node but at the end is
> bug 976792. However, the root cause here is that you create nodes yourself
> without IDs and/or with IDs that don't match the things which are in the
> default placements. I'm resolving this as INVALID because of this.

No, the root cause here is that Australis has placed an incredibly heavy reliance on everything having an ID in order for all the APIs to work. Since you are relying so heavily on the IDs of the widgets (which wasn't the case before Australis), you should assign IDs to separators, spacers and springs that don't have IDs when registerArea is called.

Comment 7

[:Gijs (he/him)]

(In reply to Mike Kaply (:mkaply) from comment #6)
> (In reply to :Gijs Kruitbosch from comment #5)
> > Yes, we change the IDs of separator, spring, and spacer elements, because
> > obviously they can't all have the same ID... this happened in pre-Australis
> > versions as well, except they didn't update default/currentset, which led to
> > other bugs.
> >
> > However, you don't rely on us inserting the separator, as far as I can tell
> > - you insert them yourself in fixToolbarSets.
> 
> I can't rely on you to insert them because I want code that works
> preAustralis.

And? I just ran the following on Holly (ie Aurora-sans-Australis) on a clean profile from a browser-privileged scratchpad:

let t = document.createElement("toolbar");
t.id = "test-defaultset-assignment";
t.setAttribute("defaultset", "print-button,spacer,cut-button");
t.setAttribute("customizable", "true");
t.setAttribute("mode", "icons");
gNavToolbox.appendChild(t);

And the buttons as well as the spacer were inserted as expected.


> > The fact that we don't insert in front of the next node but at the end is
> > bug 976792. However, the root cause here is that you create nodes yourself
> > without IDs and/or with IDs that don't match the things which are in the
> > default placements. I'm resolving this as INVALID because of this.
> 
> No, the root cause here is that Australis has placed an incredibly heavy
> reliance on everything having an ID in order for all the APIs to work.

This was already the case, and your own code relies on them, too. The emotive "incredibly heavy" doesn't really mean anything here.

> Since
> you are relying so heavily on the IDs of the widgets (which wasn't the case
> before Australis), you should assign IDs to separators, spacers and springs
> that don't have IDs when registerArea is called.

You mean registerToolbarNode, presumably? registerArea doesn't know anything about the DOM.

But more to the point, this seems like a massive edgecase - you'll notice that the add-on bar from your example never had an actual spring inside it, either. Seems like most people just rely on the attributes and Firefox to do the Right Thing as far as inserting items goes, and you're the odd one out. I don't think we should change Firefox to cater to that usecase.

Comment 8

[Mike Kaply [:mkaply]]

(In reply to :Gijs Kruitbosch from comment #7)
> > I can't rely on you to insert them because I want code that works
> > preAustralis.
> 
> And? I just ran the following on Holly (ie Aurora-sans-Australis) on a clean
> profile from a browser-privileged scratchpad:
> 
> let t = document.createElement("toolbar");
> t.id = "test-defaultset-assignment";
> t.setAttribute("defaultset", "print-button,spacer,cut-button");
> t.setAttribute("customizable", "true");
> t.setAttribute("mode", "icons");
> gNavToolbox.appendChild(t);
> 
> And the buttons as well as the spacer were inserted as expected.

But the IDs aren't consistent across invocations.

If you put a spacer in a defaultset, the ID preAustralis is dynamically generated every time the toolbar is created, at every Firefox startup.

In my case, even if I add IDs to the separators I'm creating, it's irrelevant, because on subsequent invocations of Firefox, the ID is randomly generated.

The only way that your scenario works is if the separators, springs and spacers are in XUL, not if they are dynamically added to the toolbar, because they are only added the first time with ID - subsequent times that are automatically generated by Firefox with the random IDs.

Comment 9

[Mike Kaply [:mkaply]]

I guess what this boils down to is if there is some way for my initial toolbar creation to have the same IDs that will get generated by Australis on subsequent invocations when it uses defaultset...

Comment 10

[:Gijs (he/him)]

(In reply to Mike Kaply (:mkaply) from comment #9)
> I guess what this boils down to is if there is some way for my initial
> toolbar creation to have the same IDs that will get generated by Australis
> on subsequent invocations when it uses defaultset...

Yes: don't manually create your own separators. :-)

Comment 11

[Mike Kaply [:mkaply]]

> Yes: don't manually create your own separators. :-)

How do I let Australis create them without defaultset?

The issue is that I have an array of what I want to be in my toolbar, and I'm trying to dynamically create it and it has separators...

Comment 12

[:Gijs (he/him)]

(In reply to Mike Kaply (:mkaply) from comment #11)
> > Yes: don't manually create your own separators. :-)
> 
> How do I let Australis create them without defaultset?
> 
> The issue is that I have an array of what I want to be in my toolbar, and
> I'm trying to dynamically create it and it has separators...

You could have the defaultset (although it's unnecessary if you're calling registerArea in time), you should just not call fixToolbarSets which manually loops through and creates all the nodes itself. I tried to say this back in comment 5...

Comment 13

[Mike Kaply [:mkaply]]

> You could have the defaultset (although it's unnecessary if you're calling registerArea in time), you should just not call fixToolbarSets which manually loops through and creates all the nodes itself. I tried to say this back in comment 5...

I tried that, but it doesn't work for upgrades.

The issue is that that array could change, and we want to modify the toolbar to look like the new array.

So changing defaultset isn't enough, since that only handles the initial look. We need to modify the existing toolbar to look like the new array (hide stuff, move stuff around, etc.).

Is there a concept of currentset in Australis? To somehow update the information as to what belongs in the area? That's what we were doing preAustralis.

Comment 14

[Mike Kaply [:mkaply]]

So it looks like unregistering the area with destroyPlacements and then reregistering might work?

Comment 15

[:Gijs (he/him)]

(In reply to Mike Kaply (:mkaply) from comment #14)
> So it looks like unregistering the area with destroyPlacements and then
> reregistering might work?

It might. That'd destroy your users' preferences, though. (I'm not really sure why this toolbar is even customizable, AFAICT it makes pretty strong assumptions about the placements being what it wants them to be... anyway, different discussion) Alternatively, call removeWidgetFromArea for each widget returned in getWidgetIdsInArea, then call addWidgetToArea for each new widget in your new desired array of default placements.

We listen to the currentset attribute once on import of a pre-Australis profile. Then we keep them updated pretty much like the old system did in case you ever go back to pre-Australis. Setting the attribute yourself has no effect.

There is also still a currentSet property setter. It won't let you do arbitrary things (e.g. remove items which aren't removable), but as much as possible, it'll try to do what you asked for and insert/move/remove items in/to/from the right places. We will work towards removing that setter, though, so depending on it might be unwise.