Clone into all descendant selectedcontent elements#12263
Clone into all descendant selectedcontent elements#12263josepharhar wants to merge 22 commits intowhatwg:mainfrom
Conversation
This PR improves the "clear a select's non-primary selectedcontent elements" algorithm by making it create a list of selectedcontent elements to modify separately from modifying them in order to prevent the list of elements to change while iterating. Fixes whatwg#11880
This PR makes sure that the contents of the selectedcontent element stay up to date when the selected option is changed in the selectedness setting algorithm. This issue was found here: web-platform-tests/wpt#55849 (comment)
In order to make the selectedness setting algorithm match implementations, this PR makes the selectedness setting algorithm avoid changing the selectedness of option elements which haven't ran their insertion steps yet by checking whether the options have their cached nearest ancestor select element assigned yet or not. This was discussed here: whatwg#11825
…electedcontentredo
…contentredo Merged the PRs, now I need to do the selectedcontent rewrite to clone into all descendant selectedcontent elements.
annevk
left a comment
There was a problem hiding this comment.
Thanks @josepharhar! Here's some minor editorial feedback to start. Will try to verify these changes by implementing them.
source
Outdated
| <li><p>Let <var>descendantSelectedcontents</var> be « ».</p></li> | ||
|
|
||
| <li><p>Let <var>option</var> be the first <code>option</code> in <var>select</var>'s <span | ||
| data-x="concept-select-option-list">list of options</span> whose <span | ||
| data-x="concept-option-selectedness">selectedness</span> is true, if any such <code>option</code> | ||
| exists, otherwise null.</p></li> | ||
| <li> | ||
| <p>For each <var>descendant</var> of <var>select</var>'s <span | ||
| data-x="descendant">descendants</span>:</p> | ||
|
|
||
| <li><p>If <var>option</var> is null, then run <span>clear a <code>selectedcontent</code></span> | ||
| given <var>selectedcontent</var>.</p></li> | ||
| <ol> | ||
| <li><p>If <var>descendant</var> is a <code>selectedcontent</code> element, then <span | ||
| data-x="list append">append</span> <var>descendant</var> to | ||
| <var>descendantSelectedcontents</var>.</p></li> | ||
| </ol> | ||
| </li> |
There was a problem hiding this comment.
We don't need a loop for this. We can state this declaratively:
Let selectedContents be select's descendants that are selectedcontent elements, in tree order.
There was a problem hiding this comment.
Also, I have a more substantive question here. Why doesn't this skip disabled selectedcontent elements immediately?
I think the selectedcontent-nested.html test currently expects this, but it seems wasteful and best avoided.
There was a problem hiding this comment.
We don't need a loop for this. We can state this declaratively:
Done, thanks
Also, I have a more substantive question here. Why doesn't this skip disabled selectedcontent elements immediately?
Disabled selectedcontents will be skipped deeper in the algorithms called by this one - "clone an option into a selectedcontent" and "clear a selectedcontent" both check if the selectedcontent is disabled before modifying the DOM, and return early.
We could add a step to skip disabled selectedcontents here, but it would functionally be redundant, unless I'm misreading something. What do you think? Can we leave it as-is?
There was a problem hiding this comment.
Can you explain https://github.com/web-platform-tests/wpt/blob/master/html/semantics/forms/the-select-element/customizable-select/selectedcontent-nested.html then? Isn't parentSelectedcontent disabled once it is inserted?
There was a problem hiding this comment.
Ok, yes I see how it would change the behavior to check if the selectedcontent elements are disabled before adding them to the list. I'll change the spec here and update the WPT.
This changes the behavior when selecting a new option in the case that there are nested selectedcontent elements inside the select. Before, the disabled selectedcontent would get cloned, but now it won't. Context: whatwg/html#12263 (comment) Bug: 458113204 Change-Id: I9afd550f21787b7daefb48c5a76712b5899e5b42
This changes the behavior when selecting a new option in the case that there are nested selectedcontent elements inside the select. Before, the disabled selectedcontent would get cloned, but now it won't. Context: whatwg/html#12263 (comment) Bug: 458113204 Change-Id: I9afd550f21787b7daefb48c5a76712b5899e5b42 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7737623 Reviewed-by: David Grogan <dgrogan@chromium.org> Reviewed-by: Joey Arhar <jarhar@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1611875}
This changes the behavior when selecting a new option in the case that there are nested selectedcontent elements inside the select. Before, the disabled selectedcontent would get cloned, but now it won't. Context: whatwg/html#12263 (comment) Bug: 458113204 Change-Id: I9afd550f21787b7daefb48c5a76712b5899e5b42 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7737623 Reviewed-by: David Grogan <dgrogan@chromium.org> Reviewed-by: Joey Arhar <jarhar@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1611875}
This changes the behavior when selecting a new option in the case that there are nested selectedcontent elements inside the select. Before, the disabled selectedcontent would get cloned, but now it won't. Context: whatwg/html#12263 (comment) Bug: 458113204 Change-Id: I9afd550f21787b7daefb48c5a76712b5899e5b42 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7737623 Reviewed-by: David Grogan <dgrogan@chromium.org> Reviewed-by: Joey Arhar <jarhar@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1611875}
|
I updated this PR to avoid updating the selectedcontent element in the insertion or removal steps of the option element as per this discussion: #12096 (comment) |
|
It seems that the post-connection steps are sync, while the removing steps queue a microtask. I think it would be more consistent if both were queueing a microtask? Also -- I think that reset would now also run script because it clones into selectedcontent, which I think it shouldn't (at least in Gecko it is marked as not script running). Should we also queue a microtask here? |
Yes it would be more consistent that way, but since the selectedcontent element was designed to be as synchronous as possible I figured that this was a good way to keep it synchronous more of the time. I also think that inserting an option which become selected is more common than removing the currently selected option. If others think that both should do a microtask, I'm ok with that.
Yes, reset causes cloning into selectedcontent. Why should reset not be script running? |
This PR changes several things:
Fixes #12096
Fixes #11880
Fixes #11883
Fixes #11825
(See WHATWG Working Mode: Changes for more details.)
/form-elements.html ( diff )