Merge pull request #472 from mnaza/fix/node-iterator-467

fix(js): implement a real NodeIterator instead of aliasing TreeWalker (#467)

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<NodeId> {
+        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(
@@ -1138,6 +1173,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 <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

@@ -2651,11 +2651,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;
@@ -2738,12 +2735,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; }

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::<u32>().unwrap_or(0));
+            let current = NodeId::new(arg2.parse::<u32>().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" => {

crates/obscura-js/src/runtime.rs

@@ -1538,7 +1538,106 @@ mod tests {
                 "#,
             )
             .unwrap();
-        assert_eq!(result, serde_json::json!(["P", "A"]));
+        // The rejected <section> is skipped but not pruned, so <p> 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#"<div id="root"><a></a></div>"#);
+        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#"<div id="root"><a></a></div>"#);
+        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#"<div id="root"><a><b></b></a><c></c></div>"#);
+        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 <c>; going back re-yields <c> (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"]
+            ])
+        );
     }
 
     /// Issue #463: `<template>` contents are parsed into the node's