Make Attr inherit from Node again #299

annevk · 2016-08-16T14:58:16Z

Unfortunately AttrExodus never quite became the success we all wished
it would be and the benefits with Attr no longer taking children (yay)
are rather marginal anyway.

The biggest change is to the compareDocumentPosition() algorithm. Other
noteworthy changes are that all attribute creation now needs to set a
node document.

Fixes #102.

annevk · 2016-08-16T14:59:59Z

I think one big thing that is not yet considered here is what should happen with certain existing APIs, such as mutation observers' observe(). Should that throw a TypeError if node is an attribute?

Are there other such APIs we need to consider?

@cdumez @foolip @smaug---- @Ms2ger @nox review appreciated.

Ms2ger · 2016-08-16T15:54:25Z

All APIs that take Node should throw when passed an Attr, IMO. Don't care much about which exception; some may historically have been defined as throwing InvalidNodeTypeError or HierarchyRequestError or something.

annevk · 2016-08-17T08:47:26Z

Most APIs will already throw HierarchyRequestError indeed. observe() doesn't seem to throw in implementations, but doesn't do anything either since we only ever queue attribute mutation records for elements. @smaug---- @dominiccooney @travisleithead @cdumez should we make observe() throw when passed an Attr node or just let it be useless as it is today?

annevk · 2016-08-17T08:52:26Z

More concretely, given that the current patch matches implementations, I recommend we go with this (ideally with review though) and do any "normative changes" in a follow up.

smaug---- · 2016-08-17T08:55:40Z

I guess not throwing is good here. We don't throw when one tries to observe attribute changes using document and subtree: false either, or we don't throw observing childList changes when target is a textnode etc. So there are already many possibly ways to observe something which will never happen.

foolip · 2016-08-17T20:34:49Z

dom.bs

@@ -1490,7 +1490,7 @@ can be used to explore this matter in more detail.

 <h3 id=node-trees>Node tree</h3>

-<p>{{Document}}, {{DocumentType}}, {{DocumentFragment}}, {{Element}}, {{Text}},
+<p>{{Document}}, {{DocumentType}}, {{DocumentFragment}}, {{Element}}, {{Attr}}, {{Text}},


Attr doesn't actually participate in any tree though, it can't be appended to anything nor have any children.

For the purposes of event dispatch it needs to say it does. I guess I should add a note.

Actually, maybe it doesn't.

annevk · 2016-08-18T05:23:09Z

Anything else?

foolip · 2016-08-18T14:06:05Z

dom.bs

@@ -3827,6 +3819,10 @@ the empty string instead, and then do as described below, switching on <a>contex
   <li><p><a>Replace all</a> with <var>node</var> within the <a>context object</a>.
  </ol>

+ <dt>{{Attr}}
+ <dd>


Is this extra <dd> intentional?

Unfortunately AttrExodus never quite became the success we all wished it would be and the benefits with Attr no longer taking children (yay) are rather marginal anyway. The biggest change is to the compareDocumentPosition() algorithm. Other noteworthy changes are that all attribute creation now needs to set a node document. Fixes #102.

foolip · 2016-08-18T20:57:05Z

dom.bs

@@ -4175,57 +4157,80 @@ returns as mask:
 <li><dfn const for=Node>DOCUMENT_POSITION_IMPLEMENTATION_SPECIFIC</dfn> (32, 20 in hexadecimal).
 </ul>

-The <dfn method for=Node>compareDocumentPosition(<var>other</var>)</dfn>
-method must run these steps:
+<p>The <dfn method for=Node><code>compareDocumentPosition(<var>other</var>)</code></dfn> method,


Here or later, wrap contains(other) below in <code> as well so that it's also orange?

foolip · 2016-08-18T22:23:49Z

Wow, compareDocumentPosition is so weird. @cdumez, care to review those bits for sanity?

In Blink (and WebKit still I presume), if both nodes attributes attached to different elements, things behave as if the attribute nodes are children of their elements, where attributes are considered to be before the real child nodes:
https://software.hixie.ch/utilities/js/live-dom-viewer/saved/4402

foolip · 2016-08-18T22:37:51Z

dom.bs

- <li>Otherwise, <a lt="change an attribute">change</a> the
- <a>context object</a> from <a>context object</a>'s
- <a for=Attr>element</a> to the given value.
+ <li>Otherwise, <a lt="change an attribute">change</a> <var>attribute</var> from


Internal dialog: "What does it mean to change the value from an element to something, the value wasn't an element to begin with?!"

This wasn't new in this PR, but maybe "To change an attribute attribute of an element element to value, run these steps" would result in more natural language at the call site?

I don't like the call site stating the types. And "an" makes it even worse I think since these are very specific instances. Perhaps at some point we can change the way this is done, or adopt the abstract operation syntax from ECMAScript (I think I'd favor that).

foolip · 2016-08-18T22:44:30Z

OK, I've gotten through all of the changes now, great job I must say 👍

Some follow-up will probably be needed as weird things are discovered, but that's OK.

cdumez · 2016-08-18T22:56:20Z

dom.bs

@@ -6697,23 +6715,19 @@ method, when invoked, must run these steps:

 <pre class=idl>
 [Exposed=Window]
-interface Attr {
+interface Attr : Node {
  readonly attribute DOMString? namespaceURI;


So this stays on Attr instead of going to Node?

cdumez · 2016-08-19T02:21:25Z

Nothing from me. Looks good.

annevk force-pushed the attr-odyssey branch from 93a1278 to 684e945 Compare August 17, 2016 11:12

foolip reviewed Aug 17, 2016
View reviewed changes

annevk mentioned this pull request Aug 18, 2016

Editorial: make text around "locate a namespace" more similar #304

Closed

annevk force-pushed the attr-odyssey branch from 356b352 to c7fd19b Compare August 18, 2016 05:22

foolip reviewed Aug 18, 2016
View reviewed changes

annevk added 3 commits August 18, 2016 16:41

doesn't participate in trees; does participate in namespace lookup

43c78c3

generate

bf4978a

annevk force-pushed the attr-odyssey branch from c7fd19b to bf4978a Compare August 18, 2016 14:41

remove extra dd

82df412

foolip reviewed Aug 18, 2016
View reviewed changes

cdumez reviewed Aug 18, 2016
View reviewed changes

annevk mentioned this pull request Aug 19, 2016

Editorial: wrap contains() in <code> #307

Closed

account for node1 and node2 still being attributes

6ae89ed

annevk merged commit 625a074 into master Aug 19, 2016

annevk deleted the attr-odyssey branch August 19, 2016 12:44

foolip mentioned this pull request Aug 22, 2016

compareDocumentPosition with Attr nodes still not quite right #309

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Attr inherit from Node again #299

Make Attr inherit from Node again #299

annevk commented Aug 16, 2016

annevk commented Aug 16, 2016

Ms2ger commented Aug 16, 2016

annevk commented Aug 17, 2016

annevk commented Aug 17, 2016

smaug---- commented Aug 17, 2016

foolip Aug 17, 2016

annevk Aug 18, 2016

annevk Aug 18, 2016

annevk commented Aug 18, 2016

foolip Aug 18, 2016

foolip Aug 18, 2016

annevk Aug 19, 2016

foolip commented Aug 18, 2016

foolip Aug 18, 2016

annevk Aug 19, 2016

foolip commented Aug 18, 2016

cdumez Aug 18, 2016

cdumez commented Aug 19, 2016

Make Attr inherit from Node again #299

Make Attr inherit from Node again #299

Conversation

annevk commented Aug 16, 2016

annevk commented Aug 16, 2016

Ms2ger commented Aug 16, 2016

annevk commented Aug 17, 2016

annevk commented Aug 17, 2016

smaug---- commented Aug 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Aug 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foolip commented Aug 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foolip commented Aug 18, 2016

Choose a reason for hiding this comment

cdumez commented Aug 19, 2016