1797389 - commondialog doesn't wait for title l10n properly.

Status: RESOLVED DUPLICATE of bug 1819664

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

Attachment: Screenshot on release

data:text/html,<button onclick="confirm('foo')">Foo</button>

On Linux at least on my machine, the first time in the lifetime of the browser that you click the button the content might be slightly cropped.

I dug a bit and it turns out it is because even though this line gets executed before we translate the document height (in DOMContentLoaded) the actual mutations are hooked to the refresh driver, so we don't get to actually inject the text until later.

Something like this fixes it, but unclear if it's a reasonable fix or we should do something else, thoughts?

diff --git a/toolkit/modules/SubDialog.sys.mjs b/toolkit/modules/SubDialog.sys.mjs
index 9a707af60f409..843d8af6e149a 100644
--- a/toolkit/modules/SubDialog.sys.mjs
+++ b/toolkit/modules/SubDialog.sys.mjs
@@ -442,6 +442,9 @@ SubDialog.prototype = {
     // ensure that all of the l10n is done.
     if (target.contentDocument.l10n) {
       await target.contentDocument.l10n.ready;
+      await new Promise(r => target.contentWindow.requestAnimationFrame(() =>{
+        target.contentWindow.requestAnimationFrame(r);
+      }));
     }
 
     // Some subdialogs may want to perform additional, asynchronous steps during initializations.

The text content affects the height of the dialog because of this align-items: baseline ftr:

https://searchfox.org/mozilla-central/rev/59f0bf3c13dd455d9f5415b89178de701ea6b850/toolkit/components/prompts/content/adjustableTitle.js#19

Gijs, thoughts?

Comment 1

[:Gijs (he/him)]

Would waiting for promiseDocumentFlushed also work, or no?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Not if something explicitly flushes layout sync.

Comment 3

[:Gijs (he/him)]

So should we just take the 2x rAF patch? I'm not sure if I'm following... like, are there other options? Maybe it's because I'm not sure what this means:

the actual mutations are hooked to the refresh driver, so we don't get to actually inject the text until later.

Which mutations, and what does "hooked to the refresh driver" mean?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Possibly... We'd at least need one rAF of waiting. But of course in the meantime more mutations might've happened.

With mutations I mean l10n-id mutations and so on. Mutations to those attributes (like data-l10n-id etc) go through here, which basically means they're kept in a set of pending elements until the next refresh driver tick (before styling). So roughly the RAF timing (link).

Comment 5

[:Gijs (he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

Possibly... We'd at least need one rAF of waiting. But of course in the meantime more mutations might've happened.

With mutations I mean l10n-id mutations and so on. Mutations to those attributes (like data-l10n-id etc) go through here,

Hm, do those not end up blocking l10n.ready if they happen before that promise has initially resolved? That seems bad.

IOW, would an alternative solution be moving some of the mutations forward so they (more reliably) happen sooner, so that l10n.ready includes those?

(I continue to lament the async nature of fluent for this stuff - it makes it very hard to reliably get l10n into the doc in a way that doesn't lead to flashing/resizing or broken measurements as is happening here. Like, if ^^ doesn't work then we can go the rAF route, but we'll just hit the same problem again and again in different places.)

Comment 6

[Emilio Cobos Álvarez (:emilio)]

No, they don't afaict. l10n.ready resolves as soon as the initial translation is complete: https://searchfox.org/mozilla-central/rev/59f0bf3c13dd455d9f5415b89178de701ea6b850/dom/l10n/DocumentL10n.cpp#119

We could add something else so that these mutations do block, I agree the current behavior is unfortunate.

Comment 7

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

The severity field is not set for this bug.
:tspurway, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 8

[:Gijs (he/him)]

I think bug 1788191 and this one are the same and they were both fixed by bug 1819664 or its friends, in that I cannot reproduce either anymore. Emilio, does that seem plausible, or are you still able to repro?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Yes, that seems very plausible. Can't repro anymore either.