1845737 - moz- custom elements using insertFTLIfNeeded shouldn't require `MozXULElement` (and thereby customElements.js + a privileged running context)

Status: ASSIGNED

(Toolkit :: UI Widgets, task, P3)

Description

[:Gijs (he/him)]

I ran into this in https://searchfox.org/mozilla-central/source/toolkit/content/widgets/moz-support-link/moz-support-link.mjs and https://searchfox.org/mozilla-central/rev/00e6644d0db8acf9372702324151b8077a3d2bb7/toolkit/content/widgets/moz-five-star/moz-five-star.mjs#8 and Hanna said it'll also happen for moz-message-bar when finished.

Ideally we want these components to be usable in non-privileged contexts. This also makes things easier in storybook.

The simplest solution would be if we either created a native implementation that was exposed on document.l10n, or ported this onto MozLitElement.

As I'm running into this in some high-prio work I might choose to make those calls window.MozXULElement?.insert... to ensure they don't break, and "just" add the FTL file to the main document. But it'd be better to fix this in the components - it's just more work.

Comment 1

[:Gijs (he/him)]

I have patches for this on try at this point: https://treeherder.mozilla.org/jobs?repo=try&revision=ea65b5801fad502d9f92f3619b696c076da2468c

Roughly, they:

1. move the implementation to document.l10n
2. make the MozXULElement implementation forward its call to document.l10n
3. make sure document.l10n can be created by virtue of JS asking for it (otherwise it can break in cases where the document did not have any ftl linked before the first call to insertFTLIfNeeded)
4. move the polyfill implementation from storybook to use document.l10n instead of MozXULElement, too
5. mass-replace browser/ and toolkit/ consumers

Assuming this lands, I then intend to create follow-up bugs to:

a. replace any remaining consumers (also to avoid patch races when landing) and remove the MozXULElement forwarding implementation.
b. consider removing any doc.l10n nullchecks which are currently strewn around a few different components etc. I don't think we care about loading the components in non-about, unprivileged documents. Hanna/Mark, is that right?

Comment 2

[Eemeli Aro [:eemeli]]

While this certainly looks like it works, I wonder if perhaps we don't need to do all the work of adding an actual <link rel=localization>?

Currently, when doing so three things happen in Document::LocalizationLinkAdded:

1. The document.l10n is created if necessary.
2. A resource id is added to the localization.
3. An initial translation pass is triggered.

To avoid unnecessary work, MozXULElement.insertFTLIfNeeded checks first if a <link> exists with the same href. But what if we didn't, and just added the resource id directly without reading or writing the DOM? Then for each of the steps above:

1. The document.l10n creation is handled on access.
2. The resource id addition ends up being a hashmap insert where the boolean did-modify return value is ignored.
3. The initial translation pass has an early exit when it's called more than once per DocumentL10n instance.

To deduplicate some of the code, we could (should?) have a DocumentL10n::InsertFTL that's also called by Document::LocalizationLinkAdded.

Comment 3

[:Gijs (he/him)]

That seems reasonable. FWIW I had patches in hand ~2.5 weeks ago, but I hit a bit of a small roadblock in that:

• it's currently possible to use insertFLIfNeeded before document.l10n exists.
• creating it on demand (per discussion in #DOM with Emilio at the time) causes test failures. There are some benign ones (we have unit tests that check that document.l10n isn't there yet, and obviously if you make the getter instantiate it then that doesn't work) but also some confusing ones in prompt code, where I suspect the early creation of document.l10n is causing changes to when document.l10n.ready resolves. Trypush: https://treeherder.mozilla.org/jobs?repo=try&revision=ea65b5801fad502d9f92f3619b696c076da2468c&selectedTaskRun=b5PpoPMwTau6keXOjOfTMg.0 . Relevant failures in e.g. browser_quicksuggest_onboardingDialog.js and browser/components/preferences/tests/browser_subdialogs.js suggests I broke subdialog code, which awaits l10n.ready here - but only if document.l10n exists at the point that code runs, so perhaps that's the issue?

I didn't have time yet to come back to those patches and work out the test failures. I imagine that not inserting into the DOM would not really materially alter the test failures.

The downside of not inserting into the DOM is that it's not obvious to frontend code what l10n files are / aren't present. There is no way for frontend to interrogate the internal hashset. But maybe that's OK? Mike, wdyt?

Comment 4

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

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

The downside of not inserting into the DOM is that it's not obvious to frontend code what l10n files are / aren't present. There is no way for frontend to interrogate the internal hashset. But maybe that's OK? Mike, wdyt?

From my experience with Fluent in the front-end, I think the normal pattern is to assume that the .ftl files needed aren't in the doc, and insertFTLIfNeeded in either case, expecting it to short-circuit of the requested .ftl files are already present. I don't recall ever seeing any front-end code that tries to determine what .ftl files are already loaded in the doc. Maybe I'm not aware of some, but I personally think not being able to detect the loaded .ftl files is okay.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

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

• creating it on demand (per discussion in #DOM with Emilio at the time) causes test failures. There are some benign ones (we have unit tests that check that document.l10n isn't there yet, and obviously if you make the getter instantiate it then that doesn't work) but also some confusing ones in prompt code, where I suspect the early creation of document.l10n is causing changes to when document.l10n.ready resolves. Trypush: https://treeherder.mozilla.org/jobs?repo=try&revision=ea65b5801fad502d9f92f3619b696c076da2468c&selectedTaskRun=b5PpoPMwTau6keXOjOfTMg.0 . Relevant failures in e.g. browser_quicksuggest_onboardingDialog.js and browser/components/preferences/tests/browser_subdialogs.js suggests I broke subdialog code, which awaits l10n.ready here - but only if document.l10n exists at the point that code runs, so perhaps that's the issue?

FWIW given that description, I think the main issue is that your patch allows to create a DocumentL10n that will effectively never hit the ready state. I suspect you basically need this chunk of code on the lazy getter too (or shuffle things around a little bit).

Comment 6

[Mark Striemer [:mstriemer]]

I'm not sure what "non-about, unprivileged documents" would refer to. Are these embedded in <browser> elements or in dialogs? Ideally these components would work everywhere, but I guess it would depend where we have this limitation

Comment 7

[:Gijs (he/him)]

(In reply to Mark Striemer [:mstriemer] from comment #6)

I'm not sure what "non-about, unprivileged documents" would refer to.

Basically, MozXULElement is only loaded in privileged contexts (so not in about: pages so we need hacks there to make the MozXULElement bits work). Moving insertFTLIfNeeded to an interface that's available to the about pages where we already use fluent fixes some of the immediate issues that caused me to file this bug. But effectively my question is... is there an even wider problem where we'd like this utility to be available more widely than that still? I think the answer is "no", but I wanted to doublecheck.

Right now fluent (document.l10n) works in effectively 3 places:

• about: pages that can't be opened by web content (so about:mozilla and about:preferences, but not about:blank and about:srcdoc which web content can open)
• privileged contexts ("chrome" code, so the browser window etc.)
• some webextension cases

My question is basically... do we ever care about being able to use insertFTLIfNeeded in places where document.l10n isn't present?

I think the answer is "no" (or perhaps "yes, but only storybook, and we already workaround MozXULElement not being around there so it's fine").

Does that make more sense?

Comment 8

[Eemeli Aro [:eemeli]]

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

My question is basically... do we ever care about being able to use insertFTLIfNeeded in places where document.l10n isn't present?

I think the answer is "no" (or perhaps "yes, but only storybook, and we already workaround MozXULElement not being around there so it's fine").

I agree on the "no". As is, document.l10n will only let you load message files that are packaged into the build or a langpack, and doing something special when a <link rel="localization"> is added to the DOM somewhere where it's not available doesn't really make any sense.

Comment 9

[Mark Striemer [:mstriemer]]

Agreed, I don't know that we'd need to load these components in those contexts. I'd think any component that's used in those places would need to not use Fluent so then we wouldn't have these calls in there. Likely no need to do null checking on document.l10n then

Comment 10

[:Gijs (he/him)]

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

FWIW given that description, I think the main issue is that your patch allows to create a DocumentL10n that will effectively never hit the ready state. I suspect you basically need this chunk of code on the lazy getter too (or shuffle things around a little bit).

I haven't yet checked but I would have thought that because my patch was still creating a link (like the JS version), the code there would have run anyway in response to the FTL being added as a link? But thanks for the hint, I'll dig into this...

Comment 11

[Stuart Colville [:muffinresearch]]

Hi Gijs, just wondering if there's any updates on this issue?

Comment 12

[:Gijs (he/him)]

(In reply to [e-mail] from comment #11)

Hi Gijs, just wondering if there's any updates on this issue?

Unfortunately not. I haven't had a chance to go back to the work here; new "high priority" things keep appearing. Is this blocking anything?

Comment 13

[Stuart Colville [:muffinresearch]]

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

(In reply to [e-mail] from comment #11)

Hi Gijs, just wondering if there's any updates on this issue?

Unfortunately not. I haven't had a chance to go back to the work here; new "high priority" things keep appearing. Is this blocking anything?

No problem, I'm not aware of any blockers, just checking-in given it came up on our recomp board review.