Further Attr node fixes for compareDocumentPosition() #312
Conversation
|
(Please don't merge, just review.) |
|
|
||
| <ol> | ||
| <li> | ||
| <p>For each <a>attribute</a> <var>node2Attribute</var> in <var>attr2</var>'s | ||
| <a for=Attr>element</a>'s <a for=Element>attribute list</a>: | ||
| <p>For each <a>attribute</a> <var>node2Attr</var> in <var>node2</var>'s |
There was a problem hiding this comment.
Segfault, here it's possible for node1 and node2 to both be null while attr1 is non-null. I think the check needs to be "If node1 and attr1 are non-null and node1 is node2" or similar, i.e. add a null-check for node2.
There was a problem hiding this comment.
Reading this anew, I also wonder if it'd be more clear to call node2Attr simply attr or something without a 2 in it, since node1==node2 here.
fa39e5c to
675f7c1
Compare
| </ol> | ||
| <li> | ||
| <p>If <var>node1</var>’s <a data-link-type="dfn" href="#concept-tree-root">root</a> is not <var>node2</var>’s <a data-link-type="dfn" href="#concept-tree-root">root</a> or <var>node1</var> and <var>node2</var> are <a data-link-type="dfn" href="#concept-attribute">attributes</a>, then return the result of adding <code class="idl"><a data-link-type="idl" href="#dom-node-document_position_disconnected">DOCUMENT_POSITION_DISCONNECTED</a></code>, <code class="idl"><a data-link-type="idl" href="#dom-node-document_position_implementation_specific">DOCUMENT_POSITION_IMPLEMENTATION_SPECIFIC</a></code>, and | ||
| <p>If <var>node1</var> or <var>node2</var> is null, or <var>node1</var>’s <a data-link-type="dfn" href="#concept-tree-root">root</a> is | ||
| not <var>node2</var>’s <a data-link-type="dfn" href="#concept-tree-root">root</a>, then return the result of adding <code class="idl"><a data-link-type="idl" href="#dom-node-document_position_disconnected">DOCUMENT_POSITION_DISCONNECTED</a></code>, <code class="idl"><a data-link-type="idl" href="#dom-node-document_position_implementation_specific">DOCUMENT_POSITION_IMPLEMENTATION_SPECIFIC</a></code>, and | ||
| either <code class="idl"><a data-link-type="idl" href="#dom-node-document_position_preceding">DOCUMENT_POSITION_PRECEDING</a></code> or <code class="idl"><a data-link-type="idl" href="#dom-node-document_position_following">DOCUMENT_POSITION_FOLLOWING</a></code>, with the | ||
| constraint that this is to be consistent, together. </p> | ||
| <p class="note no-backref" role="note">Whether to return <code class="idl"><a data-link-type="idl" href="#dom-node-document_position_preceding">DOCUMENT_POSITION_PRECEDING</a></code> or <code class="idl"><a data-link-type="idl" href="#dom-node-document_position_following">DOCUMENT_POSITION_FOLLOWING</a></code> is typically implemented via pointer comparison. In | ||
| JavaScript implementations a cached <code class="lang-javascript highlight"><span class="nb">Math</span><span class="p">.</span><span class="nx">random</span><span class="p">()</span> </code> value can |
There was a problem hiding this comment.
Looks like a Bikeshed bug on this line, an extra space before </code> that you can see in the rendered result. @tabatkins, known issue?
|
OK, finished reviewing, including fixes up to "node2attr -> attr". Things that are impossible just take longer. |
|
I think I fixed the last two non-bikeshed issues. |
|
Looking into the Bikeshed issue. |
| adding {{Node/DOCUMENT_POSITION_CONTAINS}} to {{Node/DOCUMENT_POSITION_PRECEDING}}. | ||
|
|
||
| <li><p>If <var>node1</var> is a <a>descendant</a> of <var>node2</var>, then return the result of | ||
| <li><p>If <var>node1</var> is a <a>descendant</a> of <var>node2</var> and <var>attr1</var> is null, |
There was a problem hiding this comment.
Oops, this one needs to say "and attr2 is null"
|
Reviewed "review comments", found a typo, but otherwise this now looks great, I like the added note! |
d90bb5f to
3f10adf
Compare
This algorithm is impossible. Hopefully fixes #309.
|
And should be fixed. |
|
Thanks Tab! |
This algorithm is impossible. Hopefully fixes #309.