Repository navigation
Keep all descendant selectedcontent elements up to date - #12263
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.
| <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}
The option removing steps set the option's cached nearest ancestor select element to null unconditionally. But the removing steps run for every shadow-including inclusive descendant of the removed node, not just the subtree root, so removing an ancestor of a select detached every contained option from a select that still contains it. After that, ask for a reset bailed out on the null cache and the selectedness setting algorithm never ran again for that select. Instead, fold the cache update and the selectedness setting algorithm calls back into "update an option's nearest ancestor select", which recomputes the new select by walking the tree, and have it return the list of select elements whose selectedcontent elements need updating. The removing and moving steps now share that one algorithm, which also removes the near-duplicate old/new branches from the moving steps. Chromium already behaves this way, so this aligns the text with the shipping implementation rather than changing behavior. Also: * Drop the "whose nearest select ancestor is select" clause when collecting descendant selectedcontent elements. It used an undefined term, and "recalculate a selectedcontent element's disabledness" already sets disabled to true for a selectedcontent in a nested select, so the disabled clause covers it. * Use the conventional "setter steps are:" form with "the given value" for the selected setter. * Fix the selectedness setting algorithm preamble: it doubled "run" and ended a "They return" sentence with a colon. * Use a colon rather than a comma for inline "For each X of Y:" steps. Tests: https://github.com/web-platform-tests/wpt/pull/…
A non-multiple select whose display size is 1 and which has an enabled option has to have a selected option; the selectedness setting algorithm establishes that. But several attribute changes could leave a select without one, and none of them ran the algorithm: * Removing multiple, or lowering size so that the display size becomes 1, turns the select into a control that needs a selection. * Enabling an option, or enabling its nearest ancestor optgroup, can leave a select whose only enabled option is not selected. An option's disabledness depends on both attributes, so both need covering. * Adding or removing the selected content attribute sets selectedness directly, so it could leave two options selected in a non-multiple select. Add "reset a select's selectedness", which runs the selectedness setting algorithm and updates descendant selectedcontent elements only when selectedness actually changed, and invoke it from all four. While here, turn the selected attribute prose into steps so the ordering against the reset is explicit. Chromium already behaves this way for size, multiple, and selected; only the two disabled attributes need an implementation change. This closes every route to a select with no selected option despite being non-multiple, having a display size of 1, and having an enabled option. The option post-connection steps can therefore return early when the inserted option is not selected, which stops them from running "replace all" on every descendant selectedcontent element on every insertion. That early return is only sound because of the resets above.
|
Thanks, I incorporated that feedback, and the commits you pushed look good too! |
Removing multiple keeps the first selected option, and always updates selectedcontent, which is not kept up to date while multiple is present. The selectedIndex, value, and selected setters can clone custom elements into selectedcontent, so they need [CEReactions]. See whatwg/html#12263.
The option removing and moving steps run for every shadow-including inclusive descendant of the mutated node, not just the subtree root. Removing or moving an ancestor of a select therefore must not detach the contained options from the select, which still contains them. Nothing covered this; every existing removal test targets an option, a selectedcontent, or the select itself. Also test that a selectedcontent element inside a nested select is disabled and never updated, which is what lets "update a select's descendant selectedcontent elements" collect descendants by disabledness alone without checking which select is nearest. See whatwg/html#12263.
The option post-connection steps update every descendant selectedcontent element of the option's select unconditionally, so inserting an option that changes nothing still runs "replace all" on the selectedcontent. That is observable through node identity and mutation records, and it scales with the number of options inserted. Guarding on the inserted option's own selectedness is not sufficient, because inserting an option can select a different one: enabling an option does not run the selectedness setting algorithm, so a select can be left with an enabled but unselected option, and the next insertion selects it. The last test covers that case and asserts after a microtask so that it stays orthogonal to whether the update happens in the post-connection steps or in a microtask. Together these require the update to be conditional on whether selectedness actually changed, not on which option triggered the insertion. See whatwg/html#12263.
Cover all four attributes that can leave a non-multiple select with a display size of 1 without a selected option: size, multiple, an option's disabled, and an optgroup's disabled. Also cover the selected content attribute, which sets selectedness directly and so could leave two options selected, including the case where dirtiness means it does not. selectedcontent-attribute-change.html covers the same five for selectedcontent, plus that an attribute change which leaves selectedness alone does not run "replace all" on the selectedcontent. Drop the third test from selectedcontent-option-insertion.html: it reached a select with an enabled but unselected option by enabling one, which is no longer possible. See whatwg/html#12263.
Removing multiple keeps the first selected option, and always updates selectedcontent, which is not kept up to date while multiple is present. The selectedIndex, value, and selected setters can clone custom elements into selectedcontent, so they need [CEReactions]. See whatwg/html#12263.
The crash on whatwg/html#12263 could only be located by noticing which `ls` line was missing, and Node's own memory numbers looked healthy while the machine ran out. - memory() snapshots now add the machine's MemTotal, MemAvailable and Shmem (tmpfs counts there) from /proc/meminfo, and the space used on the tmp dir's filesystem, where each can be read. - Each Wattsi build step (fetch, extract, diff, rewrite, html-dfn.js) logs when it starts and how it ended, with its duration, a memory snapshot and the size of the PR's scratch dir. The diff's file counts are logged too. - The unpacked-files listing uses readdir instead of spawning `ls`, only runs when debug logging is on, and no longer goes through a process.nextTick that did nothing. - WattsiClient takes an optional logger, for tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017fa64HAQWfNnsexnq3duiT
…e builds die (#219) * Shrink the Wattsi scratch footprint and always clean it up An instance on a 512 MiB flavor was OOM-killed while unzipping the second whatwg/html build, with Node's own RSS around 110 MB. The scratch files are the likely culprit: each Wattsi zip holds far more than we use, and both zips plus both unpacked trees sat in the tmp dir at once. - Extract only multipage-html/ (plus xrefs.json from the head build), and delete each zip once it is unpacked. - Clean up the per-PR scratch directory when the job ends, whatever the outcome. It used to be removed only at the end of a successful cacheAll(), so failed or skipped builds left their files behind. - Update DEPLOYMENT.md: the app runs on nano, and describes what an OOM kill looks like in the log. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017fa64HAQWfNnsexnq3duiT * Log where the Wattsi build is, and the machine's memory, for OOM hunting The crash on whatwg/html#12263 could only be located by noticing which `ls` line was missing, and Node's own memory numbers looked healthy while the machine ran out. - memory() snapshots now add the machine's MemTotal, MemAvailable and Shmem (tmpfs counts there) from /proc/meminfo, and the space used on the tmp dir's filesystem, where each can be read. - Each Wattsi build step (fetch, extract, diff, rewrite, html-dfn.js) logs when it starts and how it ended, with its duration, a memory snapshot and the size of the PR's scratch dir. The diff's file counts are logged too. - The unpacked-files listing uses readdir instead of spawning `ls`, only runs when debug logging is on, and no longer goes through a process.nextTick that did nothing. - WattsiClient takes an optional logger, for tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017fa64HAQWfNnsexnq3duiT --------- Co-authored-by: Claude <noreply@anthropic.com>
https://bugs.webkit.org/show_bug.cgi?id=320187 rdar://183736776 Reviewed by NOBODY (OOPS!). Align <selectedcontent> with whatwg/html#12263 A <selectedcontent> element now works out whether it is disabled when it is inserted, not only once it is connected. That way a <selectedcontent> nested in another one is never updated, and those of a disconnected <select> are. When a <selectedcontent> is connected, it only updates itself. Inserting a selected <option> now updates <selectedcontent> from the option's post-connection steps, which also covers options inside wrapper elements. Removing or moving a selected <option>, or moving a <selectedcontent>, updates it in a microtask instead. The moving steps of <option> now also run when an ancestor of the option is moved, so it follows its new <select>. That lets dom/nodes/moveBefore/select-option-optgroup.html run again. A form reset now updates <selectedcontent> as well. Updating <selectedcontent> in response to size and disabled changes is left for a follow-up. selectedcontent-subtree-mutations.html is skipped because moveBefore() of a <select> double-registers the named slot in its shadow tree. Tests: imported/w3c/web-platform-tests/html/semantics/forms/the-select-element/customizable-select/selectedcontent-attribute-change.html imported/w3c/web-platform-tests/html/semantics/forms/the-select-element/customizable-select/selectedcontent-nested-select.html imported/w3c/web-platform-tests/html/semantics/forms/the-select-element/customizable-select/selectedcontent-option-insertion.html imported/w3c/web-platform-tests/html/semantics/forms/the-select-element/customizable-select/selectedcontent-subtree-mutations.html imported/w3c/web-platform-tests/html/semantics/forms/the-select-element/select-ancestor-subtree-removal.html imported/w3c/web-platform-tests/html/semantics/forms/the-select-element/select-attribute-change-reset.html Tests upstream: web-platform-tests/wpt#62242
https://bugs.webkit.org/show_bug.cgi?id=320187 rdar://183736776 Reviewed by Ryosuke Niwa. Align <selectedcontent> with whatwg/html#12263 A <selectedcontent> element now works out whether it is disabled when it is inserted, not only once it is connected. That way a <selectedcontent> nested in another one is never updated, and those of a disconnected <select> are. When a <selectedcontent> is connected, it only updates itself. Inserting a selected <option> now updates <selectedcontent> from the option's post-connection steps, which also covers options inside wrapper elements. Removing or moving a selected <option>, or moving a <selectedcontent>, updates it in a microtask instead. The moving steps of <option> now also run when an ancestor of the option is moved, so it follows its new <select>. That lets dom/nodes/moveBefore/select-option-optgroup.html run again. A form reset now updates <selectedcontent> as well. Updating <selectedcontent> in response to size and disabled changes is left for a follow-up. selectedcontent-subtree-mutations.html is skipped because moveBefore() of a <select> double-registers the named slot in its shadow tree. Tests: imported/w3c/web-platform-tests/html/semantics/forms/the-select-element/customizable-select/selectedcontent-attribute-change.html imported/w3c/web-platform-tests/html/semantics/forms/the-select-element/customizable-select/selectedcontent-nested-select.html imported/w3c/web-platform-tests/html/semantics/forms/the-select-element/customizable-select/selectedcontent-option-insertion.html imported/w3c/web-platform-tests/html/semantics/forms/the-select-element/customizable-select/selectedcontent-subtree-mutations.html imported/w3c/web-platform-tests/html/semantics/forms/the-select-element/select-ancestor-subtree-removal.html imported/w3c/web-platform-tests/html/semantics/forms/the-select-element/select-attribute-change-reset.html Tests upstream: web-platform-tests/wpt#62242 Canonical link: https://commits.webkit.org/322148@main
…ness changes https://bugs.webkit.org/show_bug.cgi?id=325639 Reviewed by NOBODY (OOPS!). whatwg/html#12263 made changing the size or multiple attribute of <select>, or the disabled attribute of <option> or <optgroup>, reset the selectedness of the <select>, updating <selectedcontent> when that changes the selected option. A <select> no longer selects its first option when all of its options are disabled, as the specification requires nothing to be selected then. The parser also no longer selects an option inside a disabled <optgroup> by default, a mistake from 310930@main. Chromium and Gecko already behave this way. Removing the multiple attribute when no option is selected no longer resets the options to their default selectedness and dirtiness. It selects the first enabled option instead. Enabling an <optgroup> now selects its first option when nothing is selected. It used to recalculate the selection before updating its own disabledness.
This is a follow-up to #12263 that addresses the remaining cases where selectedcontent elements were updated too little or too much: * Inserting an option element that causes another option element to become selected now updates selectedcontent elements. This happens when the select element has no selected option, e.g., after setting selectedIndex to -1. * Inserting a select element that already contains option elements now updates its selectedcontent elements once, rather than again from the post-connection steps of its selected option element. * The microtasks queued to update selectedcontent elements after option elements are removed or moved are now coalesced per select element, and do nothing if the selectedcontent elements were updated in the meantime. The same goes for moving a selectedcontent element. * The microtask queued by the selectedcontent moving steps now determines the select element when it runs, rather than when it is queued. * Moving a selectedcontent element no longer updates it when its select element and disabledness do not change, e.g., when an ancestor of the select element is moved. * Removing a selectedcontent element now recalculates its disabledness. * Setting the selected IDL attribute of an option element only updates selectedcontent elements when selectedness changes. This also removes "ask for a reset" and "update descendant selectedcontent elements for an option", which would otherwise each have a single caller. Tests: TBD.
…ness changes https://bugs.webkit.org/show_bug.cgi?id=325639 Reviewed by Ryosuke Niwa and Tim Nguyen. whatwg/html#12263 made changing the size or multiple attribute of <select>, or the disabled attribute of <option> or <optgroup>, reset the selectedness of the <select>, updating <selectedcontent> when that changes the selected option. A <select> no longer selects its first option when all of its options are disabled, as the specification requires nothing to be selected then. The parser also no longer selects an option inside a disabled <optgroup> by default, a mistake from 310930@main. Chromium and Gecko already behave this way. Removing the multiple attribute when no option is selected no longer resets the options to their default selectedness and dirtiness. It selects the first enabled option instead. Enabling an <optgroup> now selects its first option when nothing is selected. It used to recalculate the selection before updating its own disabledness. Canonical link: https://commits.webkit.org/322349@main
Automatic update from web-platform-tests More <selectedcontent> coverage See whatwg/html#12263. -- wpt-commits: b08095447cc5207285244b0b462f2367c9b64cac wpt-pr: 62242
Previously only the first selectedcontent descendant of a select element was kept up to date. Removing it meant updating the next one from the removing steps, which mutates the tree while it is in an inconsistent state. Now every descendant selectedcontent element that is not disabled is kept up to date, and removing one has no effect on the others. Whether a selectedcontent element is disabled is now determined when it is inserted, so selectedcontent elements nested inside each other are never updated.
As removing and moving steps cannot mutate the tree, the updates that result from removing or moving an option element, or from moving a selectedcontent element, happen in a microtask. Inserting a selected option element or a selectedcontent element still updates synchronously.
In addition:
Tests: web-platform-tests/wpt#57842, web-platform-tests/wpt#58089, and web-platform-tests/wpt#62242.
Fixes #11880, fixes #11883, and fixes #12096.
(See WHATWG Working Mode: Changes for more details.)
/common-dom-interfaces.html ( diff )
/form-elements.html ( diff )