Add declarative Shadow DOM features #858

mfreed7 · 2020-04-17T22:04:15Z

The explainer for this feature is here: https://github.com/mfreed7/declarative-shadow-dom/blob/master/README.md
The issue discussion is here: #831
There is a corresponding Pull Request for the HTML spec that goes along with this PR.

At least two implementers are interested (and none opposed):
- I'm working to get this implemented in Chromium, tracking bug here.
- Other implementers have at least not been negative on the discussion thread. Not explicitly positive (yet).
Tests are written and can be reviewed and commented upon at:
- In WPT: https://wpt.fyi/results/shadow-dom/declarative?label=master&label=experimental&aligned&q=declarative
- More tests are being implemented, but the basics are there.
Implementation bugs are filed:
- Chrome: https://crbug.com/1042130
- Firefox: None yet
- Safari: None yet

Preview | Diff

This CL cleans up the variations of element::attachShadow(), refactoring the common subset back into attachShadow(). It also replaces the bool delegates_focus and manual_slotting parameters with enum versions that are common across declarative and imperative calls. This CL should not change any functionality. The spec text comes from my two PRs against DOM [1] and HTML [2]. [1] whatwg/dom#858 [2] whatwg/html#5465 Bug: 1042130 Change-Id: I3835b9d344d8005b6854909f287083cd984e832e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2155144 Commit-Queue: Mason Freed <masonfreed@chromium.org> Auto-Submit: Mason Freed <masonfreed@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Cr-Commit-Position: refs/heads/master@{#760704}

Prior to this CL, this would not work correctly: <template id=target> <div> <template shadowroot=open>Content</template> </div> </template> <script> document.body.appendChild(target.content.cloneNode(true)); </script> The inner declarative Shadow DOM (<template shadowroot>) would not get cloned from the template content to the clone. With this CL, it now works correctly. Several tests were added to test nested template and declarative Shadow DOM nodes. This CL mirrors the DOM spec changes made in [1]. [1] whatwg/dom#858 Bug: 1042130 Change-Id: I05792e038dc694ac00d13531c657afaed754f747

mfreed7 · 2020-05-04T15:34:37Z

Ok, I believe this PR is ready for review. This is my first time with a PR of this size against DOM so please let me know what I need to change. All of this PR has now been implemented in Chromium behind the DeclarativeShadowDOM flag, and WPT tests have been written.

annevk · 2020-05-04T16:53:01Z

Could you rebase it and force push so that the build output will (hopefully) be more useful?

The other question I have is where we are at with respect to implementer interest. In particular @rniwa's last comment makes me think not all design issues have been resolved.

mfreed7 · 2020-05-04T22:13:37Z

Ok, I just force-pushed. Do I also need to fix the (pre-existing) issue with en-GB spelling of "serialisation", to get Travis to build?

en-GB spelling (use lang="en-GB", or <!-- en-GB -->, on the same line to override):
6922:<a href="https://www.w3.org/TR/html5/single-page.html#html-fragment-serialisation-algorithm">fragment
Makefile:22: recipe for target 'deploy' failed
make: *** [deploy] Error 1
The command "make deploy" exited with 2.

As for implementer interest and @rniwa's last comment:

b. In the "backwards" case, the attachShadow() call from the component "wins" the race. Then the declarative content finishes parsing, and the parser tries to attach the declarative shadow root. In this case, that attachment will fail, and the declarative contents will be discarded. Here again, not performance-ideal, but everything keeps working correctly.

I don't think this behavior is acceptable. There needs to be a way for a component to let the declarative content arrive into its shadow root. Maybe we need an option to attachShadow which indicates whether the declarative content could be inserted or not.

I believe this is exactly issue 871, to expose shadowRoot on element internals. I would like to take up that issue also, but separately, and it sounds like we're close to agreement on that thread.

Also, this poses another problem. Since the component would never know when the children had finished parsing, there is no point in which the component is safe to attach a shadow root until your sibling or your ancestor's sibling starts appearing. Having to write that kind of code manually everywhere seems very fragile & developer hostile.

This second point is exactly issue 809, to add a callback when parsing is "done". I also think we need to work on that issue, but again, separately. I'm in favor of a solution to that problem, and that is @rniwa's own issue, so presumably he agrees.

Maybe it is my optimistic reading of the above, but it seemed like we might be close. I suppose we will need to wait for @rniwa to return to find out?

In the meantime, what are Mozilla's thoughts on this API? One of your comments requested more clarity about the interaction between declarative and imperative shadow roots. Hopefully that's clear with this PR and the corresponding HTML PR?

annevk · 2020-05-05T06:45:23Z

If you want to reference the HTML Standard's fragment serialization algorithm you want something like <a>HTML fragment serialization algorithm</a>. Out of curiosity, where did you copy that URL from?

It might take some time for Mozilla to determine a position. I recommend filing an issue at https://github.com/mozilla/standards-positions.

mfreed7 · 2020-05-06T00:23:23Z

If you want to reference the HTML Standard's fragment serialization algorithm you want something like <a>HTML fragment serialization algorithm</a>. Out of curiosity, where did you copy that URL from?

Thanks - done. I still don't quite understand the magic happening in the Makefile.

I got that link from the HTML spec itself - go to https://html.spec.whatwg.org/#serialising-html-fragments and check out the fragment link there for "HTML fragment serialization algorithm".

It might take some time for Mozilla to determine a position. I recommend filing an issue at https://github.com/mozilla/standards-positions.

Ok, thanks. I've filed an issue there.

annevk · 2020-05-06T06:33:38Z

Well, the link was to a completely different domain, too. 😊

Prior to this CL, this would not work correctly: <template id=target> <div> <template shadowroot=open>Content</template> </div> </template> <script> document.body.appendChild(target.content.cloneNode(true)); </script> The inner declarative Shadow DOM (<template shadowroot>) would not get cloned from the template content to the clone. With this CL, it now works correctly. Several tests were added to test nested template and declarative Shadow DOM nodes. This CL mirrors the DOM spec changes made in [1]. [1] whatwg/dom#858 Bug: 1042130 Change-Id: I05792e038dc694ac00d13531c657afaed754f747 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2175062 Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Commit-Queue: Mason Freed <masonfreed@chromium.org> Auto-Submit: Mason Freed <masonfreed@chromium.org> Cr-Commit-Position: refs/heads/master@{#766528}

Prior to this CL, this would not work correctly: <template id=target> <div> <template shadowroot=open>Content</template> </div> </template> <script> document.body.appendChild(target.content.cloneNode(true)); </script> The inner declarative Shadow DOM (<template shadowroot>) would not get cloned from the template content to the clone. With this CL, it now works correctly. Several tests were added to test nested template and declarative Shadow DOM nodes. This CL mirrors the DOM spec changes made in [1]. [1] whatwg/dom#858 Bug: 1042130 Change-Id: I05792e038dc694ac00d13531c657afaed754f747 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2175062 Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Commit-Queue: Mason Freed <masonfreed@chromium.org> Auto-Submit: Mason Freed <masonfreed@chromium.org> Cr-Commit-Position: refs/heads/master@{#766528}

mfreed7 · 2020-05-08T19:29:39Z

Well, the link was to a completely different domain, too. 😊

Oh, ha! I didn't notice it was to w3. Yeah now I'm not sure where I got that link. 😬

…ng declarative Shadow DOM, a=testonly Automatic update from web-platform-tests Add ability to clone a template containing declarative Shadow DOM Prior to this CL, this would not work correctly: <template id=target> <div> <template shadowroot=open>Content</template> </div> </template> <script> document.body.appendChild(target.content.cloneNode(true)); </script> The inner declarative Shadow DOM (<template shadowroot>) would not get cloned from the template content to the clone. With this CL, it now works correctly. Several tests were added to test nested template and declarative Shadow DOM nodes. This CL mirrors the DOM spec changes made in [1]. [1] whatwg/dom#858 Bug: 1042130 Change-Id: I05792e038dc694ac00d13531c657afaed754f747 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2175062 Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Commit-Queue: Mason Freed <masonfreed@chromium.org> Auto-Submit: Mason Freed <masonfreed@chromium.org> Cr-Commit-Position: refs/heads/master@{#766528} -- wpt-commits: 65659261a741bc42cfd35261430f47bd62deae4d wpt-pr: 23359

mfreed7 · 2020-05-21T17:19:53Z

Are there any comments on this PR, technically speaking? It is waiting for other implementer support, but it would be good to take care of any obvious spec-writing problems if possible.

annevk

FWIW, I think that apart from formatting (and it needs rebasing), this largely looks fine. Cloning shadow trees looks rather odd to me though. The explainer also doesn't go into this from a quick scan.

dom.bs

annevk · 2020-06-10T11:02:51Z

dom.bs

- {{ShadowRootInit/delegatesFocus}}.
+ <li><p>If <var>shadow host</var> has a non-null <a for=/>shadow root</a> whose
+ <a for=ShadowRoot>is declarative shadow root</a> property is true, then <a for=/>remove</a> all of
+ <a for=/>shadow root</a>'s <a>children</a>, in <a>tree order</a>. Return <var>shadow host</var>'s <a for=/>shadow root</a>.


I think it would be clearer to have a single non-null step and then have these algorithm exists (throw and return) as substeps.

Done. Let me know if this is what you were expecting.

annevk · 2020-06-10T11:12:05Z

dom.bs

@@ -5872,6 +5896,10 @@ dictionary ShadowRootInit {
  required ShadowRootMode mode;
  boolean delegatesFocus = false;
 };
+
+dictionary GetInnerHTMLOptions {
+  boolean includeShadowRoots = true;


At the very least this should say OpenShadowRoots, but it seems better to me if we did not have a separate API here for open and closed.

I've added the "rest" of this interface, as explained here in the explainer. In particular, the section just below that, Closed shadow roots goes into the details of the interface. You essentially get two knobs. includeShadowRoots is an opt-in for any shadow roots to be serialized. The second input, closedRoots is a list of closed shadow roots that you would like to be included. If a closed shadow root is encountered that is not in the list, it will be skipped to preserve encapsulation.

I am going to add the corresponding change to my HTML spec PR.

LMK if the above clarifies the situation, or if you have further questions.

mfreed7 · 2020-06-11T21:50:23Z

Thanks for the review! I think i I addressed all of your comments, but let me know what else needs attention.

FWIW, I think that apart from formatting (and it needs rebasing), this largely looks fine. Cloning shadow trees looks rather odd to me though. The explainer also doesn't go into this from a quick scan.

This is a good point, which wasn't previously in the explainer. I came across this behavior as I was implementing. Based on this comment, I've added a section to the explainer that details why this change was necessary - you can see that here. The TL/DR is that without this change (or something similar), you can't use declarative Shadow DOM inside an "ordinary" template.

mfreed7 · 2020-08-04T23:22:11Z

Just checking in on this PR. Is there anything missing, other than multi-implementer interest?

mfreed7 · 2020-09-15T21:17:33Z

Closing this, and replacing with PR 892.

mfreed7 mentioned this pull request Apr 17, 2020

Add declarative Shadow DOM features whatwg/html#5465

Merged

3 tasks

annevk added needs implementer interest Moving the issue forward requires implementers to express interest topic: shadow Relates to shadow trees (as defined in DOM) labels Apr 20, 2020

mfreed7 mentioned this pull request Apr 21, 2020

Declarative Shadow DOM #831

Closed

chromium-wpt-export-bot mentioned this pull request May 1, 2020

Add ability to clone a template containing declarative Shadow DOM web-platform-tests/wpt#23359

Merged

mfreed7 force-pushed the master branch from a4cb3ce to 6f3e2e7 Compare May 4, 2020 21:57

mfreed7 mentioned this pull request May 6, 2020

Declarative Shadow DOM mozilla/standards-positions#335

Closed

annevk reviewed Jun 10, 2020

View reviewed changes

mfreed7 added 2 commits June 11, 2020 13:03

Added "is declarative shadow root"

ffef2a2

Added existing shadow root support

0e2223a

mfreed7 added 11 commits June 11, 2020 13:03

Add getInnerHTML()

23ff7fb

Pulled out "attach a shadow root" algorithm

ffdc549

Add note about multiple declarative roots

6c35afa

Remove link to "declarative shadow root"

2f91c82

Modified clone() to allow cloning templates with declarative SD

8a0ab5d

Better logic for detecting template

9efcb60

Two quick fixes

a6a9644

Cleanup

33f4adf

Only template contents

097c512

Fix HTML link

0c41665

Addressed annevk's comments

191a88c

mfreed7 force-pushed the master branch from 54065b2 to 191a88c Compare June 11, 2020 21:42

mfreed7 mentioned this pull request Sep 15, 2020

Add declarative Shadow DOM features #892

Merged

3 tasks

mfreed7 closed this Sep 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add declarative Shadow DOM features #858

Add declarative Shadow DOM features #858

mfreed7 commented Apr 17, 2020 •

edited by pr-preview bot

Loading

mfreed7 commented May 4, 2020

annevk commented May 4, 2020 •

edited

Loading

mfreed7 commented May 4, 2020

annevk commented May 5, 2020

mfreed7 commented May 6, 2020

annevk commented May 6, 2020

mfreed7 commented May 8, 2020

mfreed7 commented May 21, 2020

annevk left a comment

annevk Jun 10, 2020

mfreed7 Jun 11, 2020

annevk Jun 10, 2020

mfreed7 Jun 11, 2020

mfreed7 commented Jun 11, 2020

mfreed7 commented Aug 4, 2020

mfreed7 commented Sep 15, 2020

Add declarative Shadow DOM features #858

Add declarative Shadow DOM features #858

Conversation

mfreed7 commented Apr 17, 2020 • edited by pr-preview bot Loading

mfreed7 commented May 4, 2020

annevk commented May 4, 2020 • edited Loading

mfreed7 commented May 4, 2020

annevk commented May 5, 2020

mfreed7 commented May 6, 2020

annevk commented May 6, 2020

mfreed7 commented May 8, 2020

mfreed7 commented May 21, 2020

annevk left a comment

Choose a reason for hiding this comment

annevk Jun 10, 2020

Choose a reason for hiding this comment

mfreed7 Jun 11, 2020

Choose a reason for hiding this comment

annevk Jun 10, 2020

Choose a reason for hiding this comment

mfreed7 Jun 11, 2020

Choose a reason for hiding this comment

mfreed7 commented Jun 11, 2020

mfreed7 commented Aug 4, 2020

mfreed7 commented Sep 15, 2020

mfreed7 commented Apr 17, 2020 •

edited by pr-preview bot

Loading

annevk commented May 4, 2020 •

edited

Loading