1617669 - Shadow DOM handled by document.l10n on mutation, but not on creation

Status: RESOLVED FIXED

(Core :: XUL, enhancement)

Description

[Axel Hecht [:Pike]]

When using Fluent in shadow DOM, we add shadowRoot as explicit root to a DOMLocalization, but when manipulating the shadow DOM, the mutation observers fire on document.l10n.

This is sadly inconsistent.

We're looking at using shadow DOM to isolate remote settings l10n from the regular document.l10n in bug 1616280, and that doesn't work out alright.

smaug, it seems that https://searchfox.org/mozilla-central/source/dom/l10n/L10nMutations.cpp is rather generous at listening to mutations, and doesn't care about the roots in the corresponding DOMLocalization?

Would it be feasible to make that content check if the content is part of its roots?

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

What content check?
I feel like I'm missing some context here.

Comment 2

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

Andreio - can you provide a minimized testcase analogous to https://searchfox.org/mozilla-central/source/dom/l10n/tests/mochitest/dom_localization/test_connectRoot_webcomponent.html of the issue you're facing?

It'll make it easier for us to reproduce it and fix.

Comment 3

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

The component has been changed since the backlog priority was decided, so we're resetting it.
For more information, please visit auto_nag documentation.

Comment 4

[Axel Hecht [:Pike]]

Attachment: Test for Bug 1617669

In this test, I don't connectRoot the shadow dom, but the
localization from document.l10n is applied via a mutation observer
on setAttributes.
Also note that it's not the localization that we call setAttributes
on.

Comment 5

[Axel Hecht [:Pike]]

The observer methods check for elem->IsInComposedDoc(), which is OK if there's only one document.l10n involved, and everything with data-l10n-id is hooked up.

These checks would need to be extended to walk up their parents and see if the hit a root in mDOMLocalization?

smaug, does this testcase help to see what we were trying to do?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

But there is // document.domLoc.connectRoot(this.shadowRoot);
Why is that?

Comment 7

[Axel Hecht [:Pike]]

I left the commented-out connectRoot in just to hightlight where the original test connected the root to it's non-default DOMLocalization object.

Just to clarify, I don't think this one test is an exhaustive test coverage, it's just one example. Which I understood to be what Zibi asked for in comment 2.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

I'm still puzzled what this bug is about.
https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/dom/l10n/DOMLocalization.cpp#85,105
observes mutations in the subtree under aNode. But I guess that works as expected? And initial translation doesn't happen? So do you mean that when ConnectRoot is called, some localization should happen? But isn't translateFragment for that case. or translateRoots.

I don't know what "Would it be feasible to make that content check if the content is part of its roots?" means.

Comment 9

[Axel Hecht [:Pike]]

The expected behavior is for this element to be not localized, w/out explicitly hooking it up to a DOMLocalization.

Once explicitly hooked up, the expectation is that it's handled by that DOMLocalization, and to not have different DOMLocalizations fight over what wins.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

ok, still a bit unclear to me.
Do you perhaps mean that L10nMutations shouldn't do anything with nodes which aren't in its subtree?
(That happens because of https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/dom/base/MutationObservers.cpp#84-88)

If so, the methods implementing nsIMutationObserver methods could check if modified node is contained within some root.
(that could be rather slow if there are lots of roots). One could of course be on L10nMutations per root, but that might be slow too if there are tons of them - but if we don't expect too many roots, that might be doable. Then L10nMutations could have a pointer to the relevant root.

Comment 11

[Axel Hecht [:Pike]]

Right.

Also, the root checker could drop if it crosses from shadow to non-shadow DOM?

As for the number of roots, I can see DOMLocalizations for things like panel objects to be quite extensive. OTH, shadow DOMs are probably pretty shallow, and you if you stop at the shadow-non-shadow bridge, you'd stop rather early.

Comment 12

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

I'll take a look at this. I'm concerned it'll lead to more code being increasingly broken when trying to fix it, so better now than later.

Comment 13

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

Attachment: Bug 1617669 - Check if an element is in any of the roots before adding to pending mutations.

Comment 14

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

Olli - can you check if the patch looks reasonable? I'm going to work on improving the tests (need to add coverage of adding elements) and will check how the performance looks like, but wanted to run it through you first.

Overall, I feel like it should be mostly ok for smaller cases, but I'm a bit concerned that for big documents like browser.xhtml or preferences.xhtml, traversing nodes up for every element to check if it's in the document will cost.

In reality, I'd probably want to just check if the element is in <template/> and only then verify if it is in roots, otherwise it must be since it got mutation observed called on it. Is that right?

Comment 15

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

Alternatively, maybe we could extend the internal MutationObserver to not report mutations on nodes that aren't in its subtree?

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

That might be useful for some other cases, but requires quite some changes, and might slow down DOM mutations in general, since the whole
nsIMutationObserver system would need to track which node is possible slotted in shadow DOM etc.

Comment 17

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

Does it mean you recommend it over the patch that I wrote? Or do you think the patch is a reasonable short term approach and we can investigate the feature for MutationObserver separately?

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Yes, go with the approach the patch has taken.

Comment 19

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

Filed a follow up as bug 1624029.

Comment 20

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

treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=41f1c1532b85af2cfaf056d9eac7d284cb09ab1d

Comment 21

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

fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=df48b021257338a3070ee26a3617414e30b4eda0

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f76a5f5c354b
Check if an element is in any of the roots before adding to pending mutations. r=smaug

Comment 23

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/f76a5f5c354b