LibWeb: Implement 'State-preserving atomic move integration' #3855
+1,581
−40
Conversation
This file contains 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
This is needed to fix timeouts for the slotchange-events.html WPT test for 'moveBefore'.
It turns out that this optimization does not hold, and results in the algorithm not running in "Moving a slot runs the assign slottables algorithm" WPT subtest of `slotchange-events.html`, resulting in a timeout. We will need to find another method of making this optimization
This was a refactoring made in the DOM spec as part of the introduction of the move before API.
shannonbooth
commented
Mar 8, 2025
| WebIDL::ExceptionOr<void> Node::move_node(Node& new_parent, Node* child) | ||
| { | ||
| // 1. If newParent’s shadow-including root is not the same as node’s shadow-including root, then throw a "HierarchyRequestError" DOMException. | ||
| if (&new_parent.shadow_including_root() != &shadow_including_root()) |
There was a problem hiding this comment.
This is what should be catching the cross document case in the spec as I understand it, but isn't currently
shannonbooth
commented
Mar 8, 2025
| // AD-HOC: This method iterates over the root's entire subtree. That iteration does nothing if the root is not a | ||
| // shadow root (see `find_slottables`). This iteration can be very expensive as the HTML parser inserts | ||
| // nodes, especially on sites with many elements. So we skip it if we know it's going to be a no-op anyways. | ||
| if (!root->is_shadow_root()) |
There was a problem hiding this comment.
I'm trying to figure out if it's only the "relevant agent’s signal slots" event firing that cares about this, in which case, I wonder if we can come up to detect a different metric which can figure out if we need to fire any slotchange events so we can keep this fast path 🤔
This was recently added to both the HTML and DOM specifications, introducing the new moveBefore DOM API, as well as the new internal 'removing steps'. See: * whatwg/html@432e8fb * whatwg/dom@eaf2ac7
github-actions
bot
added
the
conflicts
|
Your pull request has conflicts that need to be resolved before it can be reviewed and merged. Make sure to rebase your branch on top of the latest |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This was recently added to both the HTML and DOM specifications,
introducing the new moveBefore DOM API, as well as the new internal
'removing steps'.
See:
Most of the diff here is from WPT imports. We have some tests still failing in dom/nodes/moveBefore, but as far as I can tell, that is mostly due to functionality in other layers of our implementation being missing.
I'm also a little sad about removing the optimization for assigning slots for a tree. I am trying to think of a better way of fixing that, open for ideas.
The main failing test that looks to be a potential problem that I can spot is: "Fail moveBefore() on a cross-document target node throws a HierarchyRequestError"