Re: [whatwg/dom] Draft integration with Trusted Types, take 2. (PR #1247)

@annevk commented on this pull request.



> @@ -6408,6 +6411,18 @@ steps:
  <a for=Attr>value</a>.
 </ol>
 
+<p>To <dfn id=concept-element-attributes-validate-and-set-value>validate and set attribute value</dfn>
+<var>value</var> for an <a>attribute</a> <var>attribute</var>, with <a for=/>element</a>

Nit: type of value should be specified.

> @@ -6408,6 +6411,18 @@ steps:
  <a for=Attr>value</a>.
 </ol>
 
+<p>To <dfn id=concept-element-attributes-validate-and-set-value>validate and set attribute value</dfn>
+<var>value</var> for an <a>attribute</a> <var>attribute</var>, with <a for=/>element</a>
+<var>element</var>, run these steps:

Modern style is to leave out "run these steps" when the sentence starts with "To" (and maybe in some other cases).
```suggestion
<var>element</var>:
```

> @@ -6758,10 +6790,20 @@ method steps are:
  and null otherwise.
  <!-- This is step 2 of "get an attribute by name", modified as appropriate -->
 
- <li><p>If <var>attribute</var> is null, create an <a>attribute</a> whose
- <a for=Attr>local name</a> is <var>qualifiedName</var>, <a for=Attr>value</a> is
- <var>value</var>, and <a for=Node>node document</a> is <a>this</a>'s <a for=Node>node document</a>,
- then <a lt="append an attribute">append</a> this <a>attribute</a> to <a>this</a>, and then return.
+ <li><p>If <var>attribute</var> is null, then:

Wrapping is still wrong here. The `<li>` has multiple children so the `<p>` needs to be newlined and indented. The `<ol>` below doesn't seem indented enough either. Remember that it's a child of the `<li>`.

> + <ol>
+  <li><p>Set <var>attribute</var> to a new <a>attribute</a> whose <a for=Attr>namespace</a> is
+  <var>namespace</var>, <a for=Attr>namespace prefix</a> is <var>prefix</var>,
+  <a for=Attr>local name</a> is <var>localName</var> and <a for=Node>node document</a> is
+  <var>element</var>'s <a for=Node>node document</a>.
+
+  <li><p><a>Validate and set attribute value</a> <var>value</var> for <var>attribute</var> with
+  <var>element</var>.
+
+  <li><p><a lt="append an attribute">Append</a> <var>attribute</var> to <var>element</var>.
+
+  <li><p>Return.
+ </ol>

This seems wrong. This should be behind the "If attribute is null" check above, presumably?

> + <ol>
+  <li><p>Set <var>attribute</var> to a new <a>attribute</a> whose <a for=Attr>namespace</a> is
+  <var>namespace</var>, <a for=Attr>namespace prefix</a> is <var>prefix</var>,
+  <a for=Attr>local name</a> is <var>localName</var> and <a for=Node>node document</a> is
+  <var>element</var>'s <a for=Node>node document</a>.
+
+  <li><p><a>Validate and set attribute value</a> <var>value</var> for <var>attribute</var> with
+  <var>element</var>.
+
+  <li><p><a lt="append an attribute">Append</a> <var>attribute</var> to <var>element</var>.
+
+  <li><p>Return.
+ </ol>

However, I'm also not sure why this cannot be part of "append an attribute" which would give us some deduplication. If you do the validation before the attribute is appended to the element, it seems fine? What am I missing?

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

Message ID: <whatwg/dom/pull/1247/review/1843623912@github.com>