Skip to content
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

Timing of cloning for the <selectedoption> element #10520

Open
josepharhar opened this issue Jul 25, 2024 · 65 comments
Open

Timing of cloning for the <selectedoption> element #10520

josepharhar opened this issue Jul 25, 2024 · 65 comments
Labels
topic: select The <select> element

Comments

@josepharhar
Copy link
Contributor

What is the issue with the HTML Standard?

The <selectedoption> element is discussed more generally here, and this issue is for a topic which has been split out: w3c/csswg-drafts#10242

When the author modifies content inside an <option> element, we will clone the contents of that <option> into the <selectedoption>, but the timing at which we do this cloning has not yet been decided.

The timing will be web observable. For example, if the author modifies an option and then tries to query the layout of <selectedoption>, they will have to wait until the cloning happens.

Some possible options for cloning timing are:

  1. Synchronously
  2. Microtask timing
  3. CustomElements reaction timing
  4. Something slower?

I think that we should do microtask timing because I am planning on using a MutationObserver internally to implement this, and MutationObserver already uses microtask timing: https://dom.spec.whatwg.org/#queue-a-mutation-observer-compound-microtask

If we diverge from microtask timing, we would have to make a special kind of mutationobserver that does different timing, right?

@emilio @smaug----

@domenic
Copy link
Member

domenic commented Jul 25, 2024

MutationObserver kind of uses microtask timing, but it uses a very special sort, with the compound microtasks. I wonder if the difference will be observable. I think it will, as it will set the "mutation observer microtask queued" agent-wide flag, which will impact the behavior of other web developer-created MutationObservers.

@sanketj
Copy link
Member

sanketj commented Jul 26, 2024

+1 to microtask timing to avoid over-cloning

To @domenic 's point, I wonder whether we should use a special type of observer with special microtask timing to minimize observable differences in mutation observers created by web developers. The "clone into selectedoption" microtask could be queued after the regular mutation observer microtask, so that web devs don't see differences in the notify mutation observers step.

The downside is that approach does introduce more complexity, both conceptually and implementation wise. But that might be safer from a web compat perspective.

@smaug----
Copy link

The over-cloning would happen only if one mutates the content of the selected option, no? The normal case is that user selects one option and the contents get cloned once. So CEReaction or even more synchronous cloning might not be too bad in this case.

Microtasks were designed for MutationObserver, and the reason was to improve performance in cases when one does lots of DOM mutation all over the place. That is not quite the case here.

svg:use, at least in Gecko, is re-cloned when needed when layout is flushed, but of course we don't want to expose layout flush timing.

@sanketj
Copy link
Member

sanketj commented Jul 29, 2024

The over-cloning would happen only if one mutates the content of the selected option, no?

I think cloning would also need to happen if the select element APIs, ex. setting selectedIndex, are used to change the selected option. Authors can update the selected option this way several times before rendering happens.

@josepharhar
Copy link
Contributor Author

MutationObserver kind of uses microtask timing, but it uses a very special sort, with the compound microtasks. I wonder if the difference will be observable. I think it will, as it will set the "mutation observer microtask queued" agent-wide flag, which will impact the behavior of other web developer-created MutationObservers.

Chromium has already been using a regular MutationObserver to observe changes to child content in option elements, so I don't see how any behavior could be changed by re-purposing this MutationObserver to be used for <selectedoption>: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/html_option_element.cc;l=58-87;drc=e61a3c997dd7d76fdc7fe44b4f20d210bdbd700e

So CEReaction or even more synchronous cloning might not be too bad in this case.

The prototype uses a synchronous mutation observer right now in chromium, and I think that the performance is quite bad because every dom mutation to any element in the whole tree requires us to start walking the dom tree to figure out if we need to update an option/selectedoption: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/html_selected_option_element.cc;l=27-32;drc=11e2d7ffa05a985a084057676f9c76c6e88018fa

Using CEReaction sounds much better than synchronous, but I'm still inclined to just reuse MutationObserver the way it is.

@domenic
Copy link
Member

domenic commented Jul 31, 2024

Chromium has already been using a regular MutationObserver to observe changes to child content in option elements, so I don't see how any behavior could be changed by re-purposing this MutationObserver to be used for <selectedoption>:

The fact that Chrome is violating the spec currently by causing web developer mutation observers to fire at times the spec says are not allowed doesn't seem like a good reason to introduce more such violations.

@josepharhar
Copy link
Contributor Author

If we created an alternate set of MutationObserver infrastructure for user-agent MutationObservers, would that resolve this issue?

I'm trying to understand what it would mean for us to do CEReactions timing instead of microtask timing as well. Does it mean that all of the DOM algorithms that append nodes, remove nodes, or modify attributes would have a new step at the end of it to run user-agent MutationObservers?

@domenic
Copy link
Member

domenic commented Jul 31, 2024

If we created an alternate set of MutationObserver infrastructure for user-agent MutationObservers, would that resolve this issue?

Yes.

I'm trying to understand what it would mean for us to do CEReactions timing instead of microtask timing as well. Does it mean that all of the DOM algorithms that append nodes, remove nodes, or modify attributes would have a new step at the end of it to run user-agent MutationObservers?

In terms of observable effects, yes.

In terms of how you would implement it, I think you'd add a step wherever https://html.spec.whatwg.org/#invoke-custom-element-reactions is called. (Click on the definition to see all call sites.)

I'm not sure how much else of the CE reactions infrastructure we'd want to import... you'd probably need some sort of microtask timing backup for cases like contenteditable, similar to the backup element queue.

@josepharhar
Copy link
Contributor Author

Thanks! I will draft something to create an alternate set of MutationObserver infrastructure in the DOM spec.

In terms of how you would implement it, I think you'd add a step wherever https://html.spec.whatwg.org/#invoke-custom-element-reactions is called. (Click on the definition to see all call sites.)

I'm not sure how much else of the CE reactions infrastructure we'd want to import... you'd probably need some sort of microtask timing backup for cases like contenteditable, similar to the backup element queue.

I see, this sounds tricker than going with microtasks.

I think we should go with microtasks instead of CEReactions for the following reasons:

  • MutationObservers already use microtasks, so trying to create an alternate type of CEReactions MutationObserver would be harder to spec and harder to implement.
  • Performance will be better when imperatively building or modifying option elements due to fewer calls to clone all of the options contents into selectedoption elements.
  • As @dandclark said in the call, it will be easier to understand how this works because it matches the author defined API of MutationObserver. I think this also increases the likelihood that this element is polyfillable.

Microtasks were designed for MutationObserver, and the reason was to improve performance in cases when one does lots of DOM mutation all over the place. That is not quite the case here.

This might be true, perhaps authors won't be doing lots of imperative modifications to an option element subtree - although we are enabling all sorts of new use cases with the customizable select proposal. Likewise, perhaps authors won't be needing to synchronously query the state of the updated DOM content in the selectedoption element, and if they need to, they can easily queue a microtask or rAF to do so.

@emilio
Copy link
Contributor

emilio commented Aug 5, 2024

So to be clear the proposal is something like "any mutation to the selected option subtree queues a micro-task to re-clone the subtree, unless one is already queued", presumably?

That seems fair, if we're fine with all the relevant weirdness that it causes. It might indeed be the less-bad option...

@josepharhar
Copy link
Contributor Author

So to be clear the proposal is something like "any mutation to the selected option subtree queues a micro-task to re-clone the subtree, unless one is already queued", presumably?

Yes, that sounds correct

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 5, 2024
This patch improves the performance of <selectedoption> by replacing the
SynchronousMutationObserver with the existing async MutationObserver in
HTMLOptionElement.

The change from sync to async impacts some tests. The timing is being
discussed in a standards issue here:
whatwg/html#10520

Fixed: 336844298
Change-Id: I9693de9cf35913e7daaebb364c4923dcd4a2dc39
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 5, 2024
This patch improves the performance of <selectedoption> by replacing the
SynchronousMutationObserver with the existing async MutationObserver in
HTMLOptionElement.

The change from sync to async impacts some tests. The timing is being
discussed in a standards issue here:
whatwg/html#10520

Fixed: 336844298
Change-Id: I9693de9cf35913e7daaebb364c4923dcd4a2dc39
aarongable pushed a commit to chromium/chromium that referenced this issue Aug 5, 2024
This patch improves the performance of <selectedoption> by replacing the
SynchronousMutationObserver with the existing async MutationObserver in
HTMLOptionElement.

The change from sync to async impacts some tests. The timing is being
discussed in a standards issue here:
whatwg/html#10520

Fixed: 336844298
Change-Id: I9693de9cf35913e7daaebb364c4923dcd4a2dc39
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5758741
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1337462}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 5, 2024
This patch improves the performance of <selectedoption> by replacing the
SynchronousMutationObserver with the existing async MutationObserver in
HTMLOptionElement.

The change from sync to async impacts some tests. The timing is being
discussed in a standards issue here:
whatwg/html#10520

Fixed: 336844298
Change-Id: I9693de9cf35913e7daaebb364c4923dcd4a2dc39
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5758741
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1337462}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 5, 2024
This patch improves the performance of <selectedoption> by replacing the
SynchronousMutationObserver with the existing async MutationObserver in
HTMLOptionElement.

The change from sync to async impacts some tests. The timing is being
discussed in a standards issue here:
whatwg/html#10520

Fixed: 336844298
Change-Id: I9693de9cf35913e7daaebb364c4923dcd4a2dc39
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5758741
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1337462}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Aug 8, 2024
…testonly

Automatic update from web-platform-tests
Improve <selectedoption> performance

This patch improves the performance of <selectedoption> by replacing the
SynchronousMutationObserver with the existing async MutationObserver in
HTMLOptionElement.

The change from sync to async impacts some tests. The timing is being
discussed in a standards issue here:
whatwg/html#10520

Fixed: 336844298
Change-Id: I9693de9cf35913e7daaebb364c4923dcd4a2dc39
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5758741
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1337462}

--

wpt-commits: d32c223215aaa166fce5162b4f76d323e9d513e7
wpt-pr: 47467
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this issue Aug 12, 2024
…testonly

Automatic update from web-platform-tests
Improve <selectedoption> performance

This patch improves the performance of <selectedoption> by replacing the
SynchronousMutationObserver with the existing async MutationObserver in
HTMLOptionElement.

The change from sync to async impacts some tests. The timing is being
discussed in a standards issue here:
whatwg/html#10520

Fixed: 336844298
Change-Id: I9693de9cf35913e7daaebb364c4923dcd4a2dc39
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5758741
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1337462}

--

wpt-commits: d32c223215aaa166fce5162b4f76d323e9d513e7
wpt-pr: 47467
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this issue Aug 19, 2024
…testonly

Automatic update from web-platform-tests
Improve <selectedoption> performance

This patch improves the performance of <selectedoption> by replacing the
SynchronousMutationObserver with the existing async MutationObserver in
HTMLOptionElement.

The change from sync to async impacts some tests. The timing is being
discussed in a standards issue here:
whatwg/html#10520

Fixed: 336844298
Change-Id: I9693de9cf35913e7daaebb364c4923dcd4a2dc39
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5758741
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1337462}

--

wpt-commits: d32c223215aaa166fce5162b4f76d323e9d513e7
wpt-pr: 47467
@josepharhar
Copy link
Contributor Author

@smaug---- do you have any additional feedback? I'd like to get started on this, and I'd still prefer to use microtask timing.

@annevk
Copy link
Member

annevk commented Aug 29, 2024

Are we also guarding the cloning on the element being connected? I could see synchronously working as well. Either way there will be some weirdness as this is novel behavior.

@smaug----
Copy link

Using microtasks for this does feel odd. One couldn't check the value of the select after modifying the selected option.
And innerHTML and what not would give "wrong" serialization. Feels rather error prone.

And I don't understand "the performance is quite bad because every dom mutation to any element in the whole tree requires us to start walking the dom tree to figure out if we need to update an option/selectedoption". That sounds like some limitation in Blink. In Gecko we'd just observe changes at <option> level.

@josepharhar
Copy link
Contributor Author

Are we also guarding the cloning on the element being connected?

Not cloning when the element is disconnected sounds like a good idea to me.

One couldn't check the value of the select after modifying the selected option.

select.value would still reflect the newly selected option synchronously.

And innerHTML and what not would give "wrong" serialization

If you were to serialize the contents of the <selectedoption> element itself, then yeah you'd have to wait a microtask. This doesn't seem like a typical use case to me.

And I don't understand "the performance is quite bad because every dom mutation to any element in the whole tree requires us to start walking the dom tree to figure out if we need to update an option/selectedoption". That sounds like some limitation in Blink.

This was an issue with the initial implementation which did things synchronously. I'll try studying what the web-exposed MutationObserver does to see if there are any optimizations which could be rebuilt into a synchronous mutation observer.

I do prefer doing it synchronously over CEReactions timing because figuring out how to make a new type of MutationObserver which operates at that timing sounds difficult to implement and very difficult to spec, but ideally I'd still use a normal MutationObserver with microtask timing.

@josepharhar josepharhar added the agenda+ To be discussed at a triage meeting label Sep 3, 2024
@josepharhar
Copy link
Contributor Author

I've thought some more about this, and I think I understand how we could leverage CEReactions to only do one clone per script call to a DOM api which performs a mutation.

I could create a special kind of MutationObserver which instead of queueing a microtask looks to see if there is a CE reactions stack present, and tells that CE reactions stack to "notify" this MutationObserver when it is popped. If there is no CE reactions stack present, then just clone synchronously.

I wonder if this will be really slow when parsing though when the parser does one mutation at a time. Presumbaly there is no CE reactions stack present when we are just doing the initial parsing of the HTML for the document.

I also wonder if using CEReactions like this is just an internal optimization to run clones less often and is functionally the same as just synchronously cloning every time, in which case we could make the spec a lot simpler and keep it in the DOM spec. Maybe doing anything with MutationObservers is also just an optimization, and we could just add steps to the insertion/removal/attributechange steps in the HTML spec to do the cloning when appropriate...?

@josepharhar
Copy link
Contributor Author

To add some more context to my last comment, here are some notes about how I'd want to implement this in blink:

We have a "CEReactionsScope" class which is stack allocated by generated bindings code every time a method with the [CEReactions] IDL is called from script. It can be globally accessed by anything which wants to enqueue CE reactions stuff. In its destructor, which should be run right before the IDL method returns control back to script, it invokes reactions on a CustomElementReactionStack.

In order to implement this cloning at CEReactions time, I'd want to identify DOM mutations which should trigger a cloning of the selected <option> element into <selectedoption>, and then enqueue a task to do this cloning into the CEReactionsScope destructor or into something inside the CustomElementReactionStack.

I'm not sure if the "CEReactionsScope" class has an equivalent thing that exists in the spec. I'm also not sure if we will always have a context available during HTML parsing. I can see that we create a CEReactionsScope when running CreateElement for a custom element and when the parser inserts elements.

@smaug----
Copy link

I'm not sure if CEReaction is any better (or worse) to synchronous cloning (and I think I'd implement this as synchronous cloning).

Parsing is an interesting case. No matter what timing is used, cloning might need to happen several times, once for the first <option> being parsed/created and once when possibly later <option selected> is parsed/created - those might happen in different tasks.
What should happen if the network packet boundary is right in middle of an <option>? I guess elements could use a blocked-on-parser flag. Having that on <select>certainly wouldn't work since there are cases when select has thousands of options.

Still pondering issues around microtasks:
Use of microtasks for cloning might lead to some other weird cases, like scheduling first some promises, then modify the value of an option and then schedule more promises. Those first promises would still see the old content, but latter promises new. Rather surprising. And that "modify the value" applies to all the changes, removing selected options, or selecting a new one etc.

@josepharhar
Copy link
Contributor Author

The Blink implementation uses a MutationObserver for each option (which is kind of silly, it should use one per select) which means the changes are batched and doing the steps from #10520 (comment) should only clone once. I don't think this should clone synchronously.

Batch the cloning by microtask (or even rAF)

That was our original proposal (async mutation observer-like behavior), but WebKit and Gecko had some really good points about why that would be confusing to developers. We agree and that's why we changed to the new synchronous clone behavior.

For example, in a modified version of your example:

const div = document.createElement('div');
option.append(div);
select.querySelector(‘selectedoption’).firstChild.id === undefined;
div.setAttribute('id', 'whatever');
select.querySelector(‘selectedoption’).firstChild.id === ‘whatever’;

The issue they rightly pointed out is that it’s confusing if changes to <selectedoption> only happen “later”.

Or do it synchronously, but mapping the modifications (it doesn't need to be mutation records)

I see how incrementally cloning individual modifications might be faster than just cloning the entire subtree of the option element. There’s also the chance that for common/small option elements, it might be slower to maintain such a “linked clone”, as compared to just doing a full-clone at discrete points in time. I'm also worried that this would also require us to watch for modifications inside the <selectedoption> element to make sure that the author never modified it because then doing incremental stuff might not work anymore. Authors shouldn't be modifying these contents, but in the case that they do, we don't need any special handling with the current implementation. It feels like the less “magic” that happens here, the better, for developer ergonomics.

it seems really weird that modifying some inner element within the option blows away the entire contents of the selectedoption

In the case that the author is modifying the style attribute every frame inside an option element, and there isn’t a huge number of nodes inside the option, then cloning a few nodes within the option element doesn’t seem like a performance issue and is less than the cost of doing the rest of the rendering for the new style attribute. Maintaining references to the cloned nodes and handling what happens when the clones get modified in an unexpected way sounds like it could be more work, especially for common cases where cloning only happens when the user picks a new option.

@jakearchibald
Copy link
Contributor

WebKit and Gecko had some really good points about why that would be confusing to developers. We agree and that's why we changed to the new synchronous clone behavior.

For example, in a modified version of your example:

const div = document.createElement('div');
option.append(div);
select.querySelector(‘selectedoption’).firstChild.id === undefined;
div.setAttribute('id', 'whatever');
select.querySelector(‘selectedoption’).firstChild.id === ‘whatever’;

The issue they rightly pointed out is that it’s confusing if changes to <selectedoption> only happen “later”.

Right, but in the sync model, it's still very confusing. Developers are going to wonder why their custom element constructors are running eg four times per frame, due to a seemingly unrelated spinner animation in the option element. And why aren't their CSS animations working in the <selectedoption>? Oh, it's because a JS animation is causing the unrelated element to be re-cloned every frame, so the CSS animation never gets to the second frame.

I see how incrementally cloning individual modifications might be faster than just cloning the entire subtree of the option element.

This isn't just about performance, it's about reducing unexpected behaviour.

I'm also worried that this would also require us to watch for modifications inside the <selectedoption> element

It wouldn't. The rule is simple: Mutations in the <option> are mirrored within the <selectedoption>. It's one-way.

Maintaining references to the cloned nodes and handling what happens when the clones get modified in an unexpected way sounds like it could be more work, especially for common cases where cloning only happens when the user picks a new option.

Web platform features shouldn't be designed so they only make sense in what is assumed to be the "common case". This feature will live a loooooong time, and the "common case" may change. It's better to design a system that makes sense overall, rather than cut corners.

@mikeknack86

This comment was marked as spam.

@esprehn
Copy link

esprehn commented Oct 16, 2024

@josepharhar I feel pretty strongly that synchronous clone of the whole subtree is a mistake. The platform doesn't do that today, and it cannot be polyfilled without monkey patching the entire DOM surface. It also has a huge number of caveats related to performance, losing state (ex. animated images, videos or canvas), running custom element callbacks, etc. It feels like going in the wrong direction if we just removed mutation events too.

As a web developer the whole platform is based around batching too, so doing e.style.color = "red"; e.style.display = "block"; // 10 CSS properties is totally fine from a performance perspective. But this new API violates that and makes that clone the entire subtree 10 times.

The userland API shape for something like this would be for <selectedoption> to have a method on it like getContents() which does takeRecords() and does the update sync instead of waiting for the MutationObserver delivery.

@jakearchibald
Copy link
Contributor

jakearchibald commented Oct 17, 2024

If the driving force here is avoiding things that would be confusing to developers, here are the possible confusing things:

  • Not immediately script-observable: The change in <option> isn't immediately script-observable in <selectedoption>. This is the driving force behind this change according to @josepharhar.
  • One small change becomes a big 'reset': Changing eg an attribute/style on one element in <option> causes everything in <selectedoption> to 'reset' (animations, constructors) even though that wasn't the developer's intent, and isn't what happened with the related content in <option>.
  • Attack of the clones: Mutating many attributes/styles is usually assumed to be cheap, and the effects are deferred until the next microtask, but in this case every small change causes the whole tree to be cloned (and constructors run etc etc). Eg, moving a node is two whole clones of everything in the <option>, since a 'move' is a remove + insert. 10 changes to el.style, triggers 10 tree clones.
  • State missing in <selectedoption>: Eg the <canvas> in the <option> is not fully represented in the <selectedoption>, since its state is not fully represented by its tree & attributes. @justinfagnani says this will be a problem with web components.
  • Changes are one-way: Modifying things in <selectedoption> will not change content in the selected <option>.
  • Changes are unexpectedly applied: I have a fancy animation in one of my <option>s which requires JS, which I trigger on hover. Why is this also happening in my <selectedoption> which isn't being hovered??? This issue may become worse in future if the trend of elements modifying their own content & attributes continues.

And let's put those against the proposals in this thread:

  • Synchronously clone: Every DOM change within the selected <option> triggers a synchronous clone of everything within it
  • Asynchronously clone: Same, but debounced
  • Synchronously apply: Clone elements initially, but mirror modifications rather than re-cloning. So if an attribute is set on an element in the selected <option>, the attribute is set on its clone in <selectedoption>.
  • Asynchronously apply: Same, but debounced
Synchronously re-clone Asynchronously re-clone Synchronously apply Asynchronously apply
Not immediately script-observable confused confused
One small change becomes a big 'reset' confused confused
Attack of the clones confused
State missing in <selectedoption> confused confused confused confused
Changes are one-way confused confused confused confused
Changes are unexpectedly applied confused confused confused confused

I don't think the proposed change in this issue (change from "Asynchronously re-clone everything" to "Synchronously re-clone everything") reduces developer confusion, it removes one bit of confusion whilst adding another that's as bad, if not worse, and definitely harder to spot.

I don't think "this won't come up in 'common' usage" is a useful argument, as this whole change is being driven by the case where developers edit the content of the selected <option>. If we're not trying to improve that, what are we doing here?

I don't think "State missing in <selectedoption>" is solvable, without some new extremely complex feature that allows the element to exist in two places at once, whilst still being independently stylable.

"Changes are one-way" seems like an easy rule to discover/learn/teach.

The only solution I see to "Changes are unexpectedly applied" is to clone synchronously, once, on <option> select, and don't attempt to update it.

The more I think about it, the more I think that could be the best way forward. It solves the "works without JS" case. It's really easy to understand. And its limitations, which only really surface when you're using JS to modify the content of the selected <option>, can be easily overcome with a little more JS. There could be a method on <selectedoption> similar to the one @esprehn suggested, such as selectedOption.resetContent(), to perform a full synchronous re-clone of the content in the selected option. Since the developer would be calling this, they'd be fully in control of the timing & more likely to understand when it applies.

Here's where the idea of updating <selectedoption> came from. The point about hydration is interesting. But if I was going from a loading state to full options, setting the key prop on the options would ensure <selectedoption> updates.

@esprehn
Copy link

esprehn commented Oct 18, 2024

I think Jake's suggestion of cloning once synchronously when changing the selected option and then only recloning on an explicit method call on the <selectedoption> sounds very promising. Authors can layer on automatic reclones with either total reset or reconciliation at various timing using MutationObserver.

Someday if the platform adds built in reconciliation we can make that opt in with a new attribute on <selectedoption>.

@annevk
Copy link
Member

annevk commented Oct 18, 2024

Yeah. The remaining issue is when to clone during the initial setup of the element. All solutions there seem to require some kind of end tag listener in the parser, which is rather unfortunate.

@esprehn
Copy link

esprehn commented Oct 19, 2024

A hook for end tag parsing already exists in all major browsers:

https://source.chromium.org/search?q=file:renderer%2Fcore%20%22::FinishParsingChildren()%22&sq=&ss=chromium%2Fchromium%2Fsrc

https://sourcegraph.com/search?q=repo:%5Egithub%5C.com/WebKit/WebKit%24+%22::finishParsingChildren%22&patternType=keyword&sm=0

https://searchfox.org/mozilla-central/search?q=DoneAddingChildren&redirect=false

There's also specs that depend on it, ex.
https://github.com/WICG/view-transitions/blob/main/document-render-blocking.md#blocking-element-id

which says:

Each time an element's end tag is parsed, or the element's ID value changes, the browser removes the id from the render-blocking-until-parsed (old id in the changed value case).

@annevk
Copy link
Member

annevk commented Oct 19, 2024

That is a monkey patch if I've ever seen one. Geez. Anyway, I know it's possible, but that doesn't make it great. It's a complete mismatch with building up the tree using imperative APIs.

@esprehn
Copy link

esprehn commented Oct 19, 2024

I wasn't trying to point out it was possible, I was pointing out that spec'ing it increases interoperability since all the major browsers already use it across many features.

I don't think the spec avoiding that hook when everyone needs it to implement the web makes sense.

@tjenkinson
Copy link

👋 coming from the blog

The only solution I see to "Changes are unexpectedly applied" is to clone synchronously, once, on select, and don't attempt to update it.

The more I think about it, the more I think that could be the best way forward. It solves the "works without JS" case. It's really easy to understand. And its limitations, which only really surface when you're using JS to modify the content of the selected , can be easily overcome with a little more JS. There could be a method on similar to the one @esprehn suggested, such as selectedOption.resetContent(), to perform a full synchronous re-clone of the content in the selected option. Since the developer would be calling this, they'd be fully in control of the timing & more likely to understand when it applies.

IMO this feels like a nice solution. I think if I was writing JS I'd rather discover it not automatically updating and handle calling resetContent myself than have it update automatically, potentially when unnecessary.

For a none js use case it feels like it's adding a lot of complexity for something that might not be used much in the wild?

Maybe syncing could be an opt-in attribute in the future if use cases come up?

@sorvell
Copy link

sorvell commented Oct 19, 2024

Cross posting another possibility.

@bahrus
Copy link

bahrus commented Oct 19, 2024

I was just going to ask why we are assuming the content of selectedoption should match what is in the option at all?

Few dropdowns do this. For example, look at the dropdown that github provides when selecting the branch.

I think it should be possible to specify a template tag used for the selectedoption, and pass in the textContent and value from the selected option, and make this our first practical example of template instantiation.

A default template could be provided by the platform if none is provided. Has that been ruled out?

@everdimension
Copy link

Completely agree with the above.

I'm surprised that selectedoption is supposed to mirror the option element. Where did the idea come from that it was even desirable? I mean it's "kinda cool" but... that's it?

Given the complex nature of implementing this, I'd be intereseted in a better explanation for the motivation for going this route

@jakearchibald
Copy link
Contributor

Where did the idea come from that it was even desirable?

It's similar to how <select> works today.

<selectedoption> is optional, fwiw.

@jakearchibald
Copy link
Contributor

I think it should be possible to specify a template tag used for the selectedoption, and pass in the textContent and value from the selected option, and make this our first practical example of template instantiation.

If you wanted the <selectedoption> content to be totally different, and wanted the solution to work without JavaScript, each option could contain two divs, and you use CSS to show only one when it's within <option>, and the other when it's in <selectedoption>.

@bahrus
Copy link

bahrus commented Oct 20, 2024

If you wanted the content to be totally different, and wanted the solution to work without JavaScript, each option could contain two divs, and you use CSS to show only one when it's within , and the other when it's in .

That's true. Each approach has its advantages and disadvantages.

The disadvantages of the mirroring approach are:

  1. The number of DOM elements doubles, which also increases the payload size. Worst case. In some cases, a single div could probably be used with some conditional child divs, which could mitigate that.
  2. There would be more "churn" in the DOM. It seems like a sledgehammer approach. This would make it more difficult to do things like attach event listeners to specific elements within the button (work arounds would be to attach the event handlers on the button and use capture if some events don't bubble).
  3. If the developer needs to add any adjustments programmatically on the cloned option, it could throw a wrench into the mirroring process, and it would mean the developer needs to be more vigilant about making those programmatic adjustments every time the user makes a selection.

Advantages of the mirroring approach vs a templating solution

  1. The platform hasn't built a templating solution yet (sigh).
  2. If each option looks/behaves significantly different from every other option, that might be easier to achieve via the mirroring approach.

There may be more pro's and con's to each approach that I'm missing, but those seem the most transparent.

@sorvell
Copy link

sorvell commented Oct 20, 2024

The motivation for this suggestion was to support rich behavior that involved event listeners and custom elements inside the select button, selectedoption, and/or options.

However, given the proposed content model, it really looks like any kind of extra interactivity inside <select> is going to be discouraged, and in particular, custom elements are not allowed inside <select>.

If that's the case, then I think just cloning/replacing on selection is fine and no sync'ing should occur by default. I'm ambivalent about adding resetContent() because it seems like modifying the selected option should be strongly discouraged (when is it necessary?).

I do think that if there is a way to do this, it shouldn't be possible to move or modify the children of <selectedoption> via script since they exist only to be styled.

@jakearchibald
Copy link
Contributor

I'm ambivalent about adding resetContent() because it seems like modifying the selected option should be strongly discouraged (when is it necessary?).

Some examples are given here https://jakearchibald.com/2024/how-should-selectedoption-work/#but-what-if-the-selected-option-is-modified

@sorvell
Copy link

sorvell commented Oct 21, 2024

Some examples are given here https://jakearchibald.com/2024/how-should-selectedoption-work/#but-what-if-the-selected-option-is-modified

In general, it seems reasonable for devs to be responsible for updating the DOM in the <selectedoption> in these cases.

Not sync'ing would be very different from the default <select> behavior without using <selectedoption>. It's unclear what the a11y implications of this are, but right now in the Chrome prototype it looks like what's announced is based on select.selectedOptions and the content inside the triggering button is ignored. It's also unclear if the behavior is expected or just not finished.

@josepharhar
Copy link
Contributor Author

We discussed this in openui and resolved on not observing/responding to mutations within the option element: openui/open-ui#825 (comment)

I will update the PR to incorporate this.

@emilio
Copy link
Contributor

emilio commented Nov 4, 2024

We still need to account for the initial parse somehow, tho, right?

E.g. a large option with the parser yielding in the middle should probably still be cloned...

@jakearchibald
Copy link
Contributor

I thought the resolution there was that cloning wouldn't happen until the end tag was parsed?

@josepharhar
Copy link
Contributor Author

Jake is correct. I'm not currently planning on adding any behavior which specifically accounts for or hooks into the yielding behavior of the parser.

I added a step to clone the option into selectedoption (now called selectedcontent) at option end tag parsing here: #10633

The cloning will only occur if the newly appended option element is the selected option in a non-multiple select.

This behavior is implemented in chromium: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/parser/html_tree_builder.cc;l=2097-2112;drc=d9b7d6fc39b8525c84f5e97854d966f304291419

@josepharhar
Copy link
Contributor Author

The end tag parsing cloning will clone when the newly appended option is selected. Since the first option always becomes the selected until others are added, this means that the first option will always be cloned. If another option is parsed and appended which has the selected attribute, then it will cause a second clone.

@zcorpan
Copy link
Member

zcorpan commented Nov 5, 2024

I guess there are two options for the parsing case:

  • clone when the option is popped
  • clone when the select is popped

The first is better for incremental rendering, but will clone twice if there's an <option selected>.

The second will only do one clone, but there won't be any clone until the whole select element has been parsed.

Has this second option been considered and rejected, or not discussed?

@smaug----
Copy link

It has been discussed, also in this issue (#10520 (comment)) and during WHATNOT. I think I'd prefer to avoid it and let user know sooner that there is something in the select.
(I guess autocomplete might also trigger a new clone too)

@annevk annevk added the topic: select The <select> element label Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: select The <select> element
Development

No branches or pull requests