-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Comments
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. |
+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 The downside is that approach does introduce more complexity, both conceptually and implementation wise. But that might be safer from a web compat perspective. |
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. |
I think cloning would also need to happen if the select element APIs, ex. setting |
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
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. |
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. |
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? |
Yes.
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. |
Thanks! I will draft something to create an alternate set of MutationObserver infrastructure in the DOM spec.
I see, this sounds tricker than going with microtasks. I think we should go with microtasks instead of CEReactions for the following reasons:
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. |
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... |
Yes, that sounds correct |
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
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
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}
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}
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}
…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
…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
…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
@smaug---- do you have any additional feedback? I'd like to get started on this, and I'd still prefer to use microtask timing. |
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. |
Using microtasks for this does feel odd. One couldn't check the value of the select after modifying the selected option. 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 |
Not cloning when the element is disconnected sounds like a good idea to me.
If you were to serialize the contents of the
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. |
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...? |
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 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. |
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 Still pondering issues around microtasks: |
Based on the WHATNOT discussion, I'm going to pursue synchronous cloning |
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#10242When 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:
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----
The text was updated successfully, but these errors were encountered: