Re: [whatwg/dom] Add declarative Shadow DOM features (#892)

@annevk commented on this pull request.

It largely looks okay, but still a lot of nits that make it hard to spot the real issues.

> @@ -4363,6 +4363,28 @@ dom-Range-extractContents, dom-Range-cloneContents -->
  <a for=tree>children</a> of <var>node</var> and append them to <var>copy</var>, with
  <var>document</var> as specified and the <i>clone children flag</i> being set.
 
+ <li>If <var>node</var> is a <a for=Element>shadow host</a> whose <a for=/>shadow root</a>'s
+ <a for=ShadowRoot>clonable</a> flag is set, run these steps:
+
+ <ol>
+   <li><p>Run <a>attach a shadow root</a> with <var>shadow host</var> equal to

Nit: indentation is wrong. We use single space.

> +   <li><p>Run <a>attach a shadow root</a> with <var>shadow host</var> equal to
+   <var>copy</var>, <var>mode</var> equal to <var>node</var>'s <a for=/>shadow root</a>'s
+   <a for=ShadowRoot>mode</a>, <var>clonable</var> equal to true, and
+   <var>delegates focus</var> equal to <var>node</var>'s <a for=/>shadow root</a>'s
+   <a>delegates focus</a>.</p></li>
+
+   <li><p>If <var>node</var>'s <a for=/>shadow root</a>'s
+   <a for=ShadowRoot>is declarative shadow root</a> is true, then set <var>copy</var>'s
+   <a for=/>shadow root</a>'s <a for=ShadowRoot>is declarative shadow root</a> property to
+   true.</p></li>
+
+   <li>If the <i>clone children flag</i> is set, <a lt="clone a node">clone</a> all the
+   <a>children</a> of <var>node</var>'s <a for=/>shadow root</a> and append them to
+   <var>copy</var>'s <a for=/>shadow root</a>, with <var>document</var> as specified,
+   and the <i>clone children flag</i> set.
+

NitL spurious newline.

> @@ -4862,8 +4884,9 @@ known as <dfn export id=concept-document lt="document">documents</dfn>.
 <dfn export for=Document id=concept-document-content-type>content type</dfn> (a string),
 <dfn export for=Document id=concept-document-url>URL</dfn> (a <a for=/>URL</a>),
 <dfn export for=Document id=concept-document-origin>origin</dfn> (an <a for=/>origin</a>),
-<dfn export for=Document id=concept-document-type>type</dfn> ("<code>xml</code>" or "<code>html</code>"), and
-<dfn export for=Document id=concept-document-mode>mode</dfn> ("<code>no-quirks</code>", "<code>quirks</code>", or "<code>limited-quirks</code>").
+<dfn export for=Document id=concept-document-type>type</dfn> ("<code>xml</code>" or "<code>html</code>"),
+<dfn export for=Document id=concept-document-mode>mode</dfn> ("<code>no-quirks</code>", "<code>quirks</code>", or "<code>limited-quirks</code>"), and
+<dfn export for=Document id=concept-document-allow-declarative-shadow-dom>allow declarative shadow DOM</dfn> ("<code>allow</code>", "<code>deny</code>", or unset).</p>

Couple comments:

* This should be "shadow roots" not "shadow DOM". We don't really use DOM as a term-of-art internally.
* No need to use ID for new concepts.
* Unset is highly unclear. If this is a tri-state let's give all a string name.

> @@ -5762,13 +5787,19 @@ It is initially set to false.</p>
 <p><a for=/>Shadow roots</a> have an associated
 <dfn export for=ShadowRoot>available to element internals</dfn>. It is initially set to false.</p>
 
+<p><a for=/>Shadow roots</a> have an associated <dfn export for=ShadowRoot>is declarative shadow root</dfn>.
+It is initially set to false.</p>

It would be nice to start declaring the type here as well. So after the definition have " (a boolean)" as we do for the enumerations. We can fix the others in a follow-up PR.

> +<p>The <dfn attribute for=Element><code>shadowRoot</code></dfn> attribute's getter must run these
+steps:

This should use the "getter steps" language as it does today.

> @@ -6727,11 +6759,37 @@ are:
 <p>The <dfn method for=Element><code>attachShadow(<var>init</var>)</code></dfn> method steps are:
 
 <ol>
- <li><p>If <a>this</a>'s <a for=Element>namespace</a> is not the <a>HTML namespace</a>,
+
+ <li><p>Run <a>attach a shadow root</a> with <var>shadow host</var> equal to <a>this</a>,
+   <var>mode</var> equal to <var>init</var>'s {{ShadowRootInit/mode}}, <var>clonable</var>
+   equal to <var>init</var>'s {{ShadowRootInit/clonable}}, and <var>delegates focus</var>
+   equal to <var>init</var>'s {{ShadowRootInit/delegatesFocus}}.</p></li>

(This also applies to the HTML PR.)

You shouldn't repeat the argument names here as they are not named arguments (see Infra). You should just invoke it as "Run X with _y_ and _z_."

> +</ol>
+
+<p>The <dfn attribute for=Element><code>shadowRoot</code></dfn> attribute's getter must run these
+steps:
+
+<ol>
+ <li><p>Let <var>shadow</var> be <a>this</a>'s <a for=Element>shadow root</a>.
+
+ <li><p>If <var>shadow</var> is null or its <a for=ShadowRoot>mode</a> is "<code>closed</code>",
+ then return null.</p></li>
+
+ <li><p>Return <var>shadow</var>.
+</ol>
+
+<p>To <dfn id=concept-attach-a-shadow-root>attach a shadow root</dfn>, given a
+<var>shadow host</var>, <var>mode</var>, <var>clonable</var>, and <var>delegates focus</var>,

For clarity we should have the argument types here. See Infra for guidance.

>   then <a>throw</a> a "{{NotSupportedError!!exception}}" {{DOMException}}.
 
- <li>
-  <p>If <a>this</a>'s <a for=Element>local name</a> is not one of the following:
+ <li><p>If <var>shadow host</var>'s <a for=Element>local name</a> is <em>not</em> a

Apart from replacing `<a>this</a>` nothing else should have changed here, including the newline.

> @@ -6774,15 +6832,23 @@ are:
   </ol>
  </li>
 
- <li><p>If <a>this</a> is a <a for=Element>shadow host</a>, then <a>throw</a> an
- "{{NotSupportedError!!exception}}" {{DOMException}}.
+ <li><p>If <var>shadow host</var> has a non-null <a for=/>shadow root</a>, then:

Why can this not use the original language of "If _shadow host_ is a shadow host"?

> +</ol>
+
+<p>The <dfn attribute for=Element><code>shadowRoot</code></dfn> attribute's getter must run these
+steps:
+
+<ol>
+ <li><p>Let <var>shadow</var> be <a>this</a>'s <a for=Element>shadow root</a>.
+
+ <li><p>If <var>shadow</var> is null or its <a for=ShadowRoot>mode</a> is "<code>closed</code>",
+ then return null.</p></li>
+
+ <li><p>Return <var>shadow</var>.
+</ol>
+
+<p>To <dfn id=concept-attach-a-shadow-root>attach a shadow root</dfn>, given a
+<var>shadow host</var>, <var>mode</var>, <var>clonable</var>, and <var>delegates focus</var>,

Also, _shadowHost_ and _delegatesFocus_.

> @@ -6774,15 +6832,23 @@ are:
   </ol>
  </li>
 
- <li><p>If <a>this</a> is a <a for=Element>shadow host</a>, then <a>throw</a> an
- "{{NotSupportedError!!exception}}" {{DOMException}}.
+ <li><p>If <var>shadow host</var> has a non-null <a for=/>shadow root</a>, then:
+   <ol>
+     <li><p>If <var>shadow host</var>'s <a for=/>shadow root</a>'s <a for=ShadowRoot>is declarative

Nit: indentation.

> @@ -6791,20 +6857,9 @@ are:
  <li><p>Set <var>shadow</var>'s <a for=ShadowRoot>slot assignment</a> to
  <var>init</var>["{{ShadowRootInit/slotAssignment}}"].
 
- <li><p>Set <a>this</a>'s <a for=Element>shadow root</a> to <var>shadow</var>.
+ <li><p>Set <var>shadow</var>'s <a for=ShadowRoot>is declarative shadow root</a> property to false.

```suggestion
 <li><p>Set <var>shadow</var>'s <a for=ShadowRoot>is declarative shadow root</a> to false.
```

> @@ -6727,11 +6759,37 @@ are:
 <p>The <dfn method for=Element><code>attachShadow(<var>init</var>)</code></dfn> method steps are:
 
 <ol>
- <li><p>If <a>this</a>'s <a for=Element>namespace</a> is not the <a>HTML namespace</a>,
+
+ <li><p>Run <a>attach a shadow root</a> with <var>shadow host</var> equal to <a>this</a>,
+   <var>mode</var> equal to <var>init</var>'s {{ShadowRootInit/mode}}, <var>clonable</var>
+   equal to <var>init</var>'s {{ShadowRootInit/clonable}}, and <var>delegates focus</var>
+   equal to <var>init</var>'s {{ShadowRootInit/delegatesFocus}}.</p></li>
+
+ <li><p>Return <a>this</a>'s <a for=Element>shadow root</a>.</p></li>
+

Nit: spurious newline.

> @@ -6774,15 +6832,23 @@ are:
   </ol>
  </li>
 
- <li><p>If <a>this</a> is a <a for=Element>shadow host</a>, then <a>throw</a> an
- "{{NotSupportedError!!exception}}" {{DOMException}}.
+ <li><p>If <var>shadow host</var> has a non-null <a for=/>shadow root</a>, then:
+   <ol>
+     <li><p>If <var>shadow host</var>'s <a for=/>shadow root</a>'s <a for=ShadowRoot>is declarative
+     shadow root</a> property is false, then <a>throw</a> an "{{NotSupportedError!!exception}}" {{DOMException}}.

Not a property.

> @@ -6774,15 +6832,23 @@ are:
   </ol>
  </li>
 
- <li><p>If <a>this</a> is a <a for=Element>shadow host</a>, then <a>throw</a> an
- "{{NotSupportedError!!exception}}" {{DOMException}}.
+ <li><p>If <var>shadow host</var> has a non-null <a for=/>shadow root</a>, then:
+   <ol>
+     <li><p>If <var>shadow host</var>'s <a for=/>shadow root</a>'s <a for=ShadowRoot>is declarative
+     shadow root</a> property is false, then <a>throw</a> an "{{NotSupportedError!!exception}}" {{DOMException}}.
+
+     <li><p>Otherwise, <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>.

It seems weird for this algorithm to sometimes return something. That doesn't seem right.

> @@ -4363,6 +4363,28 @@ dom-Range-extractContents, dom-Range-cloneContents -->
  <a for=tree>children</a> of <var>node</var> and append them to <var>copy</var>, with
  <var>document</var> as specified and the <i>clone children flag</i> being set.
 
+ <li>If <var>node</var> is a <a for=Element>shadow host</a> whose <a for=/>shadow root</a>'s
+ <a for=ShadowRoot>clonable</a> flag is set, run these steps:

```suggestion
 <a for=ShadowRoot>clonable</a> is true:
```
Right?

Also, needs to use `<p>` on a newline as the `<li>` has multiple children.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/dom/pull/892#pullrequestreview-1200578466
You are receiving this because you are subscribed to this thread.

Message ID: <whatwg/dom/pull/892/review/1200578466@github.com>