941903 - Australis: toolbar button in new menu panel isn't found by getElementById on window load for initialization

Status: RESOLVED WONTFIX

(Firefox :: Toolbars and Customization, defect)

Description

[Carlos [nohamelin]]

In my Simple Locale Switcher extension:
https://addons.mozilla.org/firefox/addon/simple-locale-switcher/

I have an classic overlay for browser.js that adds, on window load, some dinamic menuitems to a xul toolbar button ("menu" type). The simplified code is as follows:

///////////////////////////////////////////////////////////

myextension.onLoad = function() {
  ...
  let button = document.getElementById("myextension-button");
  if (button) {
    ...
  }
}

window.addEventListener("load", myextension.onLoad);

///////////////////////////////////////////////////////////

getElementById() is expected to fail if the button is on the toolbar palette, but it's returning *null* when the button is placed in the menu panel too, skiping its initialization. Handlers for some other events (e.g. aftercustomization, observing prefs) recreating later the button's contents seem to work fine.



I can to think a few ways for to bypass it, but I guess that many other extensions are doing this and they can be affected; while trying other (working) add-ons from my dev profile I found that the button provided by Quick Locale Switcher
https://addons.mozilla.org/firefox/addon/quick-locale-switcher/

kicks it too.

Comment 1

[:Gijs (he/him)]

Jared, do you think we could/should be doing something to fix this? I suspect it will indeed hit many add-ons, and I'm wondering if there's anything we can do, apart from not lazy-loading the panel menu, which would be sad. I guess we could preload only the XUL type widgets, but I'm not sure that's much better... (because then we have to do another buildArea pass afterwards...)

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

See bug 941990 for another bug that is coming from us not preloading the panel (although it doesn't necessarily require us to preload the panel to fix it).

I'm a bit worried about the perf impact of us preloading the panel, although now that the perf numbers have stabilized we could try this and see what kind of perf hit we get from it.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

So I just tested this, and I'm surprised this works, but getting to the node via CustomizableUI works even if the area hasn't been loaded (since it builds the widget and the stashes it in its instances cache, which I think it can use to populate the menu when it lazy-loads).

Carlos - does something like this work?:

let widgetGroup = CustomizableUI.getWidget("myextension-button");

if (widgetGroup.areaType) { // areaType should be null if the item is in the palette
  let button = widgetGroup.forWindow(window).node;
  // Do stuff with button
}

Comment 4

[Carlos [nohamelin]]

(In reply to Mike Conley (:mconley) from comment #3)
> Carlos - does something like this work?:
> 
> let widgetGroup = CustomizableUI.getWidget("myextension-button");
> 
> if (widgetGroup.areaType) { // areaType should be null if the item is in the
> palette
>   let button = widgetGroup.forWindow(window).node;
>   // Do stuff with button
> }

Some things works, but I could not access to the menupopup xul children of the button. childNodes return me null.

Comment 5

[:Gijs (he/him)]

(In reply to Carlos [:nohamelin] from comment #4)
> (In reply to Mike Conley (:mconley) from comment #3)
> > Carlos - does something like this work?:
> > 
> > let widgetGroup = CustomizableUI.getWidget("myextension-button");
> > 
> > if (widgetGroup.areaType) { // areaType should be null if the item is in the
> > palette
> >   let button = widgetGroup.forWindow(window).node;
> >   // Do stuff with button
> > }
> 
> Some things works, but I could not access to the menupopup xul children of
> the button. childNodes return me null.

This doesn't make sense. If you have a real dom element, childNodes would always be an element list. Could have 0 length, but it can't itself be null. How do the menupopup xul children end up in there?



Mike, Jared: I know that in the new API we have workarounds, but that's going to break backward compatibility. It sort of makes sense that people want this... and I'm inclined to implement something for it if it doesn't cause perf problems. They'll be hidden though, so XBL and iframes will still not do much. I wonder if that won't still break enough stuff that it won't be worth appending all the extra items.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Carlos [:nohamelin] from comment #4)
> > (In reply to Mike Conley (:mconley) from comment #3)
> > > Carlos - does something like this work?:
> > > 
> > > let widgetGroup = CustomizableUI.getWidget("myextension-button");
> > > 
> > > if (widgetGroup.areaType) { // areaType should be null if the item is in the
> > > palette
> > >   let button = widgetGroup.forWindow(window).node;
> > >   // Do stuff with button
> > > }
> > 
> > Some things works, but I could not access to the menupopup xul children of
> > the button. childNodes return me null.
> 
> This doesn't make sense. If you have a real dom element, childNodes would
> always be an element list. Could have 0 length, but it can't itself be null.
> How do the menupopup xul children end up in there?
> 
> 
> 
> Mike, Jared: I know that in the new API we have workarounds, but that's
> going to break backward compatibility. It sort of makes sense that people
> want this... and I'm inclined to implement something for it if it doesn't
> cause perf problems. They'll be hidden though, so XBL and iframes will still
> not do much. I wonder if that won't still break enough stuff that it won't
> be worth appending all the extra items.

Well, let's give it a shot and see how it affects the numbers.

Comment 7

[Carlos [nohamelin]]

Up(In reply to :Gijs Kruitbosch from comment #5)
> This doesn't make sense. If you have a real dom element, childNodes would
> always be an element list. Could have 0 length, but it can't itself be null.
> How do the menupopup xul children end up in there?

Ups, my fault. Yeah, childNodes is Ok.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mike Conley (:mconley) from comment #6)
> Well, let's give it a shot and see how it affects the numbers.

Agreed (see comment #2).

Comment 9

[:Gijs (he/him)]

If we want to do this, it needs to happen before v1.

Comment 10

[:Gijs (he/him)]

For tpaint/ts_paint checks:

baseline: remote:   https://tbpl.mozilla.org/?tree=Try&rev=56b4f4bf5f43
loading the panel immediately: remote:   https://tbpl.mozilla.org/?tree=Try&rev=abe700252eb1


Actually, this is pretty gross and heavyhanded. If this regresses perf (which I somewhat expect) then we could consider doing a lightweight thing just for XUL-type widgets, ie when we have discovered the palette, check the panel placements and just appendchild all the things. buildArea should be smart enough to deal with that. Ugly, though. :-)

Comment 11

[:Gijs (he/him)]

tpaint is just dire:

http://perf.snarkfest.net/compare-talos/index.html?oldRevs=56b4f4bf5f43&newRev=abe700252eb1&submit=true

So, this isn't going to work. :-\

I would hope that a more lightweight solution /would/ do, but I'll have to look at that tomorrow. If not, I think this will be WONTFIX + a documentation issue.

Comment 12

[:Gijs (he/him)]

Another attempt:

remote:   https://tbpl.mozilla.org/?tree=Try&rev=456bb8c9fef1


If this doesn't work, I think this should be wontfix.

Comment 13

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #12)
> Another attempt:
> 
> remote:   https://tbpl.mozilla.org/?tree=Try&rev=456bb8c9fef1
> 
> 
> If this doesn't work, I think this should be wontfix.

Compared with the run in comment 10, this is strictly worse. There might be an unrelated tpaint/ts_paint decline in Windows in the middle, but Linux and OS X 10.8 are also sadfaces. So, marking WONTFIX.

The recommended way to check that your button is in the palette would be:

!CustomizableUI.getPlacementOfWidget(myButtonId)

This will return null if it's in the palette, and an object if it's anywhere else.