whatwg
Add declarative Shadow DOM features
The explainer for this feature is here: https://github.com/mfreed7/declarative-shadow-dom/blob/master/README.md The issue discussion is here: https://github.com/whatwg/dom/issues/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):
- 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).
-
[X] 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.
-
[X] Implementation bugs are filed:
- Chrome: https://crbug.com/1042130
- Firefox: None yet
- Safari: None yet
:boom: Error: Wattsi server error :boom:
PR Preview failed to build. (Last tried on Jan 15, 2021, 8:00 AM UTC).
More
PR Preview relies on a number of web services to run. There seems to be an issue with the following one:
:rotating_light: Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.
:link: Related URL
Parsing MDN data...
Parsing...
Parse Error: (85293,18) parse error while closing p element
If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.
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.
Any comments on this PR? No rush, just bumping it in case people would like to take a look.
It seems this doesn't properly introduce all the new
templateattributes and adds them to the various indexes (top of thesourcedocument 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.
Just checking in on this PR. Is there anything missing, other than multi-implementer interest?
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.
I'm also missing a description of all the new template element attributes (and their addition to various indexes).
As for documentation, there is some advice in the README as well as pointers to various other documents, but I don't think we have anything directly covering wattsi (HTML) and Bikeshed (all other WHATWG standards, including DOM), though Bikeshed has https://tabatkins.github.io/bikeshed/ which is quite good I think. Perhaps file an issue against this repository for better editing guidelines? Ideally with the questions you'd like to see addressed.
I'm also missing a description of all the new
templateelement attributes (and their addition to various indexes).
Sorry - can you clarify what you mean here? Do you just mean I need a definition of the "shadowroot" content attribute? Can you point me to the right place to put that? Currently, it just lives inline in the "An end tag whose tag name is template" section.
As for documentation, there is some advice in the README as well as pointers to various other documents, but I don't think we have anything directly covering wattsi (HTML) and Bikeshed (all other WHATWG standards, including DOM), though Bikeshed has https://tabatkins.github.io/bikeshed/ which is quite good I think. Perhaps file an issue against this repository for better editing guidelines? Ideally with the questions you'd like to see addressed.
Thanks. Will do.
The top of the source resource indicates which places to edit for a new attribute:
- The IDL and content attributes for the relevant elements
- element and attribute indexes
In this case IDL would not be changed as this is a parser-driven attribute, similar to <html manifest> (which is now gone), but it does need to be listed in the other places and have a description in the template element section.
@rniwa thanks for the updated review! I'm glad you're interested in the feature. I made some replies, and I'll make a few changes to the PR early next week, including splitting off the serialization stuff into a separate PR.
It also sounds like you're prototyping the "streaming" version, where the shadow root gets attached at the opening tag. Is that true? If so, I'd be very interested to hear more, since we decided not to go that route based on your specific concerns about changing the parser. I'd be happy to re-open that discussion, if there's room to discuss! It'd be interesting to hear what you found while prototyping. In particular, @smaug---- has performance concerns that are at least partially related to the overhead of moving template contents around when the closing </template> is parsed, and those would be eliminated by a streaming implementation.
@rniwa thanks for the updated review! I'm glad you're interested in the feature. I made some replies, and I'll make a few changes to the PR early next week, including splitting off the serialization stuff into a separate PR.
Ok, I got my branch back up to date, removed the serialization stuff, and renamed "include shadow roots state" to "allow declarative shadow DOM flag". I believe I fixed up all of the merge issues, but let me know if you see something I missed.
Ok, I got my branch back up to date, removed the serialization stuff, and renamed "include shadow roots state" to "allow declarative shadow DOM flag". I believe I fixed up all of the merge issues, but let me know if you see something I missed.
Well, hold on while I figure out what causes "parse error while closing p element". So descriptive.
Ok, I got my branch back up to date, removed the serialization stuff, and renamed "include shadow roots state" to "allow declarative shadow DOM flag". I believe I fixed up all of the merge issues, but let me know if you see something I missed.
Well, hold on while I figure out what causes "parse error while closing p element". So descriptive.
So 20 minutes of bisecting my changes tells me that this error is caused by <span>include shadow roots flag<span>, but I have no idea what I'm doing wrong there. I followed the example I see elsewhere, e.g. "process link headers" where there is one <dfn> and other referencing <span>s. @domenic any advice appreciated. One huge tooling suggestion would be to add a bit more detail than "parse error" to tell me what is broken.
It's literally a HTML parse error, because your closing tag is missing a /
Well that is just exceedingly embarrassing. Thanks. Will fix. Ugh.
It's literally a HTML parse error, because your closing tag is missing a /
Well that is just exceedingly embarrassing. Thanks. Will fix. Ugh.
I guess the linter may have been improved or something - that error was there from a year and a half ago. Or maybe it just never passed even then. Anyway, thanks, that definitely fixes things. There's still a missing <dfn> error, but I'll look at that tomorrow.
There's still a missing
<dfn>error, but I'll look at that tomorrow.
That's now fixed too. This should be good to go.
Still some validation errors on the build output:
"file:/whatwg/output/index.html":76742.332-76742.334: error: Element “p” not allowed as child of element “ol” in this context. (Suppressing further errors from this subtree.)
looks like a missing <li> or something.
@annevk, can you take point on this review, as DOM editor?
Still some validation errors on the build output:
Ahh - I wasn't running the conformance checker. Seems like that should be part of build.sh maybe?
The error has been fixed at least locally, let's see what Github says.
Seems like that should be part of build.sh maybe?
It adds about 5 minutes to the build process, so we omit it...
Seems like that should be part of build.sh maybe?
It adds about 5 minutes to the build process, so we omit it...
Hmm, ok understandable then. Perhaps an option, like build.sh --check-conformance?
Thanks for your work on this @mfreed7. I would say that modulo some details WebKit can be considered supportive and I don't think Mozilla is objecting (@smaug----?) so it seems okay to proceed here. I've done a mostly editorial pass. I suspect this doesn't have streaming shadow DOM just yet? I'll take a look at the DOM side as well.
Thanks! You are correct that this doesn't include the details of streaming yet. I'm hoping @rniwa can perhaps suggest how to best incorporate that, using the knowledge from his prototype?
Thanks for your work on this @mfreed7. I would say that modulo some details WebKit can be considered supportive and I don't think Mozilla is objecting (@smaug----?) so it seems okay to proceed here. I've done a mostly editorial pass. I suspect this doesn't have streaming shadow DOM just yet? I'll take a look at the DOM side as well.
Thanks! You are correct that this doesn't include the details of streaming yet. I'm hoping @rniwa can perhaps suggest how to best incorporate that, using the knowledge from his prototype?
Sure, the simplest approach is to hook into where template element's start tag is parsed, and instead of inserting the template element to its parent, create a template element with a shadow root for the parent as its document fragment. Still push the template element to the stack of open so that all the checks for closing template, etc... will work same as a regular template element does. The template element only exists in abstract & as a parent node of its immediate (content) child nodes.
Sure, the simplest approach is to hook into where template element's start tag is parsed, and instead of inserting the template element to its parent, create a template element with a shadow root for the parent as its document fragment. Still push the template element to the stack of open so that all the checks for closing template, etc... will work same as a regular template element does. The template element only exists in abstract & as a parent node of its immediate (content) child nodes.
Thanks! Does this approach sound reasonable to other folks? If so, I can try adding it to this PR, and prototyping it in Chromium.
Thanks! Does this approach sound reasonable to other folks? If so, I can try adding it to this PR, and prototyping it in Chromium.
Also, @rniwa did you happen to write any WPTs to test the streaming behavior?
Thanks! Does this approach sound reasonable to other folks? If so, I can try adding it to this PR, and prototyping it in Chromium.
Also, @rniwa did you happen to write any WPTs to test the streaming behavior?
Assuming this seems reasonable, I've updated this PR to attempt to represent the streaming version of DSD. I followed the approach of @rniwa's prototype. I've also updated the Chromium implementation accordingly, which included modifying a few WPTs accordingly. The changes to the tests were fairly trivial, which is hopefully a good sign that this will be a mostly-compatible change to make.
@rniwa can you review the new parser bits? It seems many of the old comments are still outstanding so maybe just look at the parser aspects for now.
Yep, I haven't forgotten about your comments, thanks! Addressing them is next on my list. I just wanted to get a first cut of the streaming stuff in here, in case there were major objections or problems.
So I still stand with my comment that declarative shadow DOM inside other template elements and allowing cloning of declarative shadow DOM shouldn't be a thing. Instead, we should keep declarative shadow DOM inside template elements "regular" template, and create a new method like HTMLTemplateElement.prototype.createInstance, which recursively handles the inner declarative shadow DOMs.