Repository navigation
Keep a pick in a multiple select that becomes a drop-down - #216
Conversation
With preserveChanges, an option the user selected in a multiple select was lost when the target turned the select into a drop-down: removing `multiple` lets the browser keep a single option of its own choosing, and an option the markup selects then takes the selection too. Picks are now noted in multiple selects as well, and given back after `multiple` is removed, as they already are after a `selected` or `checked` update. The form-state fuzzer also expected a typed textarea to take a target that the morph gives to an untouched textarea with the same text, once a typed twin of that untouched one has taken its first target. It now counts that target as taken. Nightly fuzz seeds 39013173021, 39013194161 and 38701502299. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NCD2TWTPy7idzkWwiWwefa
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf77285468
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…mes a drop-down Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NCD2TWTPy7idzkWwiWwefa
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8eb1549f29
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // one the morph is about to remove, which would leave the drop-down showing neither. | ||
| function picksKeptLast(held: Array<Pick>, to: HTMLSelectElement): Array<Pick> { | ||
| const values = new Set(Array.from(to.options, (option) => option.value)) | ||
| const kept = (pick: Pick) => isOptionElement(pick) && values.has(pick.value) |
There was a problem hiding this comment.
When two dirty selected options share a value but only one survives in the target, values.has classifies both as kept. For example, if selected <option id="keep" value="x"> and <option id="drop" value="x"> are preceded by an unselected option and the target retains only keep, this ordering restores the doomed drop last; child reconciliation then removes it and the drop-down falls back to the first option, losing the surviving user pick. Fresh evidence beyond the prior comment is that duplicate-valued options defeat the new value-only partition, so this needs to distinguish which live options can actually survive, including duplicate counts or keyed identity.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Traced it. It needs four things at once: two options with the same value in one multiple select, the user selecting both, the target dropping one of them, and the same morph turning the select into a drop-down. Two options with the same value in one select submit the same thing, so markup like that is rare, and combined with the rest it's a pathological case. Telling which twin survives would mean matching options before the children are reconciled, which is far more code than the case is worth under the project's proportionate-fix rule. So I'm leaving this as is and putting the call to the maintainer.
Generated by Claude Code
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NCD2TWTPy7idzkWwiWwefa
Requested by Joel · project thread
Before: with
preserveChanges, if the user selected an option in a<select multiple>and the target turned it into a drop-down, the user's option was lost. The browser keeps an option of its own choosing whenmultiplegoes away, and an option the markup selects then takes the selection too. The nightly fuzz failed on this twice (seeds 39013173021 and 39013194161, both on Oct 17).After: the user's option stays selected, as it already does in a select that was a drop-down all along (#208).
Picks are now noted in multiple selects too, and given back after
multipleis removed, the same way they're given back after aselectedorcheckedupdate. When the user picked several options, the ones whose value the target still has are given back last, so a pick the morph removes doesn't leave the drop-down on the default. An option gainingselectedin a select that stays multiple can't take anything from the others, so it doesn't gather the picks at all.The third failing seed (38701502299, Oct 15) was an oracle gap in the form-state fuzzer, not a library bug. The user typed in two textareas: one whose text matched an untouched sibling, and one with other text that the target removed. The fuzzer expected both typed textareas to survive. The morph gives the leftover target to the untouched textarea with the same text, as the design notes on identical siblings describe. The fuzzer now counts that target as taken.
How:
isPickno longer excludes multiple selects. The attribute removal pass gets the held picks before removingmultipleand gives them back after, ordered bypicksKeptLast. There are three new tests inchanged-default-pick, and each fails without its fix.Known gap, left on purpose: when two options in one select share a value, the user selects both, and the target drops one of them while making the select a drop-down, the doomed twin can still be given back last. See the review thread for why this isn't worth matching options early.
Checks: format, lint, typecheck, and the full Chromium suite with 100% coverage all pass. All three seeds pass, and so do 12,500 fresh form-state seeds (2,500 of them in preserve-only mode). Benchmarked against main with the perf-sweep harness, and every scenario is within noise.
🤖 Generated with Claude Code
https://claude.ai/code/session_01NCD2TWTPy7idzkWwiWwefa