Skip to content

Clone into all descendant selectedcontent elements - #12263

Open
josepharhar wants to merge 35 commits into
whatwg:mainfrom
josepharhar:selectedcontentredo
Open

Clone into all descendant selectedcontent elements#12263
josepharhar wants to merge 35 commits into
whatwg:mainfrom
josepharhar:selectedcontentredo

Conversation

@josepharhar

@josepharhar josepharhar commented Mar 16, 2026

Copy link
Copy Markdown
Contributor

This PR changes several things:

  • Instead of the first selectedcontent element in DOM order staying up to date, now all descendant selectedcontent elements of the select element will be kept up to date.
  • Rewrites the selectedness setting algorithm to prevent setting selectedness of options which haven't had their insertion steps run yet in order to match implementations and to update selectedcontent elements.
  • Creates a list of selectedcontent elements to update before doing actual DOM operations.

Fixes #12096
Fixes #11880
Fixes #11883
Fixes #11825

(See WHATWG Working Mode: Changes for more details.)


/form-elements.html ( diff )

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
…contentredo

Merged the PRs, now I need to do the selectedcontent rewrite to clone
into all descendant selectedcontent elements.

@annevk annevk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @josepharhar! Here's some minor editorial feedback to start. Will try to verify these changes by implementing them.

Comment thread source Outdated
Comment on lines +59315 to +59326
<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>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread source Outdated
Comment thread source Outdated
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 8, 2026
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
beckysiegel pushed a commit to chromium/chromium that referenced this pull request Apr 9, 2026
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}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 9, 2026
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}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 9, 2026
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}

@annevk annevk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The moving steps also haven't been addressed? And I believe the current set of algorithms would give us the Firefox outcome on #11825. Is that intentional?

Comment thread source Outdated
Comment thread source Outdated
Comment thread source Outdated
Comment thread source Outdated
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request May 12, 2026
…nsertion/removal steps, a=testonly

Automatic update from web-platform-tests
Don't update selectedcontent in option insertion/removal steps

Modifying the DOM by updating selectedcontent elements during insertion
or removal steps is bad for security reasons. We already removed the
selectedcontent element removal steps which was one way this can happen,
but the option's InsertedInto and RemovedFrom methods may change which
option is selected, which will synchronously update the selectedcontent
element.

This patch fixes this by moving the selectedcontent updating to the
post-insertion steps for insertion, and by using a microtask to update
on removal if needed.

This matches the behavior in the spec PR: whatwg/html#12263

Bug: 458113204
Change-Id: I42bb94c6eace93445cfbc816529e42ca8a561b94
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7745871
Reviewed-by: David Baron <dbaronchromium.org>
Reviewed-by: Joey Arhar <jarharchromium.org>
Commit-Queue: Joey Arhar <jarharchromium.org>
Cr-Commit-Position: refs/heads/main{#1622241}

--

wpt-commits: 8116dc1ced751ff3a62c58e020f3d7d0ad61553b
wpt-pr: 59528

UltraBlame original commit: 86fb81e4e646ca7e6da3fa87d0d72010474fb780
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request May 12, 2026
…nsertion/removal steps, a=testonly

Automatic update from web-platform-tests
Don't update selectedcontent in option insertion/removal steps

Modifying the DOM by updating selectedcontent elements during insertion
or removal steps is bad for security reasons. We already removed the
selectedcontent element removal steps which was one way this can happen,
but the option's InsertedInto and RemovedFrom methods may change which
option is selected, which will synchronously update the selectedcontent
element.

This patch fixes this by moving the selectedcontent updating to the
post-insertion steps for insertion, and by using a microtask to update
on removal if needed.

This matches the behavior in the spec PR: whatwg/html#12263

Bug: 458113204
Change-Id: I42bb94c6eace93445cfbc816529e42ca8a561b94
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7745871
Reviewed-by: David Baron <dbaronchromium.org>
Reviewed-by: Joey Arhar <jarharchromium.org>
Commit-Queue: Joey Arhar <jarharchromium.org>
Cr-Commit-Position: refs/heads/main{#1622241}

--

wpt-commits: 8116dc1ced751ff3a62c58e020f3d7d0ad61553b
wpt-pr: 59528

UltraBlame original commit: 86fb81e4e646ca7e6da3fa87d0d72010474fb780
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request May 12, 2026
…nsertion/removal steps, a=testonly

Automatic update from web-platform-tests
Don't update selectedcontent in option insertion/removal steps

Modifying the DOM by updating selectedcontent elements during insertion
or removal steps is bad for security reasons. We already removed the
selectedcontent element removal steps which was one way this can happen,
but the option's InsertedInto and RemovedFrom methods may change which
option is selected, which will synchronously update the selectedcontent
element.

This patch fixes this by moving the selectedcontent updating to the
post-insertion steps for insertion, and by using a microtask to update
on removal if needed.

This matches the behavior in the spec PR: whatwg/html#12263

Bug: 458113204
Change-Id: I42bb94c6eace93445cfbc816529e42ca8a561b94
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7745871
Reviewed-by: David Baron <dbaronchromium.org>
Reviewed-by: Joey Arhar <jarharchromium.org>
Commit-Queue: Joey Arhar <jarharchromium.org>
Cr-Commit-Position: refs/heads/main{#1622241}

--

wpt-commits: 8116dc1ced751ff3a62c58e020f3d7d0ad61553b
wpt-pr: 59528

UltraBlame original commit: 86fb81e4e646ca7e6da3fa87d0d72010474fb780
@josepharhar

Copy link
Copy Markdown
Contributor Author

Thanks, I am working on moving steps right now for option and selectedcontent elements, and I'll update this PR accordingly soon.

And I believe the current set of algorithms would give us the Firefox outcome on #11825. Is that intentional?

I think this problem exists before and after this PR and this PR does not intend to change this behavior. Is there something in here in particular that it looks like this PR is changing with regards to this behavior?

@josepharhar

Copy link
Copy Markdown
Contributor Author

Ok, I pushed a change to make the moving steps for options and selectedcontent elements update selectedcontent elements if appropriate after a microtask.

@josepharhar

Copy link
Copy Markdown
Contributor Author

Is there anything else I can do to get this PR merged? I'd like to ship the changes in chromium.

@annevk

annevk commented Jun 5, 2026

Copy link
Copy Markdown
Member

The last two commits I pushed have behavioral changes:

  • It seemed that the post-connection steps didn't recalculate disabled. With that commit they do.
  • I also wanted to see what it would look like if we relied on microtasks entirely as that means we don't need post-connection steps. This looks quite attractive to me and from the earlier discussion I'm not sure we feel that strongly about it being synchronous. The main use cases are about being able to style it differently which this shouldn't impact at all.

Thoughts?

@josepharhar

Copy link
Copy Markdown
Contributor Author

Thanks Anne, this looks great! I am ok with using microtasks more often, and it looks like when actually choosing an option the selectedcontent element is still updated synchronously, right?

Comment thread source
</div>

<div algorithm>
<p>When an <code>option</code> element is popped off the <span>stack of open elements</span> of an

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@annevk is this not needed anymore because of the microtasks?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I remember, we needed this because without it we would be cloning an empty option element into the selectedcontent element during parsing. Now that insertion uses a microtask to wait before doing cloning, parsing should have already attached all of the descendants of the option element before microtasks are run, so this isn't needed. Is that the right logic behind this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on my other analysis, I'm worried that the parser might yield and run microtasks in the middle of parsing an option, which might end up cloning an incomplete option into the selectedcontent element.

I'm still not an expert on how the parser might yield and run microtasks, but I feel like maybe we should keep this here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using microtasks for node tree manipulation where what gets manipulated can still change does seem too bold on reflection. I've added a commit that reverts those aspects of that commit. Apologies for the churn.

WDYT?

@josepharhar

Copy link
Copy Markdown
Contributor Author

I tried implementing the microtask changes, but it makes selectedcontent-mutations.html flaky. I think that microtasks are not being run at a consistent time, perhaps due to something with the parser yielding to run microtasks at different times? I already implemented microtasks during removal/moving steps for options and selectedcontent, but I think that selectedcontent-mutations.html wasn't affected by it because the test isn't really hitting that case.

I'm still ok with using microtasks, especially since we don't have post-removal or post-moving steps and I'd rather not implement them, but we would probably have to delete the selectedcontent-mutations.html test.

@jnjaeschke

Copy link
Copy Markdown

What kind of flakiness did you see @josepharhar ? I have a local implementation of what @annevk suggested (prior to the last change) and there was no flakiness in Gecko. I also couldn't find spec evidence that suggested there may be any (and I liked the idea of getting rid of post-connection steps...)

@annevk

annevk commented Jun 24, 2026

Copy link
Copy Markdown
Member

I think the flakiness you can get is that depending on when the parser yields and the microtask runs, the contents of the option element might have changed, or in case of selectedcontent element callbacks, the currently selected option element might have changed.

One thing we could consider doing still I think is doing the clone synchronously, but the insertion asynchronously, but that still leaves us with the problem of running the clone for the option element too early as we want to clone when we see the end tag, not when we've only seen the start tag.

@josepharhar

Copy link
Copy Markdown
Contributor Author

Thanks Anne! Let me try to summarize which things use microtasks and which things don't in the current state of this PR:

  • Update selectedcontent synchronously:
    • selected option element insertion via post-insertion steps
    • selected option end tag parsed during HTML parsing
    • selectedcontent element insertion via post-insertion steps
    • option selectedness changes via script or user interaction
  • Update selectedcontent after a microtask:
    • selected option removal
    • selected option moving
    • selectedcontent removal
    • selectedcontent moving

I think this sounds good to make and avoids inconsistencies/flakiness with parser yielding because moving/removal during parsing sounds unlikely to occur.

One thing we could consider doing still I think is doing the clone synchronously, but the insertion asynchronously, but that still leaves us with the problem of running the clone for the option element too early as we want to clone when we see the end tag, not when we've only seen the start tag.

So we would clone the option element immediately and store it in a document fragment variable that goes along with the microtask, then replace the contents of the selectedcontent element with the cloned stuff after the microtask? I don't really see the benefit of doing this since I'm worried that you might be able to find a way to run script within moving/removing steps as a result of cloning into the document fragment, and because it wouldn't solve the flakiness issues with parsing since replacement is still not always happening at the same time?

@annevk

annevk commented Jul 2, 2026

Copy link
Copy Markdown
Member

The summary seems right, except that we are not doing anything for selectedcontent removal.

As for cloning first, I think I agree that you would still get some non-determinism during parsing. I don't think there's a script execution risk though.

I'm happy with this PR as-is, although we might want to tweak some details further as we get more experience. @jnjaeschke any more thoughts?

@josepharhar

Copy link
Copy Markdown
Contributor Author

The summary seems right, except that we are not doing anything for selectedcontent removal.

Yes, my bad.

I'm happy with this PR as-is, although we might want to tweak some details further as we get more experience.

Thanks, sounds good!

lando-worker Bot pushed a commit to mozilla-firefox/firefox that referenced this pull request Jul 30, 2026
…s when option.selected changes. r=emilio

Implements the relevant part of whatwg/html#12263.

SetOptionsSelectedByIndex enforces the selectedness invariant itself instead
of going through RunSelectednessSettingAlgorithm, so it never schedules a
selectedcontent update. Force one when it actually changes the selection,
matching select.value/select.selectedIndex, and add
RunPendingSelectedContentUpdate() so the synchronous and microtask
schedulers can't both fire for the same pending update.

Differential Revision: https://phabricator.services.mozilla.com/D314003
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 30, 2026
…selected changes.

Implements the relevant part of whatwg/html#12263.

SetOptionsSelectedByIndex enforces the selectedness invariant itself instead
of going through RunSelectednessSettingAlgorithm, so it never schedules a
selectedcontent update. Force one when it actually changes the selection,
matching select.value/select.selectedIndex, and add
RunPendingSelectedContentUpdate() so the synchronous and microtask
schedulers can't both fire for the same pending update.

Differential Revision: https://phabricator.services.mozilla.com/D314003

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=2045023
gecko-commit: 84bee42fd8577194eac5d9dae361b9542b882bf8
gecko-commit-git: 2cb8dfd1631c50557347102bdfe126f4a979492d
gecko-reviewers: emilio
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic: select The <select> element

3 participants