1752288 - no proper way to let page scripts interact with xul dialog internals on load

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect, P3)

Description

[Magnus Melin [:mkmelin]]

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!

Comment 1

[Mark Striemer [:mstriemer]]

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?

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:mstriemer, since the bug has recent activity, could you please find another way to get the information or close the bug as INCOMPLETE if it is not actionable?

For more information, please visit auto_nag documentation.

Comment 3

[Suhaib Mujahid [:suhaib]]

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: Bug 1752288 - make xul dialog postload events handled only after the element was fully loaded. r=emilio,Gijs

Comment 5

[Magnus Melin [:mkmelin]]

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

Comment 6

[Magnus Melin [:mkmelin]]

Attachment: Bug 1752288 - Call dialog _sizeToPreferredSize() even if there are no Fluent buttons, so such dialogs get proper sizing. r=emilio

Comment 7

[Magnus Melin [:mkmelin]]

Attachment: 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);

Comment 8

[:Gijs (out for now; he/him)]

(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?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc512538cd36
Call dialog _sizeToPreferredSize() even if there are no Fluent buttons, so such dialogs get proper sizing. r=emilio

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a6e59592f94
make xul dialog postload events handled only after the element was fully loaded. r=emilio,Gijs

Comment 11

[Sandor Molnar[:smolnar]]

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

Comment 12

[Nika Layzell [:nika] (ni? for response)]

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).

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 14

[:Gijs (out for now; he/him)]

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.

Comment 15

[Intermittent Failures Robot]

13 failures in 3445 pushes (0.004 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• comm-beta: 13

Platform and build breakdown:

• windows10-32-2004-shippable-qr: 3
  • opt: 3
• linux1804-64-shippable-qr: 3
  • opt: 3
• windows10-64-2004-shippable-qr: 4
  • opt: 4
• macosx1015-64-shippable-qr: 3
  • opt: 3

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1752288&startday=2022-11-07&endday=2022-11-13&tree=all

Comment 16

[Magnus Melin [:mkmelin]]

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.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 18

[:Gijs (out for now; he/him)]

(In reply to Olli Pettay [:smaug][[e-mail]] 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.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

dialog code has changed over time, and these days connectedCallback is run well before load event. Does anyone have some other testcase?
And if there is some issue still, could someone try if adding something like https://searchfox.org/mozilla-central/rev/bf8c7a7d47debb1c22b51a72184d5c703ae57588/dom/xml/nsXMLContentSink.cpp#512 to https://searchfox.org/mozilla-central/rev/bf8c7a7d47debb1c22b51a72184d5c703ae57588/dom/prototype/PrototypeDocumentContentSink.cpp#663 helps?