Closed Bug 1738056 Opened 3 years ago Closed 2 years ago

About Firefox window appears in top left for an instant before moving to the center

Categories

(Firefox :: General, defect, P3)

Firefox 93
Desktop
macOS
defect

Tracking

()

VERIFIED FIXED
113 Branch
Accessibility Severity s2
Tracking Status
firefox-esr91 --- unaffected
firefox-esr102 --- wontfix
firefox93 --- wontfix
firefox94 --- wontfix
firefox95 --- wontfix
firefox96 --- wontfix
firefox111 --- wontfix
firefox112 --- wontfix
firefox113 --- verified

People

(Reporter: matija, Assigned: Gijs)

References

(Depends on 1 open bug, Regression)

Details

(Keywords: access, regression, Whiteboard: [fidefe-quality-foundation])

Attachments

(3 files)

Attached video firefox.mp4

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:93.0) Gecko/20100101 Firefox/93.0

Steps to reproduce:

Opened up Firefox -> About Firefox menu

Actual results:

The popup appears misplaced in top-right for a split second.

Expected results:

It should have a consistent location (e.g. in center to where it moves almost immediately anyways)

Hi Matija,

I was able to reproduce the issue on MacOS 11.6 using Firefox Nightly 95.0a1 and Firefox 93.0
I'm going to set a component in order to get this issue looked at by the development team.

Thanks for reporting!

Status: UNCONFIRMED → NEW
Component: Untriaged → Menus
Ever confirmed: true
OS: Unspecified → macOS

Based on mozregression, bug 1660392 broke this.

Zibi, can you take a look? Why has timing changed here?

More broadly, we need to be able to get l10n and then size content based on that l10n, before we start painting windows. Do we have primitives for this today? I'm not aware of them if so.

Component: Menus → General
Flags: needinfo?(zbraniecki)
Keywords: regression
Regressed by: 1660392
Hardware: Unspecified → Desktop
Summary: About Firefox popup appears in top left for an instant before moving to the center → About Firefox window appears in top left for an instant before moving to the center
Has Regression Range: --- → yes
See Also: → 1737951

Zibi, can you take a look? Why has timing changed here?

Same as with previous. We used to have a different microtask scheduling when all of l10n code was in JS. This allowed us to accrue code that has been "lucky" to work in the correct order without any guarantees of that - it was/is racy.

Now, with the switch of Fluent to Rust we had to shuffle the microtasks and many of those race conditions show up.

In case of l10n the importance of not relying on racy code is significant because the same bug (and previous bugs you NI'ed me in) will exercise the same faulty behavior if we load l10n slower than anticipated (for example loading RemoteL10n asynchronously, or loading fallback locale).

My mental model is that side effect of that js->rust work has been that we exposed those racy cases and now need to make them non-racy, but I can see how this feels like we should be able to:
a) design API that makes it harder to write racy code (not sure how)
b) catch such racy conditions before merging the work (not sure how)

More broadly, we need to be able to get l10n and then size content based on that l10n, before we start painting windows. Do we have primitives for this today? I'm not aware of them if so.

Yes, we have it on file as bug 1520659. For the better part of this year that work was blocked on the js->rust migration, and now is unblocked.

:nordzilla - You assigned yourself to that in Clickup. Are you still interested in taking it? (maybe after https://github.com/mozilla/l10nregistry-rs/issues/18 ) I think this is unblocked now.

In the meantime, there is a way to ensure correct ordering of localization vs. painting in cases where your sizing/paiting must depend on initial l10n being done:

let frag = document.getElementByid("myFrag");
await document.l10n.translateFragment(frag);
let { size, position } = getSize(frag);
paint();
Flags: needinfo?(zbraniecki) → needinfo?(enordin)

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #3)

let frag = document.getElementByid("myFrag");
await document.l10n.translateFragment(frag);
let { size, position } = getSize(frag);
paint();

OK, but as I said in the menus bug, that's really not how things work; there is no JS-accessible "paint" API, or a show/hide window API, AFAIK. We just get JS that runs when the DOM loads, and graphics/XUL/DOM code takes care of painting pretty much independently. My point is, when we built a tool to do async l10n, did we build hooks into window (and menu and other things that are widget-level windows) creation/painting that can be used to block painting / showing windows?

Flags: needinfo?(zbraniecki)
Flags: needinfo?(zbraniecki)
See Also: → 1738748

Set release status flags based on info from the regressing bug 1660392

Flags: needinfo?(enordin)

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(dtownsend)
Severity: -- → S4
Flags: needinfo?(dtownsend)

This may be fixed when bug 1746124 lands. I'd be curious to get it re-tested then.

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #8)

This may be fixed when bug 1746124 lands. I'd be curious to get it re-tested then.

This still happens on today's nightly, unfortunately.

The severity field for this bug is set to S4. However, the following bug duplicate has higher severity:

:mossop, could you consider increasing the severity of this bug to S2?

For more information, please visit auto_nag documentation.

Flags: needinfo?(dtownsend)

See bug 1735712 comment 4:

Triaging as [access-s2] due to possible injury for users with vestibular disabilities.

Severity: S4 → S2
Flags: needinfo?(dtownsend)
Keywords: access
Whiteboard: [access-s2]
Whiteboard: [access-s2] → [access-s2][fidefe-quality-foundation]

So in bug 1801456 comment 29, there was a discussion around blocking paint on l10n, which seems like it's relevant here (and in bug 1737951).

Greg, you asked if it was too late to add some. I don't think so? But I'm not sure whether it's technically difficult. Bug 1738748 might be a good place to investigate this. Pinging you and Eemeli here to add context. :-)

Flags: needinfo?(gtatum)
Flags: needinfo?(earo)

Clearing needinfo since I'm not sure there's anything actionable I can add here. I probably don't have time in the short term to really look into it, but if it's still a pain point perhaps it's worth prioritizing.

Flags: needinfo?(gtatum)
Priority: -- → P3
Depends on: 1738748

Pretty much echoing Greg's comment here. This seems like a side effect of architectural choices that ought to get fixed at a rather deep level, as in bug 1738748.

Flags: needinfo?(earo)
Component: General → Internationalization: Localization
Product: Firefox → Core

Looks like bug 1807838 didn't help for this specific problem, though the description had made me hopeful - Olli, is there more we can do here? What would it take to have stronger guarantees on the order with which things happen, especially for toplevel widgets (chrome/XUL windows, or anything with a widget-level window)? Like, can we postpone firing DOMContentLoaded until translations are complete, and postpone paint until DCL has fired, or something?

Fundamentally, there seem to be 3 things:

  1. localizing content
  2. measuring size of content + layout
  3. painting

that should be happening in that order, but in many, many, places, do not. And we keep adding hacks for it - just today I had both https://phabricator.services.mozilla.com/D171127 and https://phabricator.services.mozilla.com/D171103 in my review queue, and they're both basically variations of this problem. It's not helped, I suppose, by JS code wanting to run "once the DOM is ready" and then adding some more l10n (which usually will lead to a second l10n pass, and another round of the above).

This kind of thing keeps biting us, and forcing us to write hacky, fragile, hard-to-maintain code. :-(

(to be fair the solution in https://phabricator.services.mozilla.com/D171103 is not super hacky, but the having to manually clear out l10n + "actual" localized content, and then resetting it again, still isn't the prettiest, and although I have not yet tested, I expect still runs the risk of initially painting a tooltip with no content at all, which is certainly an issue I occasionally see with menus/panels on Windows, which subtly change height as the l10n pages in)

Flags: needinfo?(smaug)

bug 1807838 added that hack because for some reasons (unclear to me) we use that async translateElements().
Do we use that also elsewhere, and if so, why? Is the initial l10n handling not enough, and if so, why? Should l10n API be different?

The "normal" translation already does block[1][2] page(chrome) load, but it is the use of these explicitly async APIs which don't.
[1] https://searchfox.org/mozilla-central/rev/aa3ccd258b64abfd4c5ce56c1f512bc7f65b844c/dom/base/Document.cpp#4365-4369
[2] https://searchfox.org/mozilla-central/rev/aa3ccd258b64abfd4c5ce56c1f512bc7f65b844c/dom/base/Document.cpp#4424,4431

Could one write a simple helper on top of document.blockUnblockOnload() and document.l10n.ready ?

Or perhaps we could somehow reuse this array
https://searchfox.org/mozilla-central/rev/aa3ccd258b64abfd4c5ce56c1f512bc7f65b844c/dom/l10n/DocumentL10n.cpp#138
https://searchfox.org/mozilla-central/rev/aa3ccd258b64abfd4c5ce56c1f512bc7f65b844c/dom/l10n/DocumentL10n.cpp#160,168-169
since that blocks https://searchfox.org/mozilla-central/rev/aa3ccd258b64abfd4c5ce56c1f512bc7f65b844c/dom/prototype/PrototypeDocumentContentSink.cpp#631-633

Flags: needinfo?(smaug)

My preferred "proper" fix for this would be to refactor how we do locale fallback, which is the core reason for why message formatting needs to be async.

If we were to construct a synthetic full set of messages on disk for the user's preferred locale order when they make changes to it or when langpacks update, we could expect message lookups to never need to trigger fallback. This would allow for <link rel="localization"> resources to be loaded and used more like CSS, and for localization to be more predictable.

I'm not actually sure how big and deep a change this would be, how much work this would take, or what the performance effects would be. For users, the experience would be the same as currently at least when working with a full locale, but it's likely that there would be some differences when needing to use a fallback locale for parts of the UI.

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

bug 1807838 added that hack because for some reasons (unclear to me) we use that async translateElements().
Do we use that also elsewhere, and if so, why? Is the initial l10n handling not enough, and if so, why?

If you've got a window or tooltip or menu and you want to change the string/display based on logic around how/when/why that window/tooltip/menu opens, you write code that runs right before the thing opens, that then determine strings etc. By default the only APIs fluent exposes are async.

translateElements makes this async-ness explicit, but all DOM translation (document.l10n.setAttributes and friends) is async too - the l10n-id and related attributes get set synchronously, but translation then happens as an async pass and there is no way to get a promise that resolves when that translation finishes - we only expose l10n.ready for the initial translation pass, and the stuff that we translate as part of the previous para (ie in DOMContentLoaded or popupshowing events or similar) is never part of this.

There are sync APIs for non-DOM translation, but they need to be opted into on the localization object you use to access strings, and we've been heavily discouraged from using them, in part because in the cases where they need to do IO, that means the main thread ends up blocking on the IO, which would be bad for performance/responsiveness.

Should l10n API be different?

I think something needs to happen to make this more ergonomic and less error-prone. I don't know if that means an API level change. I've been repeatedly told that string resolution may need to do IO and that IO shouldn't be forced to block the main thread and so the APIs need to be async.

I think the case where the string resolution actually needs to do IO outside of the main omni.ja (which is cached anyway) is probably limited. But it also would be bad if we create situations where Firefox performance is significantly worse for people using langpacks (ie locale not shipped into omni.ja). There are already cases where this performance difference causes subtle visual bugs, cf. bug 1801456.

There's also some performance optimizations in play where we try to not bother to add the fluent link tags to the document until we actually need them (esp. for context menus).

Could one write a simple helper on top of document.blockUnblockOnload() and document.l10n.ready ?

Maybe, but AIUI load doesn't block paint of new chrome level windows. Or am I mistaken there?

Or perhaps we could somehow reuse this array
https://searchfox.org/mozilla-central/rev/aa3ccd258b64abfd4c5ce56c1f512bc7f65b844c/dom/l10n/DocumentL10n.cpp#138
https://searchfox.org/mozilla-central/rev/aa3ccd258b64abfd4c5ce56c1f512bc7f65b844c/dom/l10n/DocumentL10n.cpp#160,168-169
since that blocks https://searchfox.org/mozilla-central/rev/aa3ccd258b64abfd4c5ce56c1f512bc7f65b844c/dom/prototype/PrototypeDocumentContentSink.cpp#631-633

Perhaps, but AIUI this is currently only used before load or maybe DOMContentLoaded? So if the strings are added later, that "doesn't work". I don't know what it would take to extend the mechanism to work for those events, and/or to do something similar for popups.

(In reply to Eemeli Aro [:eemeli] from comment #17)

My preferred "proper" fix for this would be to refactor how we do locale fallback, which is the core reason for why message formatting needs to be async.

If we were to construct a synthetic full set of messages on disk for the user's preferred locale order when they make changes to it or when langpacks update, we could expect message lookups to never need to trigger fallback. This would allow for <link rel="localization"> resources to be loaded and used more like CSS, and for localization to be more predictable.

I'd be nervous about the perf impact of doing this, but otherwise it sounds reasonable. My somewhat shaky understanding of l10n resolution before now was that what happens can depend on the exact permutation of locale references. This is messy also because the set of ftl files referenced by a given doc can change after the doc has loaded (ie people can add/remove <link rel="localization"> tags after the initial DOM load).

I'm not actually sure how big and deep a change this would be, how much work this would take, or what the performance effects would be. For users, the experience would be the same as currently at least when working with a full locale, but it's likely that there would be some differences when needing to use a fallback locale for parts of the UI.

Off-hand, I wonder about a hackier but hopefully smaller-impact-change being workable here. Specifically:

  • for popups, after popupshowing has finished passing through all relevant event handlers, check for pending translation passes on the contents of the popup and block on those promises before painting and firing popupshown/popuppositioned.
  • for windows, after DOMContentLoaded has finished passing through all relevant event handlers, check for pending translation passes on the document (and any shadowroots inside it) and block on those before painting and firing load. (I think this is somewhat close to what you were suggesting wrt DocumentL10n and PrototypeDocumentContentSink, Olli, but let me know if I'm misunderstanding.)

Q for the folks I'm needinfoing: Would that be workable, and does it seem like it would solve the issues here? It seems less drastic than changing how l10n resolution works and building a synthetic cache (or at least, it sounds like a cache to me!) and dealing with the invalidation from that etc.

In both cases, I wonder if we'd need a separate event indicating the l10n pass has finished, for code that wants to e.g. call window.sizeToContent or similar.

(There would probably need to be some frontend work as well to take advantage of this by moving some logic from load to DOMContentLoaded but I don't think that should be a problem.)

The only thing that wouldn't address is if there is some async operation that needs to complete before we call l10n things, but then that would be a problem with a sync l10n framework too...

Flags: needinfo?(smaug)
Flags: needinfo?(emilio)
Flags: needinfo?(earo)

(In reply to :Gijs (he/him) from comment #18)

Should l10n API be different?

I think something needs to happen to make this more ergonomic and less error-prone. I don't know if that means an API level change. I've been repeatedly told that string resolution may need to do IO and that IO shouldn't be forced to block the main thread and so the APIs need to be async.

My preferred change would remove the need of doing IO during message formatting, aka string resolution. The message resources would still need to get loaded from disk, but that would happen as a separate operation.

Right now, the only thing that happens when we encounter a <link rel="localization"> is that the resource is registered as a possible message source. The file is not loaded until we try to format a message that isn't found in any preceding resource, at which time resources are iterated until we find one that has the message we're looking for.

We could change this so that the load starts right away when we register the resource, and that the first pass of formatting the document doesn't start until all the linked resources are loaded. In imperative code, this would mean that something like AddResourceId() would return a promise (currently void) while FormatValue() would not need to return a promise.

(In reply to Eemeli Aro [:eemeli] from comment #17)

My preferred "proper" fix for this would be to refactor how we do locale fallback, which is the core reason for why message formatting needs to be async.

If we were to construct a synthetic full set of messages on disk for the user's preferred locale order when they make changes to it or when langpacks update, we could expect message lookups to never need to trigger fallback. This would allow for <link rel="localization"> resources to be loaded and used more like CSS, and for localization to be more predictable.

I'd be nervous about the perf impact of doing this, but otherwise it sounds reasonable. My somewhat shaky understanding of l10n resolution before now was that what happens can depend on the exact permutation of locale references. This is messy also because the set of ftl files referenced by a given doc can change after the doc has loaded (ie people can add/remove <link rel="localization"> tags after the initial DOM load).

For one recent related example, see bug 1817823. There, the Romanian langpack for 111.0b3 was missing the mozSupportLink.ftl that's dynamically added to the about:preferences page and then expected to provide the moz-support-link-text message. Because the file is missing, the whole document.l10n instance considers Romanian to be broken and switches to its next locale, reloading all of the resources and reformatting the whole DOM.

With my proposal, the missing file and its messages would have been identified ahead of time, and filled in from the user's next locale. The fallback would have been limited to just mozSupportLink.ftl messages, rather than the whole of about:preferences.

Btw, I'm pretty sure that removing localization resources is a purely theoretical concern. It's possible, and we've code for supporting it, but I'm not aware of it being called anywhere other than in tests.

[...] Would that be workable, and does it seem like it would solve the issues here? It seems less drastic than changing how l10n resolution works and building a synthetic cache (or at least, it sounds like a cache to me!) and dealing with the invalidation from that etc.

Your proposal would probably patch some of the flickering, but I can't really be sure how much. For example, the tooltip of bug 1818916 isn't recreated every time it's displayed, it's just unhidden. As far as I understand, its behaviour would not be affected by your proposed fixes.

And yes, "cache" is an appropriate term for what I'm proposing. It's pretty long-term, though, as there are only two invalidation events that need to be accounted for:

  1. A user changes their UI language settings.
  2. A new langpack version is installed.

In both cases, I wonder if we'd need a separate event indicating the l10n pass has finished, for code that wants to e.g. call window.sizeToContent or similar.

My way we probably would not need that, because the initial pass could complete in time for load.

Flags: needinfo?(earo)

(In reply to Eemeli Aro [:eemeli] from comment #19)

Your proposal would probably patch some of the flickering, but I can't really be sure how much. For example, the tooltip of bug 1818916 isn't recreated every time it's displayed, it's just unhidden. As far as I understand, its behaviour would not be affected by your proposed fixes.

The tooltip fires popupshowing and we translate it in response to (or perhaps slightly before) that, I think? So I'd expect us to not paint the tooltip until the translation informing the contents of the tooltip has finished. Or am I missing something there?

Nope, you're right; your fix would work for the tooltip as well.

Maybe, but AIUI load doesn't block paint of new chrome level windows. Or am I mistaken there?

It does, that's why Olli's hack works: https://searchfox.org/mozilla-central/rev/a2d875255c67e39b28d59a0996e8b54fa2d7564e/xpfe/appshell/AppWindow.cpp#1165,1173-1176

Would that be workable, and does it seem like it would solve the issues here? It seems less drastic than changing how l10n resolution works and building a synthetic cache (or at least, it sounds like a cache to me!) and dealing with the invalidation from that etc.

for popups, after popupshowing has finished passing through all relevant event handlers, check for pending translation passes on the contents of the popup and block on those promises before painting and firing popupshown/popuppositioned.

So we do some amount of popup positioning etc async already for animate="true" popups we wait for the transition to switch to popupshown. However that is a bit finicky (and tracking all that state correctly and efficiently per subtree seems a bit tricky). It seems it would be feasible to use CSS visibility to "block" the rendering of the popup until the right time... Would something like that on the popups that depend on async translations work?

As I see it, the main issue here is that there's no easy way of telling:

  • Whether there is any async translation pending.
  • When such async translation is done.

Unless you go ahead and manually use translateElements for everything.

As I read more after your question, I guess you sorta came to the same conclusion:

In both cases, I wonder if we'd need a separate event indicating the l10n pass has finished, for code that wants to e.g. call window.sizeToContent or similar.

So, it seems it should be somewhat straight-forward to add some sort of machinery to do this. We already track pending elements and keep track of all the mutations, we just do nothing here.

Would exposing something like boolean Document.hasPendingL10nMutations, and an event when we have no longer any L10n mutation work for an MVP? That should allow you to:

  • Block load event as needed.
  • Wait for the event if needed to show the popups. I'm thinking of something like:
document.addEventListener("popupshowing", function(e) {
  if (document.hasPendingL10nMutations) {
    e.target.setAttribute("pendingl10nmutations", "true");
    document.addEventListener("DOML10nMutationsFinished", function() {
      e.target.removeAttribute("pendingl10nmutations");
    }, { once: true })
  }
});

Mixed with something like:

panel[pendingl10nmutations],
tooltip[pendingl10nmutations],
menupopup[pendingl10nmutations] {
  visibility: hidden;
}

Or so. If we don't mind waiting for all l10n mutations to show the popup (rather than just the ones inside the popup) we could even implement that in native code as you suggested (blocking popupshown).

Flags: needinfo?(emilio) → needinfo?(gijskruitbosch+bugs)

I don't have strong opinions on Eemeli's proposed changes btw, though they do seem like a bigger / more architectural change.

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

Or so. If we don't mind waiting for all l10n mutations to show the popup (rather than just the ones inside the popup) we could even implement that in native code as you suggested (blocking popupshown).

Hm, that (ie waiting for all the mutations) does seem potentially problematic in contexts that update frequently.

I wonder if, as a slight variation on your suggestion, when adding items to the "pending translation" list, we could set the attribute you suggest from native code on the closest menupopup,panel,popup,window,html in the flattened ancestor tree, instead of firing an event, and then remove it when a translation pass happens?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(emilio)

(In reply to :Gijs (he/him) from comment #24)

Hm, that (ie waiting for all the mutations) does seem potentially problematic in contexts that update frequently.

Do you have a concrete example of that? Would that be using the same l10n-id but different l10n-args? If so, I believe that should pretty much be guaranteed to be ~sync (technically async, but we batch mutations and apply all before styling, sorta like a rAF callback) after the first translation. The only thing that should delay a frame is lazily loading a new ftl or falling back to a different locale or something.

I wonder if, as a slight variation on your suggestion, when adding items to the "pending translation" list, we could set the attribute you suggest from native code on the closest menupopup,panel,popup,window,html in the flattened ancestor tree, instead of firing an event, and then remove it when a translation pass happens?

Hmm, that would require walking up the tree for every addition and removal of nodes to the document, which seems potentially problematic... We could track whether an element is in a popup subtree with some effort, but that seems code we probably don't wanna write if we can avoid it.

Flags: needinfo?(emilio) → needinfo?(gijskruitbosch+bugs)

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

(In reply to :Gijs (he/him) from comment #24)

Hm, that (ie waiting for all the mutations) does seem potentially problematic in contexts that update frequently.

Do you have a concrete example of that?

The most obvious thing I can think of is video controls or download progress which update duration/position potentially multiple times per second.

Would that be using the same l10n-id but different l10n-args?

Often but not always - downloads esp. can change (e.g. going from hours to minutes or w/e).

I guess it'd all be fine if there's some guarantee that after an l10n pass, there is a moment in time where there are 0 pending things. If the l10n process effectively empties the queue but returns async from another thread (so with the possibility that mainthread DOM code has caused more mutations while we were off dealing with the initial set) then in theory there's a possibility we're "never done", right? That's the bit that's a little tricky, in my head at least...

I wonder if, as a slight variation on your suggestion, when adding items to the "pending translation" list, we could set the attribute you suggest from native code on the closest menupopup,panel,popup,window,html in the flattened ancestor tree, instead of firing an event, and then remove it when a translation pass happens?

Hmm, that would require walking up the tree for every addition and removal of nodes to the document, which seems potentially problematic...

Ah right. That's annoying. This would only be during load / popup being shown though, right? Would that make it better? I feel like in theory we don't do absolutely huge chunks of DOM manipulation at those points... I mean, some, but I wouldn't have thought it'd be hot code? But then, I haven't instrumented it...

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(emilio)

(In reply to :Gijs (he/him) from comment #26)

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

(In reply to :Gijs (he/him) from comment #24)

Hm, that (ie waiting for all the mutations) does seem potentially problematic in contexts that update frequently.

Do you have a concrete example of that?

The most obvious thing I can think of is video controls or download progress which update duration/position potentially multiple times per second.

Would that be using the same l10n-id but different l10n-args?

Often but not always - downloads esp. can change (e.g. going from hours to minutes or w/e).

I guess it'd all be fine if there's some guarantee that after an l10n pass, there is a moment in time where there are 0 pending things. If the l10n process effectively empties the queue but returns async from another thread (so with the possibility that mainthread DOM code has caused more mutations while we were off dealing with the initial set) then in theory there's a possibility we're "never done", right? That's the bit that's a little tricky, in my head at least...

That shouldn't really happen, at least for browser.xhtml which uses data-l10n-sync="true".

I wonder if, as a slight variation on your suggestion, when adding items to the "pending translation" list, we could set the attribute you suggest from native code on the closest menupopup,panel,popup,window,html in the flattened ancestor tree, instead of firing an event, and then remove it when a translation pass happens?

Hmm, that would require walking up the tree for every addition and removal of nodes to the document, which seems potentially problematic...

Ah right. That's annoying. This would only be during load / popup being shown though, right? Would that make it better? I feel like in theory we don't do absolutely huge chunks of DOM manipulation at those points... I mean, some, but I wouldn't have thought it'd be hot code? But then, I haven't instrumented it...

Hmm, but well, not really, consider you start the async operation before the popup starts opening? You still need to know you're in a popup before it starts opening right?

Flags: needinfo?(emilio)

See bug 1717241 and bug 1683759 for (outdated) evaluation of some of the strategies that :emilio listed above.

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

(In reply to :Gijs (he/him) from comment #26)

I guess it'd all be fine if there's some guarantee that after an l10n pass, there is a moment in time where there are 0 pending things. If the l10n process effectively empties the queue but returns async from another thread (so with the possibility that mainthread DOM code has caused more mutations while we were off dealing with the initial set) then in theory there's a possibility we're "never done", right? That's the bit that's a little tricky, in my head at least...

That shouldn't really happen, at least for browser.xhtml which uses data-l10n-sync="true".

Except that the document is set async when the first translation pass concludes.

So the l10n bits here got resolved, but I think the other conclusion Emilio and I drew in matrix at some point was that this is just because of https://searchfox.org/mozilla-central/rev/416f31a51174620f04fc994d248b664b54517699/browser/base/content/aboutDialog.js#111-116 and https://searchfox.org/mozilla-central/rev/416f31a51174620f04fc994d248b664b54517699/browser/base/content/utilityOverlay.js#412-418 which appears to have appeared in https://bugzilla.mozilla.org/show_bug.cgi?id=299713 and https://bugzilla.mozilla.org/show_bug.cgi?id=295282 in 2005. Or something. Hopefully we can just remove this and stop having platform-dependent code. Will have a look at this.

Flags: needinfo?(smaug) → needinfo?(gijskruitbosch+bugs)

(In reply to :Gijs (he/him) from comment #30)

So the l10n bits here got resolved

To be clear, that was in bug 1819664.

Assignee: nobody → gijskruitbosch+bugs
Flags: needinfo?(gijskruitbosch+bugs)
Component: Internationalization: Localization → General
Product: Core → Firefox
Attachment #9321878 - Attachment description: WIP: Bug 1738056 - stop initializing the about window asynchronously and manually sizing it, r?emilio!,mconley! → Bug 1738056 - stop initializing the about window asynchronously and manually sizing it, r?emilio!,mconley!
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/e8b15a7eb6ae stop initializing the about window asynchronously and manually sizing it, r=emilio,mconley

Oof, sorry. Updated the patch, now needs some more review.

Flags: needinfo?(gijskruitbosch+bugs)
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/b352f130099f stop initializing the about window asynchronously and manually sizing it, r=emilio,mconley

Backed out for causing bc failures on browser_aboutDialog_distribution.js.

[task 2023-03-09T17:49:40.649Z] 17:49:40     INFO - TEST-PASS | browser/base/content/test/about/browser_aboutDialog_distribution.js | About dialog appeared - 
[task 2023-03-09T17:49:40.650Z] 17:49:40     INFO - Buffered messages finished
[task 2023-03-09T17:49:40.650Z] 17:49:40     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/about/browser_aboutDialog_distribution.js | Uncaught exception in test bound verify_distribution_info_hides - Waiting for init to complete - timed out after 50 tries.
[task 2023-03-09T17:49:40.651Z] 17:49:40     INFO - Leaving test bound verify_distribution_info_hides
[task 2023-03-09T17:49:40.651Z] 17:49:40     INFO - Entering test bound verify_distribution_info_displays
[task 2023-03-09T17:49:40.989Z] 17:49:40     INFO - GECKO(1526) | [Child 1752: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 5 (7f19072d3c00) [pid = 1752] [serial = 20] [outer = 0] [url = about:blank]
[task 2023-03-09T17:49:40.991Z] 17:49:40     INFO - GECKO(1526) | [Child 1752: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 4 (7f19072d4400) [pid = 1752] [serial = 21] [outer = 0] [url = about:home]
[task 2023-03-09T17:49:40.991Z] 17:49:40     INFO - GECKO(1526) | [Child 1752: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 3 (7f19072d6000) [pid = 1752] [serial = 23] [outer = 0] [url = about:blank]
[task 2023-03-09T17:49:40.992Z] 17:49:40     INFO - GECKO(1526) | [Child 1752: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 2 (7f19072d6c00) [pid = 1752] [serial = 24] [outer = 0] [url = about:home]
[task 2023-03-09T17:49:43.249Z] 17:49:43     INFO - GECKO(1526) | [Child 1714: Main Thread]: I/DocShellAndDOMWindowLeak --DOCSHELL 7ffa6a23e800 == 0 [pid = 1714] [id = 3] [url = about:blank]
[task 2023-03-09T17:49:43.273Z] 17:49:43     INFO - GECKO(1526) | [Child 1714: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 1 (7ffa6f317660) [pid = 1714] [serial = 7] [outer = 0] [url = about:blank]
[task 2023-03-09T17:49:47.353Z] 17:49:47     INFO - GECKO(1526) | [Child 1714: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 0 (7ffa6a23ec00) [pid = 1714] [serial = 8] [outer = 0] [url = about:blank]
[task 2023-03-09T17:50:24.168Z] 17:50:24     INFO - GECKO(1526) | [WARN  rkv::backend::impl_safe::environment] `load_ratio()` is irrelevant for this storage backend.
[task 2023-03-09T17:50:24.174Z] 17:50:24     INFO - GECKO(1526) | console.error: (new Error("Polling for changes failed: Unexpected content-type \"text/plain;charset=US-ASCII\".", "resource://services-settings/remote-settings.sys.mjs", 325))
[task 2023-03-09T17:51:05.266Z] 17:51:05     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 9
[task 2023-03-09T17:52:24.181Z] 17:52:24     INFO - GECKO(1526) | 1678384344180	addons.xpi	ERROR	System addon update list error Error: got node name: html, expected: updates
[task 2023-03-09T17:52:24.185Z] 17:52:24     INFO - Console message: [JavaScript Error: "1678384344180	addons.xpi	ERROR	System addon update list error Error: got node name: html, expected: updates" {file: "resource://gre/modules/Log.sys.mjs" line: 722}]
[task 2023-03-09T17:52:24.185Z] 17:52:24     INFO - append@resource://gre/modules/Log.sys.mjs:722:12
[task 2023-03-09T17:52:24.185Z] 17:52:24     INFO - log@resource://gre/modules/Log.sys.mjs:376:16
[task 2023-03-09T17:52:24.185Z] 17:52:24     INFO - error@resource://gre/modules/Log.sys.mjs:384:10
[task 2023-03-09T17:52:24.185Z] 17:52:24     INFO - updateSystemAddons/res<@resource://gre/modules/addons/XPIInstall.jsm:4104:25
[task 2023-03-09T17:52:24.185Z] 17:52:24     INFO - 
[task 2023-03-09T17:52:35.267Z] 17:52:35     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 8
[task 2023-03-09T17:54:05.268Z] 17:54:05     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 7
[task 2023-03-09T17:55:35.268Z] 17:55:35     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 6
[task 2023-03-09T17:57:05.269Z] 17:57:05     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 5
[task 2023-03-09T17:58:35.270Z] 17:58:35     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 4
[task 2023-03-09T18:00:05.271Z] 18:00:05     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 3
[task 2023-03-09T18:01:35.271Z] 18:01:35     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 2
[task 2023-03-09T18:03:05.272Z] 18:03:05     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 1
[task 2023-03-09T18:04:35.273Z] 18:04:35     INFO - Not taking screenshot here: see the one that was previously logged
[task 2023-03-09T18:04:35.274Z] 18:04:35     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/about/browser_aboutDialog_distribution.js | Test timed out - 
[task 2023-03-09T18:04:35.295Z] 18:04:35     INFO - GECKO(1526) | MEMORY STAT | vsize 3077MB | residentFast 456MB | heapAllocated 197MB
[task 2023-03-09T18:04:35.298Z] 18:04:35     INFO - TEST-OK | browser/base/content/test/about/browser_aboutDialog_distribution.js | took 900053ms
Flags: needinfo?(gijskruitbosch+bugs)

The extant code would fail if the user switched languages while the dialog was
open, as fluent would throw the manually-inserted content away. This patch
properly uses fluent to insert the channel, which also means we don't need to
wait for it to happen after load (because of the previous patch in this
stack), it'll be there immediately.

Depends on D172007

Flags: needinfo?(gijskruitbosch+bugs)
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/fd7bd9a6fce8 stop initializing the about window asynchronously and manually sizing it, r=emilio,mconley https://hg.mozilla.org/integration/autoland/rev/491f4e9701c8 use fluent to insert channel ref into about dialog instead of hoping nothing gets retranslated, r=mkaply,fluent-reviewers,flod
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 113 Branch

I have verified the fix using Nightly 113.0a1 (20230315162649) on MacOS 11.6 and MacOS 12.4.

Status: RESOLVED → VERIFIED
See Also: → 1822917
See Also: → 1835559
Accessibility Severity: --- → s2
Whiteboard: [access-s2][fidefe-quality-foundation] → [fidefe-quality-foundation]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: