-
Notifications
You must be signed in to change notification settings - Fork 3.1k
selectedcontent post-connection steps did not break out of the loop #11878
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
base: main
Are you sure you want to change the base?
Conversation
We don't use this ancestor variable. (The for each loop creates its own.)
| @@ -59187,12 +59185,15 @@ interface <dfn interface>HTMLSelectedContentElement</dfn> : <span>HTMLElement</s | |||
|
|
|||
| <li><p>Otherwise, set <var>selectedcontent</var>'s <span | |||
| data-x="selectedcontent-disabled">disabled</span> state to true.</p></li> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should move the below break into this "otherwise" condition. If we always break when the ancestor is a select element, then we can't detect the nested select elements case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also what the chromium implementation does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I think this is lacking test coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although if we end up finding ourselves in a nested situation, why do we still run update select's selectedcontents on the innermost select?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, we should not run that in this case. Can we add a check for selectedcontent-disabled in the line which returns early when nearestAncestorSelect is null or has the multiple attribute?
Good point, I think this is lacking test coverage.
I am hoping that this is exhaustive enough: https://chromium-review.googlesource.com/c/chromium/src/+/7125591
And also has an unused ancestor variable.
/form-elements.html ( diff )