From 9c6999ecf2f92e6876307097889f40e23e3cbfe2 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Wed, 9 Mar 2022 16:38:44 +0100 Subject: [PATCH] LibWeb: Implement "NodeIterator pre-removing steps" These steps run when a node is about to be removed from its parent, and adjust the position of any live NodeIterators so that they don't point at a now-removed node. Note that while this commit implements what's in the DOM specification, the specification doesn't fully match what other browsers do. Spec bug: https://github.com/whatwg/dom/issues/907 --- Userland/Libraries/LibWeb/DOM/Document.cpp | 23 ++++++++++ Userland/Libraries/LibWeb/DOM/Document.h | 15 +++++- Userland/Libraries/LibWeb/DOM/Node.cpp | 5 +- .../Libraries/LibWeb/DOM/NodeIterator.cpp | 46 +++++++++++++++++++ Userland/Libraries/LibWeb/DOM/NodeIterator.h | 4 ++ 5 files changed, 91 insertions(+), 2 deletions(-) diff --git a/Userland/Libraries/LibWeb/DOM/Document.cpp b/Userland/Libraries/LibWeb/DOM/Document.cpp index 1a123e0808..c3da23977e 100644 --- a/Userland/Libraries/LibWeb/DOM/Document.cpp +++ b/Userland/Libraries/LibWeb/DOM/Document.cpp @@ -1037,6 +1037,17 @@ void Document::adopt_node(Node& node) inclusive_descendant.adopted_from(old_document); return IterationDecision::Continue; }); + + // Transfer NodeIterators rooted at `node` from old_document to this document. + Vector node_iterators_to_transfer; + for (auto* node_iterator : old_document.m_node_iterators) { + if (node_iterator->root() == &node) + node_iterators_to_transfer.append(*node_iterator); + } + for (auto& node_iterator : node_iterators_to_transfer) { + old_document.m_node_iterators.remove(&node_iterator); + m_node_iterators.set(&node_iterator); + } } } @@ -1474,4 +1485,16 @@ NonnullRefPtr Document::create_tree_walker(Node& root, unsigned what return TreeWalker::create(root, what_to_show, move(filter)); } +void Document::register_node_iterator(Badge, NodeIterator& node_iterator) +{ + auto result = m_node_iterators.set(&node_iterator); + VERIFY(result == AK::HashSetResult::InsertedNewEntry); +} + +void Document::unregister_node_iterator(Badge, NodeIterator& node_iterator) +{ + bool was_removed = m_node_iterators.remove(&node_iterator); + VERIFY(was_removed); +} + } diff --git a/Userland/Libraries/LibWeb/DOM/Document.h b/Userland/Libraries/LibWeb/DOM/Document.h index 389c1f622c..9dbbe9e9ff 100644 --- a/Userland/Libraries/LibWeb/DOM/Document.h +++ b/Userland/Libraries/LibWeb/DOM/Document.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018-2021, Andreas Kling + * Copyright (c) 2018-2022, Andreas Kling * * SPDX-License-Identifier: BSD-2-Clause */ @@ -327,6 +327,16 @@ public: NonnullRefPtr create_node_iterator(Node& root, unsigned what_to_show, RefPtr); NonnullRefPtr create_tree_walker(Node& root, unsigned what_to_show, RefPtr); + void register_node_iterator(Badge, NodeIterator&); + void unregister_node_iterator(Badge, NodeIterator&); + + template + void for_each_node_iterator(Callback callback) + { + for (auto* node_iterator : m_node_iterators) + callback(*node_iterator); + } + private: explicit Document(const AK::URL&); @@ -430,5 +440,8 @@ private: Vector> m_media_query_lists; bool m_needs_layout { false }; + + HashTable m_node_iterators; }; + } diff --git a/Userland/Libraries/LibWeb/DOM/Node.cpp b/Userland/Libraries/LibWeb/DOM/Node.cpp index 1342f002ef..c6fb28cd65 100644 --- a/Userland/Libraries/LibWeb/DOM/Node.cpp +++ b/Userland/Libraries/LibWeb/DOM/Node.cpp @@ -387,7 +387,10 @@ void Node::remove(bool suppress_observers) // FIXME: For each live range whose start node is parent and start offset is greater than index, decrease its start offset by 1. // FIXME: For each live range whose end node is parent and end offset is greater than index, decrease its end offset by 1. - // FIXME: For each NodeIterator object iterator whose root’s node document is node’s node document, run the NodeIterator pre-removing steps given node and iterator. + // For each NodeIterator object iterator whose root’s node document is node’s node document, run the NodeIterator pre-removing steps given node and iterator. + document().for_each_node_iterator([&](NodeIterator& node_iterator) { + node_iterator.run_pre_removing_steps(*this); + }); // FIXME: Let oldPreviousSibling be node’s previous sibling. (Currently unused so not included) // FIXME: Let oldNextSibling be node’s next sibling. (Currently unused so not included) diff --git a/Userland/Libraries/LibWeb/DOM/NodeIterator.cpp b/Userland/Libraries/LibWeb/DOM/NodeIterator.cpp index 0abba71695..310cf60431 100644 --- a/Userland/Libraries/LibWeb/DOM/NodeIterator.cpp +++ b/Userland/Libraries/LibWeb/DOM/NodeIterator.cpp @@ -17,6 +17,12 @@ NodeIterator::NodeIterator(Node& root) : m_root(root) , m_reference(root) { + root.document().register_node_iterator({}, *this); +} + +NodeIterator::~NodeIterator() +{ + m_root->document().unregister_node_iterator({}, *this); } // https://dom.spec.whatwg.org/#dom-document-createnodeiterator @@ -160,4 +166,44 @@ JS::ThrowCompletionOr> NodeIterator::previous_node() return traverse(Direction::Previous); } +// https://dom.spec.whatwg.org/#nodeiterator-pre-removing-steps +void NodeIterator::run_pre_removing_steps(Node& to_be_removed_node) +{ + // NOTE: This function implements what the DOM specification tells us to do. + // However, it's known to not match other browsers: https://github.com/whatwg/dom/issues/907 + + // 1. If toBeRemovedNode is not an inclusive ancestor of nodeIterator’s reference, or toBeRemovedNode is nodeIterator’s root, then return. + if (!to_be_removed_node.is_inclusive_ancestor_of(m_reference) || &to_be_removed_node == m_root) + return; + + // 2. If nodeIterator’s pointer before reference is true, then: + if (m_pointer_before_reference) { + // 1. Let next be toBeRemovedNode’s first following node that is an inclusive descendant of nodeIterator’s root and is not an inclusive descendant of toBeRemovedNode, and null if there is no such node. + RefPtr next = to_be_removed_node.next_in_pre_order(m_root); + while (next && (!next->is_inclusive_descendant_of(m_root) || next->is_inclusive_descendant_of(to_be_removed_node))) + next = next->next_in_pre_order(m_root); + + // 2. If next is non-null, then set nodeIterator’s reference to next and return. + if (next) { + m_reference = *next; + return; + } + + // 3. Otherwise, set nodeIterator’s pointer before reference to false. + m_pointer_before_reference = false; + } + + // 3. Set nodeIterator’s reference to toBeRemovedNode’s parent, if toBeRemovedNode’s previous sibling is null, + if (!to_be_removed_node.previous_sibling()) { + VERIFY(to_be_removed_node.parent()); + m_reference = *to_be_removed_node.parent(); + } else { + // ...and to the inclusive descendant of toBeRemovedNode’s previous sibling that appears last in tree order otherwise. + auto* node = to_be_removed_node.previous_sibling(); + while (node->last_child()) + node = node->last_child(); + m_reference = *node; + } +} + } diff --git a/Userland/Libraries/LibWeb/DOM/NodeIterator.h b/Userland/Libraries/LibWeb/DOM/NodeIterator.h index 4df146617c..8e7f55912f 100644 --- a/Userland/Libraries/LibWeb/DOM/NodeIterator.h +++ b/Userland/Libraries/LibWeb/DOM/NodeIterator.h @@ -19,6 +19,8 @@ class NodeIterator public: using WrapperType = Bindings::NodeIteratorWrapper; + virtual ~NodeIterator() override; + static NonnullRefPtr create(Node& root, unsigned what_to_show, RefPtr); NonnullRefPtr root() { return m_root; } @@ -33,6 +35,8 @@ public: void detach(); + void run_pre_removing_steps(Node&); + private: NodeIterator(Node& root);