Update selectedcontent element to match spec [chromium/src : main]
David Baron (Gerrit)
David Baron added 9 comments![]( "Open in Gerrit")
This patch implements the behavior in the spec behind a flag.maybe say that the flag is enabled by default, since my default interpretation when reading "behind a flag" is that the flag isn't on yet
for (const T* node : nodes_) {
if (node == node_to_check) {
return true;
}
}
return false;Just use the `LinkedHashSet` implementation?
```suggestion
return nodes_.Contains(node_to_check);
```
Although probably also inline it in the class definition at that point?
// <seletedoption> can lead to infinite loops.```suggestion
// <selectedcontent> can lead to infinite loops.
```
// The HTML spec has a "break" here, but we continue instead because weI don't see this "break" in the spec (reading the [post-connection steps](https://html.spec.whatwg.org/multipage/form-elements.html#the-selectedcontent-element:html-element-post-connection-steps)). (Though I also don't see anything that makes the code wrong, just the comment.)
}
if (auto* select = DynamicTo<HTMLSelectElement>(ancestor)) {```suggestion
} else if (auto* select = DynamicTo<HTMLSelectElement>(ancestor)) {
```
if (nearest_ancestor_select_ && !disabled_) {This doesn't seem sufficient. If a `<select>` initially has a working `<selectedcontent>`, and then somebody inserts a new one *before* it that the algorithm makes *disabled*, I think we need to update the select at that point so that the previously-working one gets cleared, no? (That said, I'm reading the spec somewhat quickly so I may have missed something.)
// <seletedoption> can lead to infinite loops.```suggestion
// <selectedcontent> can lead to infinite loops.
```
// the spec since the spec doesn't have a dached nearest_ancestor_select_, but```suggestion
// the spec since the spec doesn't have a cached nearest_ancestor_select_, but
```
// the behavior is the same.Is the behavior the same? Won't there be detectable mutations here that the spec says shouldn't happen?
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 9 comments![]( "Open in Gerrit")
This patch implements the behavior in the spec behind a flag.maybe say that the flag is enabled by default, since my default interpretation when reading "behind a flag" is that the flag isn't on yet
good point, i changed the flag to experimental
for (const T* node : nodes_) {
if (node == node_to_check) {
return true;
}
}
return false;Just use the `LinkedHashSet` implementation?
```suggestion
return nodes_.Contains(node_to_check);
```Although probably also inline it in the class definition at that point?
Done
// <seletedoption> can lead to infinite loops.```suggestion
// <selectedcontent> can lead to infinite loops.
```
Done
// The HTML spec has a "break" here, but we continue instead because weI don't see this "break" in the spec (reading the [post-connection steps](https://html.spec.whatwg.org/multipage/form-elements.html#the-selectedcontent-element:html-element-post-connection-steps)). (Though I also don't see anything that makes the code wrong, just the comment.)
A break is being added here: https://github.com/whatwg/html/pull/11878
I added a link to it in the commit message.
I suppose that I could wait for the PR to be merged first, but I found it helpful to implement in order to review that PR.
}
if (auto* select = DynamicTo<HTMLSelectElement>(ancestor)) {```suggestion
} else if (auto* select = DynamicTo<HTMLSelectElement>(ancestor)) {
```
Done
if (nearest_ancestor_select_ && !disabled_) {This doesn't seem sufficient. If a `<select>` initially has a working `<selectedcontent>`, and then somebody inserts a new one *before* it that the algorithm makes *disabled*, I think we need to update the select at that point so that the previously-working one gets cleared, no? (That said, I'm reading the spec somewhat quickly so I may have missed something.)
Without this disabled check, this test will infinitely loop: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/html/semantics/forms/the-select-element/customizable-select/selectedcontent-in-option-crash.html
I'm not really inclined to make cases like this any less broken since putting a selectedcontent element inside another one or inside an option is not something that anybody should ever do. I can try to make it clear selectedcontent elements without doing any cloning though if you still think we should.
// <seletedoption> can lead to infinite loops.```suggestion
// <selectedcontent> can lead to infinite loops.
```
Done
// the spec since the spec doesn't have a dached nearest_ancestor_select_, but```suggestion
// the spec since the spec doesn't have a cached nearest_ancestor_select_, but
```
Done
// the behavior is the same.Is the behavior the same? Won't there be detectable mutations here that the spec says shouldn't happen?
I added DCHECKs with logic which more closely matches the spec. How does it look?
Related details
- David Baron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron added 1 comment![]( "Open in Gerrit")
if (nearest_ancestor_select_ && !disabled_) {Joey ArharThis doesn't seem sufficient. If a `<select>` initially has a working `<selectedcontent>`, and then somebody inserts a new one *before* it that the algorithm makes *disabled*, I think we need to update the select at that point so that the previously-working one gets cleared, no? (That said, I'm reading the spec somewhat quickly so I may have missed something.)
Without this disabled check, this test will infinitely loop: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/html/semantics/forms/the-select-element/customizable-select/selectedcontent-in-option-crash.html
I'm not really inclined to make cases like this any less broken since putting a selectedcontent element inside another one or inside an option is not something that anybody should ever do. I can try to make it clear selectedcontent elements without doing any cloning though if you still think we should.
As I commented on the PR, would it make sense here if we have a separate `updateSelect` boolean that gets set to `false` if we make the `selectedcontent` `disabled` because of the `selectedcontent` ancestor check, but not if it's because of the `option` ancestor check?
(Also, I'm still thinking about the multiple-select-ancestors case; I just realized the spec is wrong there and I need to make another comment on the PR!)
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
if (nearest_ancestor_select_ && !disabled_) {Joey ArharThis doesn't seem sufficient. If a `<select>` initially has a working `<selectedcontent>`, and then somebody inserts a new one *before* it that the algorithm makes *disabled*, I think we need to update the select at that point so that the previously-working one gets cleared, no? (That said, I'm reading the spec somewhat quickly so I may have missed something.)
David BaronWithout this disabled check, this test will infinitely loop: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/html/semantics/forms/the-select-element/customizable-select/selectedcontent-in-option-crash.html
I'm not really inclined to make cases like this any less broken since putting a selectedcontent element inside another one or inside an option is not something that anybody should ever do. I can try to make it clear selectedcontent elements without doing any cloning though if you still think we should.
As I commented on the PR, would it make sense here if we have a separate `updateSelect` boolean that gets set to `false` if we make the `selectedcontent` `disabled` because of the `selectedcontent` ancestor check, but not if it's because of the `option` ancestor check?
(Also, I'm still thinking about the multiple-select-ancestors case; I just realized the spec is wrong there and I need to make another comment on the PR!)
I tried adding an updateSelect boolean, but it still makes selectedcontent-in-option-crash.html have an infinite loop. I can debug and provide more context, but does it look like I implemented it correctly in the latest patch set?
Related details
- David Baron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron added 1 comment![]( "Open in Gerrit")
// The HTML spec has a "break" here, but we continue instead because weJoey ArharI don't see this "break" in the spec (reading the [post-connection steps](https://html.spec.whatwg.org/multipage/form-elements.html#the-selectedcontent-element:html-element-post-connection-steps)). (Though I also don't see anything that makes the code wrong, just the comment.)
A break is being added here: https://github.com/whatwg/html/pull/11878
I added a link to it in the commit message.
I suppose that I could wait for the PR to be merged first, but I found it helpful to implement in order to review that PR.
I think it's also probably ok to set `update_select` to false in the case a few lines lower where you set `disabled_` to true because of nested selects.
(I think the case where updating is clearly needed is the case where you're inserting a `<selectedcontent>` inside an `<option>`.)
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |