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. |
Joey Arhar (Gerrit)
Joey Arhar 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.)
David BaronA 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>`.)
I set update_select to false in that case, but it still times out on selectedcontent-in-option-crash.html
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.)
David BaronA 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.
Joey ArharI 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>`.)
I set update_select to false in that case, but it still times out on selectedcontent-in-option-crash.html
Did you try my test in https://github.com/whatwg/html/pull/11878#discussion_r2603233851 ? Do you know if it has the problem that I hypothesized (leaving a `<selectedcontent>` with stale content) without the change I'm suggesting?
(It might be worth adding that as a test.)
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")
// 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.)
David BaronA 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.
Joey ArharI 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>`.)
David BaronI set update_select to false in that case, but it still times out on selectedcontent-in-option-crash.html
Did you try my test in https://github.com/whatwg/html/pull/11878#discussion_r2603233851 ? Do you know if it has the problem that I hypothesized (leaving a `<selectedcontent>` with stale content) without the change I'm suggesting?
(It might be worth adding that as a test.)
Yes, it has the selectedcontent with stale content issue. As I mentioned in that github thread, I don't believe we should try to fix this case.
I removed the update_select flag to match the current state of the spec PR. Can you review the spec as well like Anne asked?
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. |
Joey Arhar (Gerrit)
Joey Arhar voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
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 7 comments![]( "Open in Gerrit")
So I've pointed out a bunch of issues below -- although my suspicion is that the list probably isn't complete.
I think it would probably help to decide what invariants should be maintained and then try to maintain them. I think that's true at a code level, for example, what exactly should `descendant_selectedcontents_` contain, what exactly should `nearest_ancestor_select_` be set to, etc. I think it's probably also true at an HTML spec level (when should a given `<selectedcontents>` element reflect the contents of the selected option), although I think I have a little less influence over the HTML spec part (particularly given the PR is already merged). (Below I've pointed out some issues related to both of these questions, though I don't think the list of either is complete.)
descendant_selectedcontent->CloneContentsFromOptionElement(nullptr);For what it's worth, `CloneContentsFromOptionElement` bails out if the `selectedcontent` is disabled, which doesn't match the spec. The spec says that you shouldn't get to this code at all if the newly-inserted `selectedcontent` is disabled (which is true), but then it says you should in fact clear any `selectedcontent` elements that aren't the first, whether or not they're disabled.
(I think perhaps it would make more sense to also clear the first if it is disabled! I think maybe that would be an alternative fix for some of my consistency concerns.)
(I think the reason behind those consistency concerns is that I think the original rationale for algorithmic specs is that they make it much clearer how to deal with conflicts between different aspects of descriptive specs. However, I still think it's good for the underlying behavior to be something you can describe in a straightforward way without having to run the whole algorithm, and I don't think that's what's happening here. And in particular I think the behavior here is dependent on the order of mutations that led to the current DOM in ways that I don't think it needs to be, whereas in general I think it's good for the algorithms in the spec to follow the rule that the order of mutations leading to a particular DOM shouldn't affect the behavior of that static DOM.)
select->SelectedContentElementInserted(this);So here you're calling `SelectedContentElementInserted` on a select element that this element may have *already* been inserted in. In particular, `nearest_ancestor_select_` may well have already been set to it at the start of this function (for example, if the entire select element was in a subtree that was removed, and it's now being inserted into something else).
In that case, you're going to call `TreeOrderedList::Add` *again* and I think end up with it in the list twice... which means on removal you're going to end up with it still in the list.
More generally, I think this CL could use clearer invariants about what rules are supposed to hold for `nearest_ancestor_select_`, `descendant_selectedcontents_`, etc. -- I think right now it's pretty easy to poke holes here.
!nearest_ancestor_select_->IsMultiple()) {Should the spec mention this `IsMultiple()` condition somewhere?
return;Doesn't this risk leaving `nearest_ancestor_select_` in a weird state? Would it make more sense for the `disabled_` check to guard only the call to `SelectedContentElementRemoved` (which I think still matches the spec behavior but doesn't leave the potentially-weird `nearest_ancestor_select_`)?
Joey ArharIs 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?
Acknowledged
Traversal<HTMLSelectElement>::FirstAncestor(removed_from)) {What if `removed_from` was the `<select>`?
```suggestion
Traversal<HTMLSelectElement>::FirstAncestorOrSelf(removed_from)) {
```
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 voted and added 6 comments![]( "Open in Gerrit")
Votes added by Joey Arhar
| Commit-Queue | +1 |
So I've pointed out a bunch of issues below -- although my suspicion is that the list probably isn't complete.
I think it would probably help to decide what invariants should be maintained and then try to maintain them. I think that's true at a code level, for example, what exactly should `descendant_selectedcontents_` contain, what exactly should `nearest_ancestor_select_` be set to, etc. I think it's probably also true at an HTML spec level (when should a given `<selectedcontents>` element reflect the contents of the selected option), although I think I have a little less influence over the HTML spec part (particularly given the PR is already merged). (Below I've pointed out some issues related to both of these questions, though I don't think the list of either is complete.)
I think there will be more significant changes coming due to this: https://github.com/whatwg/html/issues/12096#issuecomment-3817882213
Hopefully this simplifies things by removing the invariant that the first selectedcontent element in dom order is the one that is always up to date.
I still want to get this patch merged in order to get the next one merged and keep things moving to avoid more situations like this: https://github.com/whatwg/html/pull/11891#issuecomment-3744421971
descendant_selectedcontent->CloneContentsFromOptionElement(nullptr);For what it's worth, `CloneContentsFromOptionElement` bails out if the `selectedcontent` is disabled, which doesn't match the spec. The spec says that you shouldn't get to this code at all if the newly-inserted `selectedcontent` is disabled (which is true), but then it says you should in fact clear any `selectedcontent` elements that aren't the first, whether or not they're disabled.
(I think perhaps it would make more sense to also clear the first if it is disabled! I think maybe that would be an alternative fix for some of my consistency concerns.)
(I think the reason behind those consistency concerns is that I think the original rationale for algorithmic specs is that they make it much clearer how to deal with conflicts between different aspects of descriptive specs. However, I still think it's good for the underlying behavior to be something you can describe in a straightforward way without having to run the whole algorithm, and I don't think that's what's happening here. And in particular I think the behavior here is dependent on the order of mutations that led to the current DOM in ways that I don't think it needs to be, whereas in general I think it's good for the algorithms in the spec to follow the rule that the order of mutations leading to a particular DOM shouldn't affect the behavior of that static DOM.)
Yeah I will try following up on checking disabled in that method. Hopefully I can add an assert to the spec that it isn't disabled in that case: https://chromium-review.googlesource.com/c/chromium/src/+/7568753
The keeping all selectedcontent elements up to date thing will also likely impact this.
I am also tweaking that in the next followup which I hope to merge very soon: https://chromium-review.googlesource.com/c/chromium/src/+/7128135/6/third_party/blink/renderer/core/html/forms/html_selected_content_element.cc
select->SelectedContentElementInserted(this);So here you're calling `SelectedContentElementInserted` on a select element that this element may have *already* been inserted in. In particular, `nearest_ancestor_select_` may well have already been set to it at the start of this function (for example, if the entire select element was in a subtree that was removed, and it's now being inserted into something else).
In that case, you're going to call `TreeOrderedList::Add` *again* and I think end up with it in the list twice... which means on removal you're going to end up with it still in the list.
More generally, I think this CL could use clearer invariants about what rules are supposed to hold for `nearest_ancestor_select_`, `descendant_selectedcontents_`, etc. -- I think right now it's pretty easy to poke holes here.
I believe I fixed this in the next patch by moving the call to SelectedContentElementInserted to HTMLSelectedContentElement::InsertedInto: https://chromium-review.googlesource.com/c/chromium/src/+/7128135/6/third_party/blink/renderer/core/html/forms/html_selected_content_element.cc
!nearest_ancestor_select_->IsMultiple()) {Should the spec mention this `IsMultiple()` condition somewhere?
Yes, but I am going to make a spec PR soon to remove all IsMultiple checks as part of this: https://issues.chromium.org/issues/483769482
return;Doesn't this risk leaving `nearest_ancestor_select_` in a weird state? Would it make more sense for the `disabled_` check to guard only the call to `SelectedContentElementRemoved` (which I think still matches the spec behavior but doesn't leave the potentially-weird `nearest_ancestor_select_`)?
I see, I added a TODO to remove the disabled check after we stop doing cloning during removal.
Traversal<HTMLSelectElement>::FirstAncestor(removed_from)) {What if `removed_from` was the `<select>`?
```suggestion
Traversal<HTMLSelectElement>::FirstAncestorOrSelf(removed_from)) {
```
- 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. |