Merge pull request #465 from mnaza/fix/treewalker-previousnode-462

fix(js): walk reverse document order in TreeWalker.previousNode() (#462)

crates/obscura-dom/src/tree.rs

@@ -564,7 +564,28 @@ impl DomTree {
         if let Some(child) = current_node.first_child {
             return Some(child);
         }
+        Self::climb_to_next_sibling(&inner, root, current)
+    }
 
+    /// Returns the node after the whole subtree rooted at `current`, in document
+    /// order, without leaving the subtree rooted at `root`.
+    ///
+    /// This is `next_in_subtree` minus the descend-into-children step, which is
+    /// what `NodeFilter.FILTER_REJECT` needs: it rejects a node *and* its
+    /// descendants, unlike `FILTER_SKIP`, which only skips the node itself and
+    /// is served by `next_in_subtree`.
+    pub fn next_after_subtree(&self, root: NodeId, current: NodeId) -> Option<NodeId> {
+        let inner = self.inner.borrow();
+        Self::climb_to_next_sibling(&inner, root, current)
+    }
+
+    /// Follow `current`'s next sibling, climbing ancestors until one has a next
+    /// sibling — without stepping outside `root`.
+    fn climb_to_next_sibling(
+        inner: &DomTreeInner,
+        root: NodeId,
+        current: NodeId,
+    ) -> Option<NodeId> {
         let mut node_id = current;
         for _ in 0..=inner.nodes.len() {
             if node_id == root {
@@ -1027,6 +1048,29 @@ mod tests {
         assert_eq!(tree.next_in_subtree(root, second), None);
     }
 
+    #[test]
+    fn test_next_after_subtree_skips_descendants() {
+        // Same shape as above: root > [first > nested, second]. Stepping past
+        // `first` must land on `second`, not descend into `nested` — that is
+        // what NodeFilter.FILTER_REJECT needs.
+        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);
+
+        assert_eq!(tree.next_after_subtree(root, first), Some(second));
+        // A leaf behaves identically to next_in_subtree: there is no subtree.
+        assert_eq!(tree.next_after_subtree(root, nested), Some(second));
+        assert_eq!(tree.next_after_subtree(root, second), None);
+        // Rejecting the root itself exhausts the walk rather than escaping it.
+        assert_eq!(tree.next_after_subtree(root, root), None);
+    }
+
     // Builds a chain of `depth` nested <div> 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

crates/obscura-js/js/bootstrap.js

@@ -2616,36 +2616,68 @@ class Document extends Node {
       currentNode: root,
       whatToShow: whatToShow,
       filter: filter || null,
-      _accept(node) {
+      // Three-valued per NodeFilter: 1 ACCEPT, 2 REJECT, 3 SKIP. REJECT and
+      // SKIP both mean "don't return this node", but only REJECT prunes its
+      // descendants, so nextNode() needs to tell them apart (issue #461).
+      // A node filtered out by whatToShow is a SKIP: the spec never consults
+      // the filter for it, and its descendants stay eligible.
+      _filter(node) {
         const nodeType = node.nodeType;
-        const show = (whatToShow >> (nodeType - 1)) & 1;
-        if (!show) return false;
+        if (!((whatToShow >> (nodeType - 1)) & 1)) return 3;
         if (this.filter) {
-          if (typeof this.filter === 'function') return this.filter(node) === 1;
-          if (this.filter.acceptNode) return this.filter.acceptNode(node) === 1;
+          if (typeof this.filter === 'function') return this.filter(node);
+          if (this.filter.acceptNode) return this.filter.acceptNode(node);
         }
-        return true;
+        return 1;
       },
+      _accept(node) { return this._filter(node) === 1; },
       nextNode() {
         let node = _wrap(+_dom("next_in_subtree", this.root._nid, this.currentNode._nid));
         while (node) {
-          if (this._accept(node)) { this.currentNode = node; return node; }
-          node = _wrap(+_dom("next_in_subtree", this.root._nid, node._nid));
+          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";
+          node = _wrap(+_dom(step, this.root._nid, node._nid));
         }
         return null;
       },
+      // DOM 6.1 "previousNode", implemented as specified (issue #462). The old
+      // version looked at exactly one candidate — the previous sibling's
+      // deepest last child — and returned null the moment it was filtered out,
+      // so a backward walk died mid-tree the way nextNode used to before #432.
+      //
+      // Unlike nextNode this stays in JS rather than using a DOM traversal op:
+      // the descent into last children has to stop on FILTER_REJECT, so the
+      // filter is consulted at every step anyway and there is no run of
+      // crossings for a native helper to collapse.
       previousNode() {
         let node = this.currentNode;
-        if (node === this.root) return null;
-        let sibling = node.previousSibling;
-        if (sibling) {
-          while (sibling.lastChild) sibling = sibling.lastChild;
-          if (this._accept(sibling)) { this.currentNode = sibling; return sibling; }
-        }
-        let parent = node.parentNode;
-        if (parent && parent !== this.root && this._accept(parent)) {
-          this.currentNode = parent;
-          return parent;
+        while (node !== this.root) {
+          let sibling = node.previousSibling;
+          while (sibling) {
+            node = sibling;
+            let verdict = this._filter(node);
+            // Descend to the deepest last descendant, but never into a rejected
+            // subtree — that is what makes REJECT prune backwards as well.
+            while (verdict !== 2 && node.lastChild) {
+              node = node.lastChild;
+              verdict = this._filter(node);
+            }
+            if (verdict === 1) { this.currentNode = node; return node; }
+            sibling = node.previousSibling;
+          }
+          const parent = node.parentNode;
+          // Reaching root (or a detached node) ends the walk: root is never
+          // returned by a backward traversal.
+          if (!parent || node === this.root) return null;
+          node = parent;
+          if (node === this.root) return null;
+          if (this._filter(node) === 1) { this.currentNode = node; return node; }
         }
         return null;
       },
@@ -2693,7 +2725,11 @@ class Document extends Node {
     return walker;
   }
   createNodeIterator(root, whatToShow, filter) {
-    return this.createTreeWalker(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;
   }
   getSelection() { return this.defaultView ? _selectionFor(this) : null; }
   get activeElement() { return globalThis.__obscura_focused || this.body; }

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())
         }
+        // Step past a whole subtree rather than into it: NodeFilter.FILTER_REJECT
+        // prunes the rejected node's descendants, unlike FILTER_SKIP.
+        "next_after_subtree" => {
+            let root = NodeId::new(arg1.parse::<u32>().unwrap_or(0));
+            let current = NodeId::new(arg2.parse::<u32>().unwrap_or(0));
+            dom.next_after_subtree(root, current)
+                .map(|id| id.index().to_string())
+                .unwrap_or("-1".into())
+        }
         "child_nodes" => {
             let nid = arg1.parse::<u32>().unwrap_or(0);
             let ids: Vec<i32> = dom.children(NodeId::new(nid)).iter().map(|id| id.index() as i32).collect();

crates/obscura-js/src/runtime.rs

@@ -1378,6 +1378,169 @@ mod tests {
         );
     }
 
+    /// Issue #461: FILTER_REJECT must prune the rejected node's whole subtree,
+    /// while FILTER_SKIP only skips the node and leaves descendants eligible.
+    /// Collapsing both into "not accepted" let a TreeWalker yield nodes from
+    /// inside a subtree the page explicitly rejected.
+    #[test]
+    fn tree_walker_filter_reject_prunes_the_whole_subtree() {
+        let mut rt = setup_runtime(
+            r#"<div id="root"><section><p>deep</p></section><a></a></div>"#,
+        );
+        rt.run_page_init();
+        let result = rt
+            .evaluate(
+                r#"
+                const root = document.getElementById('root');
+                function walk(verdict) {
+                    const w = document.createTreeWalker(root, NodeFilter.SHOW_ELEMENT, {
+                        acceptNode(node) {
+                            return node.tagName === 'SECTION' ? verdict : NodeFilter.FILTER_ACCEPT;
+                        }
+                    });
+                    const seen = [];
+                    let node;
+                    while ((node = w.nextNode())) seen.push(node.tagName);
+                    return seen;
+                }
+                return [walk(NodeFilter.FILTER_REJECT), walk(NodeFilter.FILTER_SKIP)];
+                "#,
+            )
+            .unwrap();
+        // REJECT drops <p> with its <section> parent; SKIP drops only <section>.
+        assert_eq!(result, serde_json::json!([["A"], ["P", "A"]]));
+    }
+
+    /// Issue #462: previousNode() must walk reverse document order until a node
+    /// is accepted, not give up as soon as the first candidate is filtered out.
+    #[test]
+    fn previous_node_walks_reverse_document_order() {
+        let mut rt = setup_runtime(r#"<div id="root"><a><b></b></a><c></c></div>"#);
+        rt.run_page_init();
+        let result = rt
+            .evaluate(
+                r#"
+                const root = document.getElementById('root');
+                const w = document.createTreeWalker(root, NodeFilter.SHOW_ELEMENT, {
+                    acceptNode(node) {
+                        return node.tagName === 'B'
+                            ? NodeFilter.FILTER_SKIP
+                            : NodeFilter.FILTER_ACCEPT;
+                    }
+                });
+                const forward = [];
+                let node;
+                while ((node = w.nextNode())) forward.push(node.tagName);
+                const backward = [];
+                while ((node = w.previousNode())) backward.push(node.tagName);
+                return [forward, backward];
+                "#,
+            )
+            .unwrap();
+        // From <c>, the previous sibling's deepest last child <b> is skipped, so
+        // the walk must keep going up to <a> instead of returning null.
+        assert_eq!(result, serde_json::json!([["A", "C"], ["A"]]));
+    }
+
+    /// Issue #462: a backward walk must retrace a forward walk exactly, and stop
+    /// at the root without ever returning it.
+    #[test]
+    fn previous_node_retraces_a_full_forward_walk() {
+        let mut rt = setup_runtime(
+            r#"<div id="root"><section><p>one</p><span></span></section><a><b></b></a></div>"#,
+        );
+        rt.run_page_init();
+        let result = rt
+            .evaluate(
+                r#"
+                const root = document.getElementById('root');
+                const w = document.createTreeWalker(root, NodeFilter.SHOW_ELEMENT);
+                const forward = [];
+                let node;
+                while ((node = w.nextNode())) forward.push(node.tagName);
+                const backward = [];
+                while ((node = w.previousNode())) backward.push(node.tagName);
+                backward.reverse();
+                // previousNode never yields root, and never yields the node the
+                // forward walk ended on, so compare against forward minus its last.
+                // A failed traversal leaves currentNode untouched (DOM 6.1), so
+                // it stays on the last node previousNode did return.
+                return [forward, backward, w.currentNode.tagName];
+                "#,
+            )
+            .unwrap();
+        assert_eq!(
+            result,
+            serde_json::json!([
+                ["SECTION", "P", "SPAN", "A", "B"],
+                ["SECTION", "P", "SPAN", "A"],
+                "SECTION"
+            ])
+        );
+    }
+
+    /// Issue #462: FILTER_REJECT prunes a subtree in the backward direction too
+    /// — the descent into a rejected node's last children must stop.
+    #[test]
+    fn previous_node_honours_filter_reject_subtree_pruning() {
+        let mut rt = setup_runtime(
+            r#"<div id="root"><a></a><section><p>deep</p></section><c></c></div>"#,
+        );
+        rt.run_page_init();
+        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_REJECT
+                            : NodeFilter.FILTER_ACCEPT;
+                    }
+                });
+                while (w.nextNode()) { /* advance to the last accepted node */ }
+                const backward = [];
+                let node;
+                while ((node = w.previousNode())) backward.push(node.tagName);
+                return backward;
+                "#,
+            )
+            .unwrap();
+        // <p> lives inside the rejected <section>, so the backward walk from <c>
+        // must jump straight to <a>.
+        assert_eq!(result, serde_json::json!(["A"]));
+    }
+
+    /// Issue #461: NodeIterator has no subtree pruning — DOM 6.2 says
+    /// FILTER_REJECT behaves as FILTER_SKIP there. The shared walker must not
+    /// leak TreeWalker's pruning into it.
+    #[test]
+    fn node_iterator_treats_filter_reject_as_skip() {
+        let mut rt = setup_runtime(
+            r#"<div id="root"><section><p>deep</p></section><a></a></div>"#,
+        );
+        rt.run_page_init();
+        let result = rt
+            .evaluate(
+                r#"
+                const root = document.getElementById('root');
+                const it = document.createNodeIterator(root, NodeFilter.SHOW_ELEMENT, {
+                    acceptNode(node) {
+                        return node.tagName === 'SECTION'
+                            ? NodeFilter.FILTER_REJECT
+                            : NodeFilter.FILTER_ACCEPT;
+                    }
+                });
+                const seen = [];
+                let node;
+                while ((node = it.nextNode())) seen.push(node.tagName);
+                return seen;
+                "#,
+            )
+            .unwrap();
+        assert_eq!(result, serde_json::json!(["P", "A"]));
+    }
+
     #[test]
     fn append_child_flattens_document_fragment() {
         let mut rt = setup_runtime(r#"<main id="host"></main>"#);