From ab3ca260460c49a3f49474990dd24bdf01e2b77f Mon Sep 17 00:00:00 2001 From: Andrey Mnatsakanov Date: Wed, 22 Jul 2026 20:12:20 +0200 Subject: [PATCH] fix(js): implement a real NodeIterator instead of aliasing TreeWalker (#467) createNodeIterator returned a TreeWalker with a flag on it. The two are different interfaces, and the alias got two things wrong. An iterator's pointer starts *before* its root, so the first nextNode() returns the root itself; a TreeWalker starts *at* its root and never returns it. So createNodeIterator(el, SHOW_ELEMENT) silently skipped exactly the element it was rooted at:
-> ["A"] (Chrome: ["DIV","A"]) It also exposed the wrong surface: no referenceNode, pointerBeforeReferenceNode or detach (older library code still calls detach and hit "not a function"), while leaking currentNode, firstChild, parentNode and the rest of TreeWalker's movers. Implemented DOM 6.2 "traverse" directly. Keeping the pointer as (referenceNode, pointerBeforeReferenceNode) rather than a single cursor is what makes a direction change re-yield the current node instead of stepping over it, and a failed traversal leaves both untouched so the iterator stays resumable either way. The backward step is a new `DomTree::prev_in_subtree`, mirroring `next_in_subtree`. It can live entirely in the DOM layer because a NodeIterator prunes nothing -- unlike the TreeWalker's backward walk (#462), where FILTER_REJECT forces the filter to be consulted at every step of the descent. FILTER_REJECT still behaves as FILTER_SKIP here, which the existing test keeps covering; its expectation gains the root node the alias used to drop. The TreeWalker's now-dead `_rejectIsSkip` branch is removed. --- crates/obscura-dom/src/tree.rs | 58 +++++++++++++++++ crates/obscura-js/js/bootstrap.js | 67 +++++++++++++++++--- crates/obscura-js/src/ops.rs | 9 +++ crates/obscura-js/src/runtime.rs | 101 +++++++++++++++++++++++++++++- 4 files changed, 224 insertions(+), 11 deletions(-) diff --git a/crates/obscura-dom/src/tree.rs b/crates/obscura-dom/src/tree.rs index e428ebf..5d960a1 100644 --- a/crates/obscura-dom/src/tree.rs +++ b/crates/obscura-dom/src/tree.rs @@ -579,6 +579,41 @@ impl DomTree { Self::climb_to_next_sibling(&inner, root, current) } + /// Returns the node before `current` in document order, without leaving the + /// subtree rooted at `root`. `root` has no predecessor within its own + /// subtree, but it is itself reachable as one — a NodeIterator can return + /// its root, unlike a TreeWalker. + /// + /// A NodeIterator applies no subtree pruning (DOM 6.2: FILTER_REJECT + /// behaves as FILTER_SKIP), so unlike the TreeWalker's backward walk the + /// whole step fits here instead of being interleaved with filter calls. + pub fn prev_in_subtree(&self, root: NodeId, current: NodeId) -> Option { + let inner = self.inner.borrow(); + if current == root { + return None; + } + let current_node = inner.nodes.get(current.index())?.as_ref()?; + + let Some(prev) = current_node.prev_sibling else { + // No previous sibling: the parent immediately precedes `current`. + return current_node.parent; + }; + + // Otherwise it is the previous sibling's deepest last descendant. + let mut node_id = prev; + for _ in 0..=inner.nodes.len() { + let node = inner.nodes.get(node_id.index())?.as_ref()?; + match node.last_child { + Some(child) => node_id = child, + None => return Some(node_id), + } + } + + // Same defense in depth as the forward walk: a malformed tree must not + // spin here. + None + } + /// Follow `current`'s next sibling, climbing ancestors until one has a next /// sibling — without stepping outside `root`. fn climb_to_next_sibling( @@ -1071,6 +1106,29 @@ mod tests { assert_eq!(tree.next_after_subtree(root, root), None); } + #[test] + fn test_prev_in_subtree_reverses_document_order() { + // root > [first > nested, second]; document order is root, first, + // nested, second, so the reverse walk must retrace it exactly. + let tree = DomTree::new(); + let root = tree.new_node(NodeData::Text { contents: "root".into() }); + let first = tree.new_node(NodeData::Text { contents: "first".into() }); + let nested = tree.new_node(NodeData::Text { contents: "nested".into() }); + let second = tree.new_node(NodeData::Text { contents: "second".into() }); + tree.append_child(tree.document(), root); + tree.append_child(root, first); + tree.append_child(first, nested); + tree.append_child(root, second); + + // Previous sibling's deepest last descendant, not the sibling itself. + assert_eq!(tree.prev_in_subtree(root, second), Some(nested)); + assert_eq!(tree.prev_in_subtree(root, nested), Some(first)); + // No previous sibling: the parent precedes it, and root is returnable. + assert_eq!(tree.prev_in_subtree(root, first), Some(root)); + // Root has no predecessor inside its own subtree. + assert_eq!(tree.prev_in_subtree(root, root), None); + } + // Builds a chain of `depth` nested
elements under the document and // returns (outermost_div, innermost_div). Depth this large overflows a // recursive tree walk and aborts the process, so it guards the iterative diff --git a/crates/obscura-js/js/bootstrap.js b/crates/obscura-js/js/bootstrap.js index 8e70cb3..2e5f2d3 100644 --- a/crates/obscura-js/js/bootstrap.js +++ b/crates/obscura-js/js/bootstrap.js @@ -2637,11 +2637,8 @@ class Document extends Node { const verdict = this._filter(node); if (verdict === 1) { this.currentNode = node; return node; } // FILTER_REJECT skips the node AND its subtree; FILTER_SKIP (and any - // other non-accept value) skips only the node. NodeIterator has no - // pruning at all, so `_rejectIsSkip` keeps it on the plain step. - const step = (verdict === 2 && !this._rejectIsSkip) - ? "next_after_subtree" - : "next_in_subtree"; + // other non-accept value) skips only the node. + const step = verdict === 2 ? "next_after_subtree" : "next_in_subtree"; node = _wrap(+_dom(step, this.root._nid, node._nid)); } return null; @@ -2724,12 +2721,62 @@ class Document extends Node { }; return walker; } + // A real NodeIterator (DOM 6.2), not a TreeWalker in disguise (issue #467). + // The two differ in more than naming: an iterator's pointer starts *before* + // its root, so the first nextNode() returns the root itself, and it exposes + // referenceNode/pointerBeforeReferenceNode/detach rather than a TreeWalker's + // currentNode and child/sibling movers. createNodeIterator(root, whatToShow, filter) { - // Shares the TreeWalker implementation, but DOM 6.2 gives NodeIterator no - // subtree pruning: FILTER_REJECT behaves exactly as FILTER_SKIP there. - const iterator = this.createTreeWalker(root, whatToShow, filter); - iterator._rejectIsSkip = true; - return iterator; + // whatToShow is unsigned long; default SHOW_ALL only when the arg is + // omitted. An explicit 0 (show nothing) must stay 0, not become SHOW_ALL. + whatToShow = (whatToShow === undefined) ? 0xFFFFFFFF : (whatToShow >>> 0); + return { + root: root, + referenceNode: root, + pointerBeforeReferenceNode: true, + whatToShow: whatToShow, + filter: filter || null, + // NodeIterator prunes nothing: FILTER_REJECT behaves as FILTER_SKIP, so + // unlike the TreeWalker only "accepted or not" matters here. + _accept(node) { + if (!((whatToShow >> (node.nodeType - 1)) & 1)) return false; + if (this.filter) { + if (typeof this.filter === 'function') return this.filter(node) === 1; + if (this.filter.acceptNode) return this.filter.acceptNode(node) === 1; + } + return true; + }, + // DOM 6.2 "traverse". The pointer sits either before or after + // referenceNode, which is why reversing direction re-yields the current + // node instead of stepping over it. + _traverse(forward) { + let node = this.referenceNode; + let before = this.pointerBeforeReferenceNode; + for (;;) { + if (forward === before) { + // Consume the pointer's side without moving: it flips to the other + // side of the node it already references. + before = !before; + } else { + const step = forward ? "next_in_subtree" : "prev_in_subtree"; + const next = _wrap(+_dom(step, this.root._nid, node._nid)); + // A failed traversal leaves referenceNode and the pointer + // untouched, so the iterator can be resumed in either direction. + if (!next) return null; + node = next; + } + if (this._accept(node)) break; + } + this.referenceNode = node; + this.pointerBeforeReferenceNode = before; + return node; + }, + nextNode() { return this._traverse(true); }, + previousNode() { return this._traverse(false); }, + // Legacy no-op since DOM4, but older library code still calls it and + // used to hit "detach is not a function". + detach() {}, + }; } getSelection() { return this.defaultView ? _selectionFor(this) : null; } get activeElement() { return globalThis.__obscura_focused || this.body; } diff --git a/crates/obscura-js/src/ops.rs b/crates/obscura-js/src/ops.rs index aeb4405..edfb95e 100644 --- a/crates/obscura-js/src/ops.rs +++ b/crates/obscura-js/src/ops.rs @@ -275,6 +275,15 @@ fn op_dom_inner(state: &OpState, cmd: String, arg1: String, arg2: String) -> Str .map(|id| id.index().to_string()) .unwrap_or("-1".into()) } + // Reverse document order within a subtree, for NodeIterator's backward + // walk (which prunes nothing, so the whole step fits in the DOM layer). + "prev_in_subtree" => { + let root = NodeId::new(arg1.parse::().unwrap_or(0)); + let current = NodeId::new(arg2.parse::().unwrap_or(0)); + dom.prev_in_subtree(root, current) + .map(|id| id.index().to_string()) + .unwrap_or("-1".into()) + } // Step past a whole subtree rather than into it: NodeFilter.FILTER_REJECT // prunes the rejected node's descendants, unlike FILTER_SKIP. "next_after_subtree" => { diff --git a/crates/obscura-js/src/runtime.rs b/crates/obscura-js/src/runtime.rs index ca7ade2..003862c 100644 --- a/crates/obscura-js/src/runtime.rs +++ b/crates/obscura-js/src/runtime.rs @@ -1538,7 +1538,106 @@ mod tests { "#, ) .unwrap(); - assert_eq!(result, serde_json::json!(["P", "A"])); + // The rejected
is skipped but not pruned, so

still shows. + // The leading root is #467: an iterator yields the node it is rooted at. + assert_eq!(result, serde_json::json!(["DIV", "P", "A"])); + } + + /// Issue #467: a NodeIterator starts *before* its root, so the first + /// nextNode() returns the root itself. Aliasing createTreeWalker silently + /// dropped exactly the element the iterator was rooted at. + #[test] + fn node_iterator_yields_the_root_node_first() { + let mut rt = setup_runtime(r#"

"#); + let result = rt + .evaluate( + r#" + const root = document.getElementById('root'); + const it = document.createNodeIterator(root, NodeFilter.SHOW_ELEMENT); + const seen = []; + let node; + while ((node = it.nextNode())) seen.push(node.tagName); + return seen; + "#, + ) + .unwrap(); + assert_eq!(result, serde_json::json!(["DIV", "A"])); + } + + /// Issue #467: the NodeIterator interface surface, and that TreeWalker-only + /// members are not exposed on it. + #[test] + fn node_iterator_exposes_its_own_interface() { + let mut rt = setup_runtime(r#"
"#); + let result = rt + .evaluate( + r#" + const root = document.getElementById('root'); + const it = document.createNodeIterator(root, NodeFilter.SHOW_ELEMENT); + const before = [it.referenceNode === root, it.pointerBeforeReferenceNode]; + it.nextNode(); + return [ + before, + typeof it.detach, + it.detach() === undefined, + typeof it.previousNode, + it.root === root, + it.whatToShow, + // TreeWalker-only members must not leak onto a NodeIterator. + typeof it.currentNode, + typeof it.firstChild, + typeof it.parentNode, + // The pointer advanced past the root it just returned. + [it.referenceNode.tagName, it.pointerBeforeReferenceNode], + ]; + "#, + ) + .unwrap(); + assert_eq!( + result, + serde_json::json!([ + [true, true], + "function", + true, + "function", + true, + 1, + "undefined", + "undefined", + "undefined", + ["DIV", false] + ]) + ); + } + + /// Issue #467: previousNode() retraces the iterator, and the root is the + /// last node it yields going backwards. + #[test] + fn node_iterator_previous_node_retraces_the_walk() { + let mut rt = setup_runtime(r#"
"#); + let result = rt + .evaluate( + r#" + const root = document.getElementById('root'); + const it = document.createNodeIterator(root, NodeFilter.SHOW_ELEMENT); + const forward = []; + let node; + while ((node = it.nextNode())) forward.push(node.tagName); + const backward = []; + while ((node = it.previousNode())) backward.push(node.tagName); + return [forward, backward]; + "#, + ) + .unwrap(); + // Forward ends on ; going back re-yields (the pointer sits after + // it), then the rest in reverse, root included. + assert_eq!( + result, + serde_json::json!([ + ["DIV", "A", "B", "C"], + ["C", "B", "A", "DIV"] + ]) + ); } #[test]