Add declarative Shadow DOM features by mfreed7 · Pull Request #5465 · whatwg/html

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

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

At least two implementers are interested (and none opposed):
- Chromium: Implemented and shipped.
- WebKit: supportive
- Gecko: supportive
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: https://bugzilla.mozilla.org/show_bug.cgi?id=1712140
- Safari: https://bugs.webkit.org/show_bug.cgi?id=245556

Closes #7069

/acknowledgements.html ( diff )
/document-lifecycle.html ( diff )
/document-sequences.html ( diff )
/dynamic-markup-insertion.html ( diff )
/indices.html ( diff )
/infrastructure.html ( diff )
/parsing.html ( diff )
/scripting.html ( 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}

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

Ok, I believe this PR is ready for review. This is my first time with a PR of this size against HTML 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.

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

Any comments on this PR? No rush, just bumping it in case people would like to take a look.

annevk

It seems this doesn't properly introduce all the new template attributes and adds them to the various indexes (top of the source document has instructions). I guess some special care is needed given they seem to be parser-only.

source

mfreed7 · 2020-06-11T23:21:05Z

It seems this doesn't properly introduce all the new template attributes and adds them to the various indexes (top of the source document has instructions). I guess some special care is needed given they seem to be parser-only.

Thanks for the review! I believe I've addressed all of your comments, but let me know if I missed anything. Note that the latest push also includes the addition of closed shadow roots to the serialization algorithm, which is used by my latest code in the DOM PR.

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

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

This CL implements most of the suggestions from [1], which effectively block declarative Shadow DOM from being used by any fragment parser entry point, unless an explicit opt-in is toggled. The opt-ins include: - DOMParser.allowDeclarativeShadowDom = true; - HTMLTemplateElement.allowDeclarativeShadowDom = true; - XMLHttpRequest.allowDeclarativeShadowDom = true; - DocumentFragment.allowDeclarativeShadowDom = true; - Document.allowDeclarativeShadowDom = true; // For innerHTML - A new <iframe> sandbox flag: allow-declarative-shadow-dom This mitigates the potential client-side XSS sanitizer bypass detailed in the explainer and at [1]. Assuming these changes are functional, and mitigate the issue, this new behavior will be folded into the spec PRs at [2] and [3]. But given the security implications of the existing code, I'd like to get this landed first. [1] whatwg/dom#912 (comment) [2] whatwg/html#5465 [3] whatwg/dom#892 Bug: 1042130 Change-Id: I088f28f63078a0d26e354a4442494c0132b47ffc

This CL implements most of the suggestions from [1], which effectively block declarative Shadow DOM from being used by any fragment parser entry point, unless an explicit opt-in is toggled. The opt-ins include: - DOMParser.allowDeclarativeShadowDom = true; - HTMLTemplateElement.allowDeclarativeShadowDom = true; - XMLHttpRequest.allowDeclarativeShadowDom = true; - DocumentFragment.allowDeclarativeShadowDom = true; - Document.allowDeclarativeShadowDom = true; // For innerHTML - A new <iframe> sandbox flag: allow-declarative-shadow-dom This mitigates the potential client-side XSS sanitizer bypass detailed in the explainer and at [1]. Assuming these changes are functional, and mitigate the issue, this new behavior will be folded into the spec PRs at [2] and [3]. But given the security implications of the existing code, I'd like to get this landed first. [1] whatwg/dom#912 (comment) [2] whatwg/html#5465 [3] whatwg/dom#892 Bug: 1042130 Change-Id: I088f28f63078a0d26e354a4442494c0132b47ffc Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2513525 Commit-Queue: Mason Freed <masonfreed@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/heads/master@{#824591}

…arsing to be opt-in, a=testonly Automatic update from web-platform-tests Change declarative Shadow DOM fragment parsing to be opt-in This CL implements most of the suggestions from [1], which effectively block declarative Shadow DOM from being used by any fragment parser entry point, unless an explicit opt-in is toggled. The opt-ins include: - DOMParser.allowDeclarativeShadowDom = true; - HTMLTemplateElement.allowDeclarativeShadowDom = true; - XMLHttpRequest.allowDeclarativeShadowDom = true; - DocumentFragment.allowDeclarativeShadowDom = true; - Document.allowDeclarativeShadowDom = true; // For innerHTML - A new <iframe> sandbox flag: allow-declarative-shadow-dom This mitigates the potential client-side XSS sanitizer bypass detailed in the explainer and at [1]. Assuming these changes are functional, and mitigate the issue, this new behavior will be folded into the spec PRs at [2] and [3]. But given the security implications of the existing code, I'd like to get this landed first. [1] whatwg/dom#912 (comment) [2] whatwg/html#5465 [3] whatwg/dom#892 Bug: 1042130 Change-Id: I088f28f63078a0d26e354a4442494c0132b47ffc Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2513525 Commit-Queue: Mason Freed <masonfreed@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/heads/master@{#824591} -- wpt-commits: b13461b9a46b46eb1b092a58bde2b10418e7a73d wpt-pr: 26398

mfreed7 · 2020-11-24T18:31:23Z

This PR has been updated to include the "opt-in" mechanics described in #912. The changes I made here roughly match the current implementation of Chromium, so I'm hoping they're functional. But comments appreciated.

Note that no matter what I do, I am unable to get rid of the "parse error while closing p element" error. I bisected this error down to the addition of <span>include shadow roots flag<span> which does not contain a <p>. Help.

source

@rniwa

More edits Fixup Cleanup Add language for content Add shadow root serialization Link to new "attach a shadow root" algorithm. Added exception handling. Add early-out for topmost node Added shadowroot to HTMLTemplateElement for feature detection Addressed annevk's comments Add available_to_internals = true Add declarative shadow dom opt-in mechanics Trying, unsuccessfully, to get rid of "parse error while closing p element" Remove data-x for instance checks Merge fixup Rename include shadow roots state Strip out serialization stuff Lint cleanup Fix closing tags Fix dfn Fix conformance error Address all comments Fix lint First cut at streaming DSD definition Address some comments Address @rniwa comments Capitalization Rename shadowrootmode and add clonable Address comments Address comments Line length Fix regular document parsing Address comments Update to match DOM's new `declarative` Clean up call to attach a shadow root Address speculative parser comment Replace tri-state with boolean Allow about:blank Address annevk comments Address annevk comments Report the exception

The spec PR [1] is about to land, so these tests should no longer be tentative. [1] whatwg/html#5465 Bug: 1379513 Change-Id: I577abb26b3c29a04f14ddafec7400abadb4119ed

mfreed7 · 2023-10-11T16:55:46Z

This now needs rebasing on top of 3ca0811 and make the appropriate changes to those algorithms. I can take care of a final nit commit after that and then after you double check we can hopefully finally land this.

Sounds good, will do.

Done. Let me know if the new text looks ok.

I'll take care of this today, in anticipation of this PR landing.

web-platform-tests/wpt#42488

The spec PR [1] is about to land, so these tests should no longer be tentative. [1] whatwg/html#5465 Bug: 1379513 Change-Id: I577abb26b3c29a04f14ddafec7400abadb4119ed Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4932173 Auto-Submit: Mason Freed <masonf@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Reviewed-by: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1208576}

annevk · 2023-10-16T12:36:12Z

source

  [<span>HTMLConstructor</span>] constructor();

  readonly attribute <span>DocumentFragment</span> <span data-x="dom-template-content">content</span>;
+  [<span>CEReactions</span>] attribute DOMString <span data-x="dom-template-shadowrootmode">shadowRootMode</span>;


Both Chromium and WebKit fail this assert:

document.createElement("template").shadowRootMode === ""

is this tested and planned to be fixed or should we change the standard?

Looking at the tests the tests don't match the specification here. I think the specification is better, but it might not be worth the churn.

@rniwa @mfreed7?

Both Chromium and WebKit fail this assert:

document.createElement("template").shadowRootMode === ""

is this tested and planned to be fixed or should we change the standard?

This is likely due to your request to change from nullable? It'll require a change to browsers, which hasn't happened yet. I'd obviously be happier not changing browsers and instead changing the standard.

I was expecting tests and such to be updated as we reached agreement on things here. Are there other things that might now be out-of-sync?

I was expecting tests and such to be updated as we reached agreement on things here. Are there other things that might now be out-of-sync?

That'll require a (breaking) behavior change in Chrome, so no, I wasn't planning to do that work until I was sure this behavior "stuck". I.e. when this PR lands.

I don't believe there were other breaking changes you requested, no. I believe this is the only one.

I'm a bit puzzled by these comments. In the initial PR there wasn't even a shadowrootmode attribute. It looks like it was called shadowroot. I think we made a number of "breaking" changes over the lifetime of this PR. I'm happy to push a commit that makes this attribute return null, but I'd really like to be sure it's the only thing where the specification mismatches with the tests and implementations.

Also, did you have a chance to review the commit I already pushed?

I'm a bit puzzled by these comments. In the initial PR there wasn't even a shadowrootmode attribute. It looks like it was called shadowroot. I think we made a number of "breaking" changes over the lifetime of this PR.

Sorry you're puzzled. The "initial PR" was 3.5 years ago, and a lot has happened in the intervening years. I've shipped and then deprecated the original shadowroot attribute in Chromium. As discussion resumed on the PR in 2023, we renamed to shadowrootmode and added the corresponding IDL. I had to push through some (reasonable) opposition to bikeshedding after shipping. I then shipped again, with shadowrootmode, so that developers had something to migrate to from the old shadowroot. Five weeks ago, you requested the change from DOMString? to DOMString for shadowRootMode and I agreed, to try to finally get this PR landed. You then asked (just above) whether this was "worth the churn". In general, through all of this, I'd say "no it has never been worth the churn". But I'd like to get this feature shipped interoperably, so as with the other changes, I went along with this one, and I'm ok keeping it as DOMString. Hopefully it's the last change, and we can finally land this PR.

I'm happy to push a commit that makes this attribute return null, but I'd really like to be sure it's the only thing where the specification mismatches with the tests and implementations.

I believe this is the only thing that doesn't match, and in an hour or two, the test update should land. At that point, the spec should match the tests. Once this PR lands, I'll make the behavior change to Chromium (for DOMString? -> DOMString), so it'll at least match one implementation. I can't speak for other implementations.

Also, did you have a chance to review the commit I already pushed?

I just reviewed the nits commit, and all of those changes look good to me. Thanks.

The test update has landed. At this point the tests should match this PR.

Thanks for the update, that helps. I'm happy to make the shadowRootMode change in WebKit so let's go with that given you already updated the tests.

I created web-platform-tests/wpt#42833 to reflect that getInnerHTML() is non-standard and also took the opportunity to add some shadowRootMode setter coverage.

mfreed7 · 2023-10-29T22:47:16Z

🎉 Thanks for all of the reviews to get this landed!

annevk · 2023-10-30T15:00:44Z

@mfreed7 thank you! For contributing this feature to the web and working on these PRs over a period of three(!) years.

sideshowbarker added the impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation label Apr 18, 2020

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 whatwg/dom#831

Closed

This was referenced May 4, 2020

Add declarative Shadow DOM features whatwg/dom#858

Closed

Declarative Shadow DOM mozilla/standards-positions#335

Closed

annevk reviewed Jun 10, 2020

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

mfreed7 force-pushed the add_declarative branch from c8d4d8d to fd5171b Compare June 11, 2020 23:15

mfreed7 force-pushed the add_declarative branch from f3c293e to 29cdf7e Compare September 11, 2020 21:10

mfreed7 mentioned this pull request Sep 15, 2020

Add declarative Shadow DOM features whatwg/dom#892

Merged

3 tasks

mfreed7 force-pushed the add_declarative branch from 4e2da5b to 57f9319 Compare September 25, 2020 05:05

chromium-wpt-export-bot mentioned this pull request Nov 4, 2020

Change declarative Shadow DOM fragment parsing to be opt-in web-platform-tests/wpt#26398

Merged

mfreed7 mentioned this pull request Nov 16, 2020

[Declarative Shadow DOM] How should we build the "opt-in" for fragment parsing of declarative Shadow DOM? whatwg/dom#912

Closed

mfreed7 force-pushed the add_declarative branch from 96267bf to 53dde66 Compare November 24, 2020 18:26

mfreed7 mentioned this pull request Nov 24, 2020

Add link to declarative shadow dom opt-in whatwg/xhr#300

Closed

3 tasks

annevk reviewed Dec 3, 2020

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

mfreed7 force-pushed the add_declarative branch from 53dde66 to 0067e75 Compare December 4, 2020 01:08

mfreed7 and others added 12 commits October 11, 2023 09:09

Merge fixup and remove DOMParser stuff

b595e73

Address comments

b1f8e5d

Remove nullable

ac1eb01

Address comments

84faf12

Convert enumerated attribute to table

894ab62

Convert to dir example format for enumerated attribute

39558d7

Connect to attributes

700fba0

Fix the build

7d4b563

Address comments

e2530e1

Fix copypasta

8d9043c

Undo shadowrootmode and linkify delegates focus

d65e189

mfreed7 force-pushed the add_declarative branch from 8686d64 to d65e189 Compare October 11, 2023 16:10

Update for {set|parse}HTMLUnsafe()

e148a63

chromium-wpt-export-bot mentioned this pull request Oct 11, 2023

Remove .tentative extension for declarative shadow DOM tests web-platform-tests/wpt#42488

Merged

annevk reviewed Oct 16, 2023

View reviewed changes

nits

e9eccef

chromium-wpt-export-bot mentioned this pull request Oct 19, 2023

Change expectation for invalid/missing shadowRootMode IDL to null web-platform-tests/wpt#42619

Merged

annevk mentioned this pull request Oct 29, 2023

Account for latest declarative shadow trees changes WebKit/WebKit#19692

Merged

annevk approved these changes Oct 29, 2023

View reviewed changes

MarcusNotheis mentioned this pull request Nov 3, 2023

[framework][Feature Request] Server-Side Rendering of UI5 Web Components UI5/webcomponents#6626

Open

7 tasks

YusukeHirao mentioned this pull request Nov 7, 2023

Add the shadowrootmode and shadowrootdelegatesfocus attributes to the template element markuplint/markuplint#1255

Closed

mfreed7 mentioned this pull request Mar 19, 2024

template element declarative shadow tree IDL attributes #10211

Closed

annevk mentioned this pull request Nov 25, 2025

Questions about HTMLTemplateElement.content getter #11953

Closed

Conversation

mfreed7 commented Apr 17, 2020 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mfreed7 commented May 4, 2020

Uh oh!

mfreed7 commented May 21, 2020

Uh oh!

annevk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mfreed7 commented Jun 11, 2020

Uh oh!

mfreed7 commented Aug 4, 2020

Uh oh!

mfreed7 commented Nov 24, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mfreed7 commented Oct 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfreed7 commented Oct 29, 2023

Uh oh!

annevk commented Oct 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

10 participants

mfreed7 commented Apr 17, 2020 •

edited by pr-preview bot

Loading