diff --git a/Libraries/LibWeb/DOM/NodeIterator.cpp b/Libraries/LibWeb/DOM/NodeIterator.cpp index c359f2bf86e17..bc2df6129aad1 100644 --- a/Libraries/LibWeb/DOM/NodeIterator.cpp +++ b/Libraries/LibWeb/DOM/NodeIterator.cpp @@ -15,12 +15,12 @@ namespace Web::DOM { GC_DEFINE_ALLOCATOR(NodeIterator); -NodeIterator::NodeIterator(JS::Realm& realm, Node& root) +NodeIterator::NodeIterator(JS::Realm& realm, GC::Ref root, NodePointer reference) : PlatformObject(realm) , m_root(root) - , m_reference({ root }) + , m_reference(reference) { - root.document().register_node_iterator({}, *this); + m_root->document().register_node_iterator({}, *this); } NodeIterator::~NodeIterator() = default; @@ -44,17 +44,17 @@ void NodeIterator::visit_edges(Cell::Visitor& visitor) visitor.visit(m_root); visitor.visit(m_reference.node); - if (m_traversal_pointer.has_value()) - visitor.visit(m_traversal_pointer->node); + if (m_candidate_reference.has_value()) + visitor.visit(m_candidate_reference->node); } // https://dom.spec.whatwg.org/#dom-document-createnodeiterator GC::Ref NodeIterator::create(JS::Realm& realm, Node& root, unsigned what_to_show, GC::Ptr filter) { // 1. Let iterator be a new NodeIterator object. - // 2. Set iterator’s root and iterator’s reference to root. - // 3. Set iterator’s pointer before reference to true. - auto iterator = realm.create(realm, root); + // 2. Set iterator’s root to root. + // 3. Set iterator’s reference to (root, true). + auto iterator = realm.create(realm, root, NodePointer { root, true }); // 4. Set iterator’s whatToShow to whatToShow. iterator->m_what_to_show = what_to_show; @@ -76,65 +76,82 @@ void NodeIterator::detach() // https://dom.spec.whatwg.org/#concept-nodeiterator-traverse JS::ThrowCompletionOr> NodeIterator::traverse(Direction direction) { - // 1. Let node be iterator’s reference. - // 2. Let beforeNode be iterator’s pointer before reference. - m_traversal_pointer = m_reference; + // 1. Set iterator’s candidate reference to iterator’s reference. + m_candidate_reference = m_reference; - GC::Ptr candidate; + // 2. Let result be null. + GC::Ptr result; // 3. While true: while (true) { - // 4. Branch on direction: + // 1. If type is "next": if (direction == Direction::Next) { - // next - // If beforeNode is false, then set node to the first node following node in iterator’s iterator collection. - // If there is no such node, then return null. - if (!m_traversal_pointer->is_before_node) { - auto* next_node = m_traversal_pointer->node->next_in_pre_order(m_root.ptr()); - if (!next_node) - return nullptr; - m_traversal_pointer->node = *next_node; - } else { - // If beforeNode is true, then set it to false. - m_traversal_pointer->is_before_node = false; + // 1. If iterator’s candidate reference’s pointer before is false: + if (!m_candidate_reference->pointer_before) { + // 1. Let following be the first node following candidate reference’s node in iterator’s iterator collection. + // If there is no such node, then break. + auto* following = m_candidate_reference->node->next_in_pre_order(m_root); + if (!following) + break; + + // 2. Set candidate reference to (following, false). + m_candidate_reference = { *following, false }; } - } else { - // previous - // If beforeNode is true, then set node to the first node preceding node in iterator’s iterator collection. - // If there is no such node, then return null. - if (m_traversal_pointer->is_before_node) { - if (m_traversal_pointer->node.ptr() == m_root.ptr()) - return nullptr; - auto* previous_node = m_traversal_pointer->node->previous_in_pre_order(); - if (!previous_node) - return nullptr; - m_traversal_pointer->node = *previous_node; - } else { - // If beforeNode is false, then set it to true. - m_traversal_pointer->is_before_node = true; + // 2. Otherwise, set candidate reference to (candidate reference’s node, false). + else { + m_candidate_reference = { m_candidate_reference->node, false }; } } + // 2. Otherwise: + else { + // 1. If iterator’s candidate reference’s pointer before is true: + if (m_candidate_reference->pointer_before) { + // 1. Let preceding be the first node preceding candidate reference’s node in iterator’s iterator collection. + // If there is no such node, then break. + if (m_candidate_reference->node == m_root) + break; + auto* preceding = m_candidate_reference->node->previous_in_pre_order(); + if (!preceding) + break; + + // 2. Set iterator’s candidate reference to (preceding, true). + m_candidate_reference = { *preceding, true }; + } + // 2. Otherwise, set candidate reference to (candidate reference’s node, true). + else { + m_candidate_reference = { m_candidate_reference->node, true }; + } + } + + // 3. Let node be candidate reference’s node. + GC::Ref node = m_candidate_reference->node; - // NOTE: If the NodeFilter deletes the iterator's current traversal pointer, - // we will automatically retarget it. However, in that case, we're expected - // to return the node passed to the filter, not the adjusted traversal pointer's - // node after the filter returns! - candidate = m_traversal_pointer->node; + // 4. Let filterResult be the result of filtering node within iterator. If this throws an exception, + // then set iterator’s candidate reference to null and rethrow that exception. + auto filter_result = filter(node); + if (filter_result.is_error()) { + m_candidate_reference.clear(); + return filter_result.release_error(); + } + + // 5. If filterResult is FILTER_ACCEPT: + if (filter_result.value() == NodeFilter::Result::FILTER_ACCEPT) { + // 1. Set iterator’s reference to iterator’s candidate reference. + m_reference = *m_candidate_reference; - // 2. Let result be the result of filtering node within iterator. - auto result = TRY(filter(*m_traversal_pointer->node)); + // 2. Set result to node. + result = node; - // 3. If result is FILTER_ACCEPT, then break. - if (result == NodeFilter::Result::FILTER_ACCEPT) + // 3. Break. break; + } } - // 4. Set iterator’s reference to node. - // 5. Set iterator’s pointer before reference to beforeNode. - m_reference = m_traversal_pointer.release_value(); + // 4. Set iterator’s candidate reference to null. + m_candidate_reference.clear(); - // 6. Return node. - return candidate; + // 5. Return result. + return result; } // https://dom.spec.whatwg.org/#concept-traversal-filter @@ -183,76 +200,60 @@ JS::ThrowCompletionOr NodeIterator::filter(Node& node) // https://dom.spec.whatwg.org/#dom-nodeiterator-nextnode JS::ThrowCompletionOr> NodeIterator::next_node() { + // The nextNode() method steps are to return the result of traversing with this and "next". return traverse(Direction::Next); } // https://dom.spec.whatwg.org/#dom-nodeiterator-previousnode JS::ThrowCompletionOr> NodeIterator::previous_node() { + // The previousNode() method steps are to return the result of traversing with this and "previous". return traverse(Direction::Previous); } -void NodeIterator::run_pre_removing_steps_with_node_pointer(Node& to_be_removed_node, NodePointer& pointer) +// https://dom.spec.whatwg.org/#nodeiterator-adjust +NodeIterator::NodePointer NodeIterator::adjust_node_pointer(NodePointer node_pointer, Node& to_be_removed_node) { - // NOTE: This function tries to match the behavior of other engines, but not the DOM specification - // as it's a known issue that the spec doesn't match how major browsers behave. - // Spec bug: https://github.com/whatwg/dom/issues/907 - - if (!to_be_removed_node.is_descendant_of(root())) - return; - - if (!to_be_removed_node.is_inclusive_ancestor_of(pointer.node)) - return; - - if (pointer.is_before_node) { - if (auto* node = to_be_removed_node.next_in_pre_order(root())) { - while (node && node->is_descendant_of(to_be_removed_node)) - node = node->next_in_pre_order(root()); - if (node) - pointer.node = *node; - return; - } - if (auto* node = to_be_removed_node.previous_in_pre_order()) { - if (to_be_removed_node.is_ancestor_of(pointer.node)) { - while (node && node->is_descendant_of(to_be_removed_node)) - node = node->previous_in_pre_order(); - } - if (node) { - pointer = { - .node = *node, - .is_before_node = false, - }; - } - } - return; + // 1. If toBeRemovedNode is not an inclusive ancestor of nodePointer’s node, or toBeRemovedNode is an inclusive + // ancestor of nodeIterator’s root, then return nodePointer. + if (!to_be_removed_node.is_inclusive_ancestor_of(node_pointer.node) || to_be_removed_node.is_inclusive_ancestor_of(root())) + return node_pointer; + + // 2. If nodePointer’s pointer before is true: + if (node_pointer.pointer_before) { + // 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, if there is such a node; otherwise null. + auto* next = to_be_removed_node.next_in_pre_order(root()); + while (next && next->is_descendant_of(to_be_removed_node)) + next = next->next_in_pre_order(root()); + + // 2. If next is non-null, then return (next, true). + if (next) + return { *next, true }; } - if (auto* node = to_be_removed_node.previous_in_pre_order()) { - if (to_be_removed_node.is_ancestor_of(pointer.node)) { - while (node && node->is_descendant_of(to_be_removed_node)) - node = node->previous_in_pre_order(); - } - if (node) - pointer.node = *node; - return; - } - auto* node = to_be_removed_node.next_in_pre_order(root()); - if (to_be_removed_node.is_ancestor_of(pointer.node)) { - while (node && node->is_descendant_of(to_be_removed_node)) - node = node->previous_in_pre_order(); - } - if (node) - pointer.node = *node; + // 3. Let newNode be toBeRemovedNode’s parent, if toBeRemovedNode’s previous sibling is null; otherwise the + // inclusive descendant of toBeRemovedNode’s previous sibling that appears last in tree order. + // NOTE: This is exactly toBeRemovedNode’s previous node in pre-order. It is never null here, because step 1 + // guarantees toBeRemovedNode is a proper descendant of the root and thus has a parent. + auto* new_node = to_be_removed_node.previous_in_pre_order(); + VERIFY(new_node); + + // 4. Return (newNode, false). + return { *new_node, false }; } // https://dom.spec.whatwg.org/#nodeiterator-pre-removing-steps void NodeIterator::run_pre_removing_steps(Node& to_be_removed_node) { - // NOTE: If we're in the middle of traversal, we have to adjust the traversal pointer in response to node removal. - if (m_traversal_pointer.has_value()) - run_pre_removing_steps_with_node_pointer(to_be_removed_node, *m_traversal_pointer); - - run_pre_removing_steps_with_node_pointer(to_be_removed_node, m_reference); + // 1. Set nodeIterator’s reference to the result of adjusting a node pointer given nodeIterator’s reference, + // nodeIterator, and toBeRemovedNode. + m_reference = adjust_node_pointer(m_reference, to_be_removed_node); + + // 2. If nodeIterator’s candidate reference is non-null, then set nodeIterator’s candidate reference to the result + // of adjusting a node pointer given nodeIterator’s candidate reference, nodeIterator, and toBeRemovedNode. + if (m_candidate_reference.has_value()) + m_candidate_reference = adjust_node_pointer(*m_candidate_reference, to_be_removed_node); } } diff --git a/Libraries/LibWeb/DOM/NodeIterator.h b/Libraries/LibWeb/DOM/NodeIterator.h index 41df114175217..5fe22f2dc4af9 100644 --- a/Libraries/LibWeb/DOM/NodeIterator.h +++ b/Libraries/LibWeb/DOM/NodeIterator.h @@ -25,7 +25,7 @@ class NodeIterator final : public Bindings::PlatformObject { GC::Ref root() { return m_root; } GC::Ref reference_node() { return m_reference.node; } - bool pointer_before_reference_node() const { return m_reference.is_before_node; } + bool pointer_before_reference_node() const { return m_reference.pointer_before; } unsigned what_to_show() const { return m_what_to_show; } GC::Ptr filter() const; @@ -38,7 +38,14 @@ class NodeIterator final : public Bindings::PlatformObject { void run_pre_removing_steps(Node&); private: - explicit NodeIterator(JS::Realm&, Node& root); + // https://dom.spec.whatwg.org/#node-pointer + // A node pointer is a tuple consisting of a node (a node) and a pointer before (a boolean). + struct NodePointer { + GC::Ref node; + bool pointer_before { true }; + }; + + explicit NodeIterator(JS::Realm&, GC::Ref root, NodePointer); virtual void initialize(JS::Realm&) override; virtual void visit_edges(Cell::Visitor&) override; @@ -56,21 +63,13 @@ class NodeIterator final : public Bindings::PlatformObject { // https://dom.spec.whatwg.org/#concept-traversal-root GC::Ref m_root; - struct NodePointer { - GC::Ref node; - - // https://dom.spec.whatwg.org/#nodeiterator-pointer-before-reference - bool is_before_node { true }; - }; - - void run_pre_removing_steps_with_node_pointer(Node&, NodePointer&); + NodePointer adjust_node_pointer(NodePointer, Node& to_be_removed_node); // https://dom.spec.whatwg.org/#nodeiterator-reference NodePointer m_reference; - // While traversal is ongoing, we keep track of the current node pointer. - // This allows us to adjust it during traversal if calling the filter ends up removing the node from the DOM. - Optional m_traversal_pointer; + // https://dom.spec.whatwg.org/#nodeiterator-candidate-reference + Optional m_candidate_reference; // https://dom.spec.whatwg.org/#concept-traversal-whattoshow unsigned m_what_to_show { 0 }; diff --git a/Tests/LibWeb/Text/expected/wpt-import/dom/traversal/NodeIterator-removal-during-filtering.txt b/Tests/LibWeb/Text/expected/wpt-import/dom/traversal/NodeIterator-removal-during-filtering.txt new file mode 100644 index 0000000000000..a796b5bf2ab5d --- /dev/null +++ b/Tests/LibWeb/Text/expected/wpt-import/dom/traversal/NodeIterator-removal-during-filtering.txt @@ -0,0 +1,9 @@ +Harness status: OK + +Found 4 tests + +4 Pass +Pass Removing the accepted node during its own filtering retargets the reference and returns the filtered node +Pass Removing an already-visited subtree during filtering does not disrupt forward traversal +Pass referenceNode during filtering reflects the last accepted node, not the candidate +Pass Removing an ancestor of the in-flight position during filtering, pointer before it, with no following node within root \ No newline at end of file diff --git a/Tests/LibWeb/Text/expected/wpt-import/dom/traversal/NodeIterator-removal.txt b/Tests/LibWeb/Text/expected/wpt-import/dom/traversal/NodeIterator-removal.txt new file mode 100644 index 0000000000000..44a80554bdb2e --- /dev/null +++ b/Tests/LibWeb/Text/expected/wpt-import/dom/traversal/NodeIterator-removal.txt @@ -0,0 +1,30 @@ +Harness status: OK + +Found 25 tests + +25 Pass +Pass Test removing node paras[0] +Pass Test removing node paras[0].firstChild +Pass Test removing node paras[1].firstChild +Pass Test removing node paras[5].firstChild +Pass Test removing node foreignPara1 +Pass Test removing node foreignPara1.firstChild +Pass Test removing node detachedPara1 +Pass Test removing node detachedPara1.firstChild +Pass Test removing node foreignPara2 +Pass Test removing node xmlElement +Pass Test removing node foreignTextNode +Pass Test removing node processingInstruction +Pass Test removing node comment +Pass Test removing node doctype +Pass Test removing node foreignDoctype +Pass Test removing node paras[1] +Pass Test removing node detachedPara2 +Pass Test removing node detachedPara2.firstChild +Pass Test removing node testDiv +Pass Test removing node xmlTextNode +Pass Test removing node xmlComment +Pass Test removing node foreignComment +Pass Test removing node xmlDoctype +Pass Removing an inclusive ancestor of reference, pointer before reference is true, with a following node within root +Pass Removing an inclusive ancestor of reference, pointer before reference is true, with no following node within root \ No newline at end of file diff --git a/Tests/LibWeb/Text/input/wpt-import/dom/traversal/NodeIterator-removal-during-filtering.html b/Tests/LibWeb/Text/input/wpt-import/dom/traversal/NodeIterator-removal-during-filtering.html new file mode 100644 index 0000000000000..7d1c67ab934cb --- /dev/null +++ b/Tests/LibWeb/Text/input/wpt-import/dom/traversal/NodeIterator-removal-during-filtering.html @@ -0,0 +1,156 @@ + +NodeIterator: removing nodes during filtering + +
+ + + diff --git a/Tests/LibWeb/Text/input/wpt-import/dom/traversal/NodeIterator-removal.html b/Tests/LibWeb/Text/input/wpt-import/dom/traversal/NodeIterator-removal.html new file mode 100644 index 0000000000000..8002006a6a94a --- /dev/null +++ b/Tests/LibWeb/Text/input/wpt-import/dom/traversal/NodeIterator-removal.html @@ -0,0 +1,182 @@ + +NodeIterator removal tests + + +
+ + + +