See discussion in https://phabricator.services.mozilla.com/D131063

If the page script wants to e.g. enable/disable buttons on the <xul:dialog> onload, this is not possible, since the page script event listener for onload will get added (so executed) before the dialog has has connected.

A workaround is to place the script after </dialog> but it's not pretty

A) with the script placed after dialog
xxxmagnus loaded customElements.js
xxxmagnus dialog constructor()
xxxmagnus dialog.connectedCallback()!
xxxmagnus executing markByDate.js
xxxmagnus dialog.connectedCallback()!
xxxmagnus ... connected!
xxxmagnus MozDialog - DOMContentLoaded event
xxxmagnus DOMContentLoaded event (markByDate.js)
xxxmagnus MozDialog - load event
xxxmagnus load event (markByDate.js)

B) with script in head - bug 1708090 may be playing in
xxxmagnus loaded customElements.js
xxxmagnus executing markByDate.js
xxxmagnus dialog constructor()
xxxmagnus DOMContentLoaded event (markByDate.js)
xxxmagnus MozDialog - DOMContentLoaded event
xxxmagnus load event (markByDate.js)
xxxmagnus MozDialog - load event
xxxmagnus dialog.connectedCallback()!
xxxmagnus ... connected!

Hi Magnus, what do you think might be a solution here?

Would you like an event that gets fired at a known time in the dialog startup code so you can hook in to this._l10nButtons?

It seems like if in postLoadInit() we fired a beforeload event or something similar this would normalize the issue you were seeing with the location of the script tag affecting the order?

Bug 1796408 made the problem worse.
With the attached patch things seem to work as expected. Thunderbird try was happy and it didn't break any tests on mozilla-central either. https://treeherder.mozilla.org/#/jobs?repo=try&revision=32b9faf99a0d4b06f939847b256bfc5ff025bedf

To reproduce, apply patch and in the console do something like:

var args = Cc["@mozilla.org/hash-property-bag;1"].createInstance(Ci.nsIWritablePropertyBag2);
args.setPropertyAsUint32("modalType", Services.prompt.MODAL_TYPE_WINDOW);
args.setPropertyAsAUTF8String("title", "FOO BAR");
args.setPropertyAsAUTF8String("promptType", "alert");
window.openDialog("chrome://global/content/commonDialog.xhtml", "", "chrome,modal,centerscreen", args);

(In reply to Magnus Melin [:mkmelin] from comment #7)

Created attachment 9301289 [details]
WIP: Bug 1752288 - debug onload happening before connectedCallback()

To reproduce, apply patch and in the console do something like:

var args = Cc["@mozilla.org/hash-property-bag;1"].createInstance(Ci.nsIWritablePropertyBag2);
args.setPropertyAsUint32("modalType", Services.prompt.MODAL_TYPE_WINDOW);
args.setPropertyAsAUTF8String("title", "FOO BAR");
args.setPropertyAsAUTF8String("promptType", "alert");
window.openDialog("chrome://global/content/commonDialog.xhtml", "", "chrome,modal,centerscreen", args);

Nika or Olli, can you help? Something is weird with how we fire load events in this testcase. From the phab patch:

I converted commonDialog.xhtml to top level html to get a test case.

It turns out it's the Fluent <link> elements that cause the difference in behavior.
With the <link link="localization"> removed or commented out like i this patch, the loading behavior is not as expected. Put it back, and onload happens last as expected.

So presumably the link elements blocking load is because they require additional network requests that delay the load event (as it won't fire until all resources in the doc have loaded). So that would explain why those references would make a difference.

If defer worked, then the scripts would be loaded at the end and the problem also wouldn't happen (based on comments in the bug suggesting putting the script after </dialog>). (see bug 1708090)

I dug into the DCL and load and custom element connectedCallback event firing code a little bit but am too out of my depth to quickly see why we would be OK with firing load here before the custom element's connectedCallback runs. I did notice that connectedCallback gets called using a microtask, but dialog.js is loaded directly by customElements.js and registers the custom element before DCL is complete, so I wouldn't have expected it to be possible for the load event to fire before the custom element's connectedCallback or constructors fired. If I set breakpoints in CustomElementRegistry.cpp to notice when custom element connected callbacks fire, the bug actually isn't easy to reproduce, presumably because it's quite that racy. But I'm not sure how to dig in further than that.

I did notice, as an aside, that we do a bunch of script loading using the subscript loader, so I wondered if those loads block the load event of the document correctly as they're using the scriptloader rather than being loaded from the document directly - but from dump logging in the JS, the dialog.js code gets executed before DCL, so that shouldn't be the problem, I would have thought?

https://hg.mozilla.org/mozilla-central/rev/cc512538cd36
https://hg.mozilla.org/mozilla-central/rev/8a6e59592f94

We discussed this a bit on matrix, and unfortunately I am very unfamiliar with how custom elements work, so I don't know if I'll be able to provide much insight here. Perhaps :smaug will know more about how custom elements are handled, and how that might interact with xul prototypes (or whatever is changing the behaviour).

Looks like the changes were done in JS.
(Custom elements use custom element reaction timing. Not bound to load event at all.)

Per discussion on matrix/zoom, renewing needinfo - the fundamental issue of connectedCallback firing after load is not yet fixed, we just worked around it in some frontend code, so it would be good to understand why that is happening. Olli suggested this is probably due to the XUL prototype cache stuff.

Right, I found the patch doesn't fix things 100%. I tried to remove the workarounds we had for it, but there will still be issues if one tries to e.g. set a dialog button to disabled onLoad - since the button will be added by the dialog connectedCallback, and onLoad runs before that. Adding the Fluent <link> to the dialog will work around this issue.

I hadn't realized the code does its own connectedCallback handling (because of delayConnectedCallback()). So this has nothing to do with custom elements, I think.

xul:dialog itself is rather weird because it does asynchronous translateElements in its load event listener. So only once that is done, everything is really up and running.

(In reply to Olli Pettay [:smaug][bugs@pettay.fi] from comment #17)

I hadn't realized the code does its own connectedCallback handling (because of delayConnectedCallback()). So this has nothing to do with custom elements, I think.

But AIUI the logging in comment 0 shows that that isn't relevant here. IOW, I think the bug still reproduces even if you commented out that handling.

xul:dialog itself is rather weird because it does asynchronous translateElements in its load event listener. So only once that is done, everything is really up and running.

Why would this matter? That's in load, which should be after the connectedCallback etc. has already fired, because by the time DOMContentLoaded fires all the connectedcallbacks for stuff directly in the DOM should have fired. The translateElements call only modifies the buttons, and the connectedCallback is an ancestor in the DOM, so I don't see how it'd be affected by fluent changing things on the buttons, either.