Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAttr does not implement interface Node #25124
Closed
Labels
Projects
Comments
|
Several years ago the spec made changes to remove the Node parent from Attr, but I'm pretty sure it was deemed incompatible with the web and reverted since then. |
|
What's the right style for implementing something like the polymorphism in https://dom.spec.whatwg.org/#dom-node-nodevalue ? Make NodeMethods::GetNodeValue/SetNodeValue call an internal method that Attr/Text/etc have their own overrides of? The current implementation isn't being polymorphic at all, which it's getting away with because everything other than Attr is either null or the common case that's currently the only case. |
|
It's easiest to compare against the specification if the implementation in Node just performs a series of |
bors-servo
added a commit
that referenced
this issue
Dec 23, 2019
Attr is a Node, with consequences for many Node methods <!-- Please describe your changes on the following line: --> Attr is now a Node, as current WHATWG specs require. I think I did some unidiomatic things to make compareDocumentPosition work and it could use a look by someone more familiar with how to write concise Rust code. I also think the new cases in compareDocumentPosition lack tests; it is possible to compare the position of two attributes, or of an attribute and an element, and I don't think any tests are exercising that functionality. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #25124 <!-- Either: --> - [ ] There are tests for these changes (existing cases of Node methods are well-tested, and there is a WPT test specifically for whether Attr is Node, but I'm not sure about new Node method cases) <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo
added a commit
that referenced
this issue
Jan 3, 2020
Attr is a Node, with consequences for many Node methods <!-- Please describe your changes on the following line: --> Attr is now a Node, as current WHATWG specs require. I think I did some unidiomatic things to make compareDocumentPosition work and it could use a look by someone more familiar with how to write concise Rust code. I also think the new cases in compareDocumentPosition lack tests; it is possible to compare the position of two attributes, or of an attribute and an element, and I don't think any tests are exercising that functionality. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #25124 <!-- Either: --> - [ ] There are tests for these changes (existing cases of Node methods are well-tested, and there is a WPT test specifically for whether Attr is Node, but I'm not sure about new Node method cases) <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo
added a commit
that referenced
this issue
Jan 7, 2020
Attr is a Node, with consequences for many Node methods <!-- Please describe your changes on the following line: --> Attr is now a Node, as current WHATWG specs require. I think I did some unidiomatic things to make compareDocumentPosition work and it could use a look by someone more familiar with how to write concise Rust code. I also think the new cases in compareDocumentPosition lack tests; it is possible to compare the position of two attributes, or of an attribute and an element, and I don't think any tests are exercising that functionality. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #25124 <!-- Either: --> - [ ] There are tests for these changes (existing cases of Node methods are well-tested, and there is a WPT test specifically for whether Attr is Node, but I'm not sure about new Node method cases) <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
WPT dom/nodes/Document-importNode, last test.
doc is created using document.implementation.createHTMLDocument. setAttributeNS and getAttributeNodeNS are used on it, and then the output of getAttributeNodeNS is passed to document.importNode, which doesn't believe it's a Node. Comparing Servo's IDL to https://dom.spec.whatwg.org/#interface-attr it looks like Servo has implemented Node attributes directly into attr without inheritance.