Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RadioNodeList should be a live view #25415

Closed
pshaughn opened this issue Jan 2, 2020 · 3 comments
Closed

RadioNodeList should be a live view #25415

pshaughn opened this issue Jan 2, 2020 · 3 comments

Comments

@pshaughn
Copy link
Member

@pshaughn pshaughn commented Jan 2, 2020

The concept of a live NodeList appears in several places, much like the concept of a live HTMLCollection. Currently, the only kind of live NodeList we have is for a ParentNode's children. Three other cases need a live one: the labels of a labelable element (issue #25391), the result of Document.getElementsByName (issue #25147), and RadioNodeList. [querySelectorAll and MutationRecord have NodeLists but are not meant to be live]

https://html.spec.whatwg.org/multipage/forms.html#dom-form-nameditem step 1 and https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#dom-htmlformcontrolscollection-nameditem step 4 are the two places where a RadioNodeList can be created; both specify that it's live, so RadioNodeList should always be live.

The two specifications for what the RadioNodeList exposes do not seem identical at first glance, and the implementation might need two different states to cover them both:
"all the listed [form-associated] elements, whose form owner is the form element, that have either an id attribute or a name attribute equal to name, with the exception of input elements whose type attribute is in the Image Button state, in tree order."
" a live view of the HTMLFormControlsCollection object, further filtered so that the only nodes in the RadioNodeList object are those that have either an id attribute or a name attribute equal to name. The nodes in the RadioNodeList object must be sorted in tree order."

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Jan 2, 2020

To clarify: the non-compliant thing Servo is doing here is that adding or removing nodes from the form won't update an already-created RadioNodeList that is supposed to be a live view. This is separate from the RadioNodeList's value, which is itself already live but is backed by a static collection of nodes instead of a live collection of nodes.

@jdm jdm added the A-content/dom label Jan 2, 2020
@jdm jdm added this to To do in web-platform-test failures via automation Jan 2, 2020
@jdm jdm added the B-high-value label Jan 2, 2020
@jdm
Copy link
Member

@jdm jdm commented Jan 2, 2020

Marking this as high value since this seems like the kind of behaviour that existing web content could be relying upon (especially via getElementsByName and the named form getter) that would break.

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Jan 4, 2020

There are indeed two cases, but the two lines I listed above are actually one case, as the only way to get an HTMLFormControlsCollection is backed by a single form element and excludes image-buttons.
The other case is https://html.spec.whatwg.org/multipage/forms.html#dom-form-nameditem step 2, in which the list contains only image-buttons. Even though we don't really have working image-button support, we do have the type="image" state itself defined, so we can implement and test the NodeList behavior for this case. I'm thinking RadioNodeList gets a Dom<HTMLFormElement> for the form that produced it, a DOMString for the name it's reflecting, and an internal boolean to indicate whether it's an image-buttons list or an everything-but-image-buttons list; other than that boolean it's highly analogous to Document.getElementsByName. so I can reuse a lot of what I did in #25428.

@pshaughn pshaughn self-assigned this Jan 4, 2020
bors-servo added a commit that referenced this issue Jan 6, 2020
Removing .get() from some property descriptor field assignments to see what happens

This is towards #25036 and #25415; it could have far-reaching implications so I want to test it in isolation and see the results on the full test suite.

A few lines of code had a FIXME(#11868) despite that issue being closed, and looking for the pattern that was marked that way, I found one other unmarked instance of it. It doesn't immediately crash, and maybe it will pass some tests or fail some tests in informative ways.

---
<!-- 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
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- 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 10, 2020
Make getOwnPropertyDescriptor hold the correct value for indexed/named properties

This is towards #25036 and #25415; it could have far-reaching implications so I want to test it in isolation and see the results on the full test suite.

A few lines of code had a FIXME(#11868) despite that issue being closed, and looking for the pattern that was marked that way, I found one other unmarked instance of it. It doesn't immediately crash, and maybe it will pass some tests or fail some tests in informative ways.

EDIT: After adding an overlooked extended attribute to HTMLFormElement, this works very well indeed and seems to be worth merging!

---
<!-- 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 #25036

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- 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 10, 2020
Make getOwnPropertyDescriptor hold the correct value for indexed/named properties

This is towards #25036 and #25415; it could have far-reaching implications so I want to test it in isolation and see the results on the full test suite.

A few lines of code had a FIXME(#11868) despite that issue being closed, and looking for the pattern that was marked that way, I found one other unmarked instance of it. It doesn't immediately crash, and maybe it will pass some tests or fail some tests in informative ways.

EDIT: After adding an overlooked extended attribute to HTMLFormElement, this works very well indeed and seems to be worth merging!

---
<!-- 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 #25036

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- 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 10, 2020
Make getOwnPropertyDescriptor hold the correct value for indexed/named properties

This is towards #25036 and #25415; it could have far-reaching implications so I want to test it in isolation and see the results on the full test suite.

A few lines of code had a FIXME(#11868) despite that issue being closed, and looking for the pattern that was marked that way, I found one other unmarked instance of it. It doesn't immediately crash, and maybe it will pass some tests or fail some tests in informative ways.

EDIT: After adding an overlooked extended attribute to HTMLFormElement, this works very well indeed and seems to be worth merging!

---
<!-- 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 #25036

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- 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. -->
web-platform-test failures automation moved this from To do to Done Jan 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.