From 49d4b9134815b05af5d363b735e7ef724babeafa Mon Sep 17 00:00:00 2001 From: Andrey Mnatsakanov Date: Wed, 22 Jul 2026 20:08:50 +0200 Subject: [PATCH] fix(js): descend on FILTER_SKIP in TreeWalker child/sibling movers (#469) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit firstChild/lastChild/nextSibling/previousSibling stepped straight to the next sibling whenever a node was not accepted, so a FILTER_SKIP node hid its children instead of exposing them:
, SECTION -> FILTER_SKIP w.firstChild() // null (Chrome: ) They also used the boolean `_accept`, so FILTER_REJECT (which must not descend) and FILTER_SKIP (which must) were indistinguishable — the same root cause #461 fixed for nextNode(). Replaced all four with the spec's "traverse children" and "traverse siblings" algorithms, parameterised by direction so forward and backward share one implementation and cannot drift: `edge` is first/last child and `step` is next/previous sibling. Both consult the three-valued `_filter` from #461, so SKIP descends, REJECT prunes, and the climb back out stops at root or the node the walk started from. parentNode() is left alone here: its deviations are different (it checks only the immediate parent, and excludes root, which the spec permits it to return), so it belongs in its own change. --- crates/obscura-js/js/bootstrap.js | 75 ++++++++++++++++--------- crates/obscura-js/src/runtime.rs | 93 +++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 27 deletions(-) diff --git a/crates/obscura-js/js/bootstrap.js b/crates/obscura-js/js/bootstrap.js index 8e70cb3..aa0d7b2 100644 --- a/crates/obscura-js/js/bootstrap.js +++ b/crates/obscura-js/js/bootstrap.js @@ -2681,38 +2681,59 @@ class Document extends Node { } return null; }, - firstChild() { - let child = this.currentNode.firstChild; - while (child) { - if (this._accept(child)) { this.currentNode = child; return child; } - child = child.nextSibling; + // DOM 6.1 "traverse children" (issue #469). The movers used to step + // straight to the next sibling when a node was not accepted, so a + // FILTER_SKIP node hid its children instead of exposing them. `edge` and + // `step` pick the direction: first/next for forward, last/previous for + // backward. + _traverseChildren(edge, step) { + let node = this.currentNode[edge]; + while (node) { + const verdict = this._filter(node); + if (verdict === 1) { this.currentNode = node; return node; } + // Only SKIP leaves the children eligible; REJECT prunes the subtree. + if (verdict === 3) { + const child = node[edge]; + if (child) { node = child; continue; } + } + // Subtree exhausted: step sideways, climbing out without passing + // root or the node the walk started from. + while (node) { + const sibling = node[step]; + if (sibling) { node = sibling; break; } + const parent = node.parentNode; + if (!parent || parent === this.root || parent === this.currentNode) return null; + node = parent; + } } return null; }, - lastChild() { - let child = this.currentNode.lastChild; - while (child) { - if (this._accept(child)) { this.currentNode = child; return child; } - child = child.previousSibling; + // DOM 6.1 "traverse siblings" (issue #469). + _traverseSiblings(edge, step) { + let node = this.currentNode; + if (node === this.root) return null; + for (;;) { + let sibling = node[step]; + while (sibling) { + node = sibling; + const verdict = this._filter(node); + if (verdict === 1) { this.currentNode = node; return node; } + // Descend into a skipped sibling's subtree; a rejected one is + // off-limits, and a childless one has nothing to descend into. + sibling = node[edge]; + if (verdict === 2 || !sibling) sibling = node[step]; + } + node = node.parentNode; + if (!node || node === this.root) return null; + // An accepted parent is where the walk would go next, so there is no + // sibling to return. + if (this._filter(node) === 1) return null; } - return null; - }, - nextSibling() { - let sibling = this.currentNode.nextSibling; - while (sibling) { - if (this._accept(sibling)) { this.currentNode = sibling; return sibling; } - sibling = sibling.nextSibling; - } - return null; - }, - previousSibling() { - let sibling = this.currentNode.previousSibling; - while (sibling) { - if (this._accept(sibling)) { this.currentNode = sibling; return sibling; } - sibling = sibling.previousSibling; - } - return null; }, + firstChild() { return this._traverseChildren('firstChild', 'nextSibling'); }, + lastChild() { return this._traverseChildren('lastChild', 'previousSibling'); }, + nextSibling() { return this._traverseSiblings('firstChild', 'nextSibling'); }, + previousSibling() { return this._traverseSiblings('lastChild', 'previousSibling'); }, parentNode() { let parent = this.currentNode.parentNode; if (parent && parent !== this.root && this._accept(parent)) { diff --git a/crates/obscura-js/src/runtime.rs b/crates/obscura-js/src/runtime.rs index ca7ade2..2afc944 100644 --- a/crates/obscura-js/src/runtime.rs +++ b/crates/obscura-js/src/runtime.rs @@ -1541,6 +1541,99 @@ mod tests { assert_eq!(result, serde_json::json!(["P", "A"])); } + /// Issue #469: FILTER_SKIP leaves a skipped node's children eligible, so + /// firstChild()/lastChild() must descend into them. FILTER_REJECT must not. + #[test] + fn tree_walker_child_movers_descend_on_skip_but_not_on_reject() { + let mut rt = setup_runtime( + r#"
"#, + ); + let result = rt + .evaluate( + r#" + const root = document.getElementById('root'); + function mover(verdict, method) { + const w = document.createTreeWalker(root, NodeFilter.SHOW_ELEMENT, { + acceptNode(node) { + return node.tagName === 'SECTION' ? verdict : NodeFilter.FILTER_ACCEPT; + } + }); + const found = w[method](); + return found ? found.tagName : null; + } + return [ + mover(NodeFilter.FILTER_SKIP, 'firstChild'), + mover(NodeFilter.FILTER_SKIP, 'lastChild'), + mover(NodeFilter.FILTER_REJECT, 'firstChild'), + mover(NodeFilter.FILTER_REJECT, 'lastChild'), + ]; + "#, + ) + .unwrap(); + // SKIP descends into
; REJECT prunes it and finds nothing else. + assert_eq!(result, serde_json::json!(["A", "B", null, null])); + } + + /// Issue #469: nextSibling()/previousSibling() must descend into a skipped + /// sibling's subtree rather than stepping straight over it. + #[test] + fn tree_walker_sibling_movers_descend_into_skipped_siblings() { + let mut rt = setup_runtime( + r#"

"#, + ); + let result = rt + .evaluate( + r#" + const root = document.getElementById('root'); + function mover(verdict, method, from) { + const w = document.createTreeWalker(root, NodeFilter.SHOW_ELEMENT, { + acceptNode(node) { + return node.tagName === 'SECTION' ? verdict : NodeFilter.FILTER_ACCEPT; + } + }); + w.currentNode = document.getElementById(from); + const found = w[method](); + return found ? found.tagName : null; + } + return [ + //
is skipped, so its child is the next sibling. + mover(NodeFilter.FILTER_SKIP, 'nextSibling', 'start'), + // Rejected: the subtree is off-limits, so skip past to . + mover(NodeFilter.FILTER_REJECT, 'nextSibling', 'start'), + ]; + "#, + ) + .unwrap(); + assert_eq!(result, serde_json::json!(["A", "Q"])); + } + + /// Issue #469: the backward sibling mover descends to *last* children. + #[test] + fn tree_walker_previous_sibling_descends_to_last_child() { + let mut rt = setup_runtime( + r#"

"#, + ); + let result = rt + .evaluate( + r#" + const root = document.getElementById('root'); + const w = document.createTreeWalker(root, NodeFilter.SHOW_ELEMENT, { + acceptNode(node) { + return node.tagName === 'SECTION' + ? NodeFilter.FILTER_SKIP + : NodeFilter.FILTER_ACCEPT; + } + }); + w.currentNode = document.getElementById('start'); + const found = w.previousSibling(); + return found ? found.tagName : null; + "#, + ) + .unwrap(); + // Reverse order descends to
's last child, not its first. + assert_eq!(result, serde_json::json!("B")); + } + #[test] fn append_child_flattens_document_fragment() { let mut rt = setup_runtime(r#"
"#);