Custom element reactions v2#2429
Open
karlseguin wants to merge 5 commits into
Open
Conversation
While this PR touches a lot of files, and isn't trivial, many of the changes
are either:
1 - removing guards added in previous PRs, e.g.
#1969
#2172
#2313
#2366
2 - Adding the `.ce_reactions = true` flag to various WebAPIs
CustomElements have callbacks, e.g. connectedCallback. Also, many WebAPI calls
are implemented as a series of mutations, e.g. appendChild = remove from current
+ append to new.
These two things interact in an important way: when should callbacks execute?
Before this PR, we were invoking callbacks at each individual step. This is
(a) technically wrong and (b) breaks a lot of assumptions (the reason the above
4 PRs were needed to fix bugs).
This PR adds a `_ce_reactions` queue to the frame. And, instead of invoking
callbacks, we "enqueue" the reaction. At various boundaries, a scope is created
the DOM manipulation is done, and then we pop the scope, invoking all queued
reactions.
popAndInvoke captured the reactions queue as a slice at loop entry. If firing a reaction triggered a nested scope whose enqueue grew the underlying ArrayList, the captured slice became dangling and the next iteration read freed memory. Switch to indexed iteration so the queue pointer is re-read each step. Repros via wpt/custom-elements/ enqueue-custom-element-callback-reactions-inside-another-callback.html. The rest of the diff adds .ce_reactions = true to bridge declarations that were missing it per WebIDL [CEReactions] *and* whose Zig impl actually performs a DOM/attribute mutation (verified by reading each setter). Without the flag, the algorithm's enqueue path hits assertScopeActive and panics. Runtime-state-only setters (Input.value, Input.checked, Select.value, Option.selected, Media.muted/volume, etc.) are deliberately left untagged. Covered: - CustomElementRegistry.define, .upgrade - HTMLDocument: body, title, dir - Document: open, close - HTMLElement base: insertAdjacentHTML, dir, hidden, lang, tabIndex, title, innerText - Range: insertNode, deleteContents, extractContents, surroundContents, createContextualFragment - Selection: deleteFromDocument - HTMLOptionsCollection: add, remove - DOMStringMap (dataset): namedIndexed setter/deleter — required adding .ce_reactions to NamedIndexed.Opts in bridge.zig - ~28 HTMLxxxElement files: every settable attribute that resolves to setAttributeSafe/removeAttribute (Anchor, Button, Canvas, Data, Details, Dialog, FieldSet, Form, IFrame, Image, Input, Label, Link, LI, Media, Meta, OL, OptGroup, Option, Quote, Script, Select, Slot, Style, TableCell, Template, TextArea, Time, Track, Video)
Turns a crash into a slightly delayed (incorrect) execution order which has some basis in the spec.
Flag Range.cloneContents as CEReactions setAttributeNode triggers single reaction, rather than a remove + add.
karlseguin
force-pushed
the
custom-element-reactions-v2
branch
from
58a0ade to
0b6a979
Compare
willmafh
pushed a commit
to willmafh/browser
that referenced
this pull request
May 20, 2026
…ack) Removes an assertion that can break with custom element callbacks. lightpanda-io#2429 does not solve this issue since it isn't a result of when reactions are executed, but just that they happen. (I should note, that I'm not 100% sure the above statement is correct. It's possible that our CE reactions are (in some cases at least) too micro. Maybe some operations, like setInnerHtml should operate within a more atomic frameworks, vs an CE-reaction per internal step. But that's a pretty big change)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Re-applies #2394 with fixes which was reverted in #2428
Should not be merged until WPT tests can reliably be run against the /custom-elements/ category.