Closed Bug 1608197 Opened 5 months ago Closed 2 months ago

[MSU Capstone] Port toolkit/chrome/mozapps/profile/createProfileWizard.dtd to Fluent

Categories

(Toolkit :: Startup and Profile System, task, P3)

task

Tracking

()

VERIFIED FIXED
mozilla76
Tracking Status
firefox76 --- fixed
firefox77 --- verified

People

(Reporter: mconley, Assigned: salniker)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

This DTD appears to be used here:

https://searchfox.org/mozilla-central/rev/ba4fab1cc2f1c9c4e07cdb71542b8d441707c577/toolkit/profile/content/createProfileWizard.xhtml

This might suffer the same difficulty as bug 1608165, so let's wait for the needinfo to come back from gandalf for bug 1608165 comment 1 before digging into this.

(In reply to Mike Conley (:mconley) (:⚙️) (PTO until Feb 10) from comment #0)

This might suffer the same difficulty as bug 1608165, so let's wait for the needinfo to come back from gandalf for bug 1608165 comment 1 before digging into this.

Per bug 1608165 comment 4, this should be fine to work on now.

See Also: 1608165

This dialog can be opened by going to about:profiles, and clicking on "Create a new Profile".

Assignee: nobody → salniker

Needinfo for the phabricator questions at https://phabricator.services.mozilla.com/D62867#1996831 re: a fluent issue that is blocking landing this patch.

Flags: needinfo?(gandalf)
Status: NEW → ASSIGNED

I'm investigating this. So far I can't confirm that this is XULCache related. Disabling cache did not fix the problem, but I can reproduce it on Linux so I'll debug deeper.

Ok, so, I did more debugging.

First observation is that this is a race condition. If I delay the document.l10n.setAttributes by 1000ms it works every time.
So what happens is that on second/cached load the document loads faster (XULCache likely, but not Fluent in XULCache related), then we trigger the attributes to be set before and then mutation is not registered.

Now, I'd argue mutation should not be registered in either case - cold or warm - because the shadow dom is not connected to the DOMLocalization!

The reason it works at all in the cold case is likely bug 1617669.

So, once we fix it, it will just not work. Now, you can fix it by calling document.l10n.connectRoot, or by manually triggering the translation using document.l10n.translateElements([label]); right after setting the attributes.

I'd prefer the former because it fits into the dynamic translation model better, but since its a dialog I doubt it'll matter and the latter will work as well.

I'd ask thyself why is the adjustFluentHeaders called in a racy way, maybe it should be called from a different place or behind some rediness/connectedness guard? But that's not Fluent related.

Flags: needinfo?(gandalf)

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

Ok, so, I did more debugging.

First observation is that this is a race condition. If I delay the document.l10n.setAttributes by 1000ms it works every time.
So what happens is that on second/cached load the document loads faster (XULCache likely, but not Fluent in XULCache related), then we trigger the attributes to be set before and then mutation is not registered.

Now, I'd argue mutation should not be registered in either case - cold or warm - because the shadow dom is not connected to the DOMLocalization!

The reason it works at all in the cold case is likely bug 1617669.

So, once we fix it, it will just not work. Now, you can fix it by calling document.l10n.connectRoot, or by manually triggering the translation using document.l10n.translateElements([label]); right after setting the attributes.

I'd prefer the former because it fits into the dynamic translation model better, but since its a dialog I doubt it'll matter and the latter will work as well.

I'd ask thyself why is the adjustFluentHeaders called in a racy way, maybe it should be called from a different place or behind some rediness/connectedness guard? But that's not Fluent related.

What is racy about the call? What is it racing with? You wrote:

we trigger the attributes to be set before and then mutation is not registered.

but not... before what?

Flags: needinfo?(gandalf)

What is racy about the call? What is it racing with?

What I think I'm seeing is that the alternation of the DOM happens before or after the DOM is cloned into shadow DOM from the template depending on whether its a cold or warm load.
I may be wrong here, I just see that if I delay the setAttributes, it always hits.

My deduction is that there's something specific about the "warm case" which makes the mutations not be covered by the L10nMutations.
In cold case and in delayed case it is.

Flags: needinfo?(gandalf)

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

My deduction is that there's something specific about the "warm case" which makes the mutations not be covered by the L10nMutations.

Isn't that a bug in fluent?

Flags: needinfo?(gandalf)

Isn't that a bug in fluent?

No, if my hypothe^H^H^H^ guess is correct, then the way this code works at the moment is that it clones the source of the web component DOM between and applies translation ID/attrs in racy order. That would be a bug in this code. I'll try to finish the patch in bug 1617669 today and will test against this patch.

Flags: needinfo?(gandalf)

Ok, I confirmed that with the patch from bug 1617669 the header is not translated neither on the cold nor hot load.

So my hypothesis was correct - we were applying the translation too early (in template), and it was localized using document's l10n.

Instead, you should call document.l10n.connectRoot in connectedCallback.

Verified that this makes it work:

connectedCallback() {
  if (document.l10n) {
    document.l10n.connectRoot(this.shadowRoot);
  }
}
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/154f0e6408f6
port createProfileWizard DTD to fluent r=Gijs,fluent-reviewers,flod
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla76
Regressions: 1626779

Verified fixed as part of PI 551; tested with Nightly 77 across platforms (Windows 10, macOS 10.15 and Ubuntu 18.04).

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.