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

Status: VERIFIED FIXED

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

Description

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

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

Comment 2

[Mike Conley (:mconley) (:⚙️)]

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

Comment 3

[Artem]

Attachment: Bug 1608197 - port createProfileWizard DTD to fluent r?mconley,gijs

Comment 4

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

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

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

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.

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

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.

Comment 7

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

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

Comment 8

[Zibi Braniecki [:zbraniecki][: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.

Comment 9

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

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

Comment 10

[Zibi Braniecki [:zbraniecki][: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.

Comment 11

[Zibi Braniecki [:zbraniecki][: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.

Comment 12

[Zibi Braniecki [:zbraniecki][:gandalf]]

Verified that this makes it work:

connectedCallback() {
  if (document.l10n) {
    document.l10n.connectRoot(this.shadowRoot);
  }
}

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/154f0e6408f6
port createProfileWizard DTD to fluent r=Gijs,fluent-reviewers,flod

Comment 14

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/154f0e6408f6

Comment 15

[Bogdan Maris, Desktop Test Engineering]

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