Skip to content

Commit 18c851e

Browse files
karlseguinkarlseguin
authored
Merge pull request #1149 from lightpanda-io/iterators_and_walker_fix
Improve correctness of NodeIterator and Treewalker
2 parents ff70f4e + 4db8a96 commit 18c851e

File tree

4 files changed

+289
-12
lines changed

4 files changed

+289
-12
lines changed

‎src/browser/dom/node_filter.zig‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,9 @@ pub fn verify(what_to_show: u32, filter: ?js.Function, node: *parser.Node) !Veri
4747
const node_type = parser.nodeType(node);
4848

4949
// Verify that we can show this node type.
50+
// Per the DOM spec, what_to_show filters which nodes to return, but should
51+
// still traverse children. So we return .skip (not .reject) when the node
52+
// type doesn't match.
5053
if (!switch (node_type) {
5154
.attribute => what_to_show & NodeFilter._SHOW_ATTRIBUTE != 0,
5255
.cdata_section => what_to_show & NodeFilter._SHOW_CDATA_SECTION != 0,
@@ -60,7 +63,7 @@ pub fn verify(what_to_show: u32, filter: ?js.Function, node: *parser.Node) !Veri
6063
.notation => what_to_show & NodeFilter._SHOW_NOTATION != 0,
6164
.processing_instruction => what_to_show & NodeFilter._SHOW_PROCESSING_INSTRUCTION != 0,
6265
.text => what_to_show & NodeFilter._SHOW_TEXT != 0,
63-
}) return .reject;
66+
}) return .skip;
6467

6568
// Verify that we aren't filtering it out.
6669
if (filter) |f| {

‎src/browser/dom/node_iterator.zig‎

Lines changed: 34 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -74,10 +74,10 @@ pub const NodeIterator = struct {
7474

7575
return .{
7676
.root = node,
77-
.reference_node = node,
78-
.what_to_show = what_to_show,
7977
.filter = filter,
78+
.reference_node = node,
8079
.filter_func = filter_func,
80+
.what_to_show = what_to_show,
8181
};
8282
}
8383

@@ -106,6 +106,7 @@ pub const NodeIterator = struct {
106106
defer self.callbackEnd();
107107

108108
if (self.pointer_before_current) {
109+
self.pointer_before_current = false;
109110
// Unlike TreeWalker, NodeIterator starts at the first node
110111
if (.accept == try NodeFilter.verify(self.what_to_show, self.filter_func, self.reference_node)) {
111112
self.pointer_before_current = false;
@@ -120,9 +121,21 @@ pub const NodeIterator = struct {
120121

121122
var current = self.reference_node;
122123
while (current != self.root) {
123-
if (try self.nextSibling(current)) |sibling| {
124-
self.reference_node = sibling;
125-
return try Node.toInterface(sibling);
124+
// Try to get next sibling (including .skip/.reject nodes we need to descend into)
125+
if (try self.nextSiblingOrSkipReject(current)) |result| {
126+
if (result.should_descend) {
127+
// This is a .skip/.reject node - try to find acceptable children within it
128+
if (try self.firstChild(result.node)) |child| {
129+
self.reference_node = child;
130+
return try Node.toInterface(child);
131+
}
132+
// No acceptable children, continue looking at this node's siblings
133+
current = result.node;
134+
continue;
135+
}
136+
// This is an .accept node - return it
137+
self.reference_node = result.node;
138+
return try Node.toInterface(result.node);
126139
}
127140

128141
current = (parser.nodeParentNode(current)) orelse break;
@@ -254,6 +267,22 @@ pub const NodeIterator = struct {
254267
return null;
255268
}
256269

270+
// Get the next sibling that is either acceptable or should be descended into (skip/reject)
271+
fn nextSiblingOrSkipReject(self: *const NodeIterator, node: *parser.Node) !?struct { node: *parser.Node, should_descend: bool } {
272+
var current = node;
273+
274+
while (true) {
275+
current = (parser.nodeNextSibling(current)) orelse return null;
276+
277+
switch (try NodeFilter.verify(self.what_to_show, self.filter_func, current)) {
278+
.accept => return .{ .node = current, .should_descend = false },
279+
.skip, .reject => return .{ .node = current, .should_descend = true },
280+
}
281+
}
282+
283+
return null;
284+
}
285+
257286
fn callbackStart(self: *NodeIterator) !void {
258287
if (self.is_in_callback) {
259288
// this is the correct DOMExeption

‎src/browser/dom/tree_walker.zig‎

Lines changed: 41 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,23 @@ pub const TreeWalker = struct {
144144
return null;
145145
}
146146

147+
// Get the next sibling that is either acceptable or should be descended into (skip)
148+
fn nextSiblingOrSkip(self: *const TreeWalker, node: *parser.Node) !?struct { node: *parser.Node, should_descend: bool } {
149+
var current = node;
150+
151+
while (true) {
152+
current = (parser.nodeNextSibling(current)) orelse return null;
153+
154+
switch (try NodeFilter.verify(self.what_to_show, self.filter_func, current)) {
155+
.accept => return .{ .node = current, .should_descend = false },
156+
.skip => return .{ .node = current, .should_descend = true },
157+
.reject => continue,
158+
}
159+
}
160+
161+
return null;
162+
}
163+
147164
fn previousSibling(self: *const TreeWalker, node: *parser.Node) !?*parser.Node {
148165
var current = node;
149166

@@ -193,19 +210,37 @@ pub const TreeWalker = struct {
193210
}
194211

195212
pub fn _nextNode(self: *TreeWalker) !?NodeUnion {
196-
if (try self.firstChild(self.current_node)) |child| {
213+
var current = self.current_node;
214+
215+
// First, try to go to first child of current node
216+
if (try self.firstChild(current)) |child| {
197217
self.current_node = child;
198218
return try Node.toInterface(child);
199219
}
200220

201-
var current = self.current_node;
221+
// No acceptable children, move to next node in tree
202222
while (current != self.root) {
203-
if (try self.nextSibling(current)) |sibling| {
204-
self.current_node = sibling;
205-
return try Node.toInterface(sibling);
223+
const result = try self.nextSiblingOrSkip(current) orelse {
224+
// No next sibling, go up to parent and continue
225+
// or, if there is no parent, we're done
226+
current = (parser.nodeParentNode(current)) orelse break;
227+
continue;
228+
};
229+
230+
231+
if (!result.should_descend) {
232+
// This is an .accept node - return it
233+
self.current_node = result.node;
234+
return try Node.toInterface(result.node);
206235
}
207236

208-
current = (parser.nodeParentNode(current)) orelse break;
237+
// This is a .skip node - try to find acceptable children within it
238+
if (try self.firstChild(result.node)) |child| {
239+
self.current_node = child;
240+
return try Node.toInterface(child);
241+
}
242+
// No acceptable children, continue looking at this node's siblings
243+
current = result.node;
209244
}
210245

211246
return null;

‎src/tests/dom/node_filter.html‎

Lines changed: 210 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,219 @@
11
<!DOCTYPE html>
22
<script src="../testing.js"></script>
3+
4+
<!-- Test fixture -->
5+
<div id="container">
6+
<!-- comment1 -->
7+
<div id="outer">
8+
<!-- comment2 -->
9+
<span id="inner">
10+
<!-- comment3 -->
11+
Text content
12+
<!-- comment4 -->
13+
</span>
14+
<!-- comment5 -->
15+
</div>
16+
<!-- comment6 -->
17+
</div>
18+
319
<script id=nodeFilter>
420
testing.expectEqual(1, NodeFilter.FILTER_ACCEPT);
521
testing.expectEqual(2, NodeFilter.FILTER_REJECT);
622
testing.expectEqual(3, NodeFilter.FILTER_SKIP);
723
testing.expectEqual(4294967295, NodeFilter.SHOW_ALL);
824
testing.expectEqual(129, NodeFilter.SHOW_ELEMENT | NodeFilter.SHOW_COMMENT);
925
</script>
26+
27+
<script id=treeWalkerComments>
28+
{
29+
const container = $('#container');
30+
const walker = document.createTreeWalker(
31+
container,
32+
NodeFilter.SHOW_COMMENT,
33+
null,
34+
false
35+
);
36+
37+
const comments = [];
38+
let node;
39+
while (node = walker.nextNode()) {
40+
comments.push(node.data.trim());
41+
}
42+
43+
// Should find all 6 comments, including those nested inside elements
44+
testing.expectEqual(6, comments.length);
45+
testing.expectEqual('comment1', comments[0]);
46+
testing.expectEqual('comment2', comments[1]);
47+
testing.expectEqual('comment3', comments[2]);
48+
testing.expectEqual('comment4', comments[3]);
49+
testing.expectEqual('comment5', comments[4]);
50+
testing.expectEqual('comment6', comments[5]);
51+
}
52+
</script>
53+
54+
<script id=treeWalkerElements>
55+
{
56+
const container = $('#container');
57+
const walker = document.createTreeWalker(
58+
container,
59+
NodeFilter.SHOW_ELEMENT,
60+
null,
61+
false
62+
);
63+
64+
const elements = [];
65+
let node;
66+
while (node = walker.nextNode()) {
67+
if (node.id) {
68+
elements.push(node.id);
69+
}
70+
}
71+
72+
// Should find the 2 nested elements (outer and inner)
73+
testing.expectEqual(2, elements.length);
74+
testing.expectEqual('outer', elements[0]);
75+
testing.expectEqual('inner', elements[1]);
76+
}
77+
</script>
78+
79+
<script id=treeWalkerAll>
80+
{
81+
const container = $('#container');
82+
const walker = document.createTreeWalker(
83+
container,
84+
NodeFilter.SHOW_ALL,
85+
null,
86+
false
87+
);
88+
89+
let commentCount = 0;
90+
let elementCount = 0;
91+
let textCount = 0;
92+
93+
let node;
94+
while (node = walker.nextNode()) {
95+
if (node.nodeType === 8) commentCount++; // Comment
96+
else if (node.nodeType === 1) elementCount++; // Element
97+
else if (node.nodeType === 3) textCount++; // Text
98+
}
99+
100+
testing.expectEqual(6, commentCount);
101+
testing.expectEqual(2, elementCount);
102+
testing.expectEqual(true, textCount > 0);
103+
}
104+
</script>
105+
106+
<script id=treeWalkerCombined>
107+
{
108+
const container = $('#container');
109+
const walker = document.createTreeWalker(
110+
container,
111+
NodeFilter.SHOW_ELEMENT | NodeFilter.SHOW_COMMENT,
112+
null,
113+
false
114+
);
115+
116+
let commentCount = 0;
117+
let elementCount = 0;
118+
119+
let node;
120+
while (node = walker.nextNode()) {
121+
if (node.nodeType === 8) commentCount++; // Comment
122+
else if (node.nodeType === 1) elementCount++; // Element
123+
}
124+
125+
// Should find 6 comments and 2 elements, but no text nodes
126+
testing.expectEqual(6, commentCount);
127+
testing.expectEqual(2, elementCount);
128+
}
129+
</script>
130+
131+
<script id=treeWalkerCustomFilter>
132+
{
133+
const container = $('#container');
134+
135+
// Filter that accepts only elements with id
136+
const walker = document.createTreeWalker(
137+
container,
138+
NodeFilter.SHOW_ELEMENT,
139+
{
140+
acceptNode: function(node) {
141+
return node.id ? NodeFilter.FILTER_ACCEPT : NodeFilter.FILTER_SKIP;
142+
}
143+
},
144+
false
145+
);
146+
147+
const elements = [];
148+
let node;
149+
while (node = walker.nextNode()) {
150+
elements.push(node.id);
151+
}
152+
153+
// Should find only elements with id (outer and inner)
154+
testing.expectEqual(2, elements.length);
155+
testing.expectEqual('outer', elements[0]);
156+
testing.expectEqual('inner', elements[1]);
157+
}
158+
</script>
159+
160+
<script id=nodeIteratorComments>
161+
{
162+
const container = $('#container');
163+
const iterator = document.createNodeIterator(
164+
container,
165+
NodeFilter.SHOW_COMMENT,
166+
null,
167+
false
168+
);
169+
170+
const comments = [];
171+
let node;
172+
while (node = iterator.nextNode()) {
173+
comments.push(node.data.trim());
174+
}
175+
176+
// Should find all 6 comments, including those nested inside elements
177+
testing.expectEqual(6, comments.length);
178+
testing.expectEqual('comment1', comments[0]);
179+
testing.expectEqual('comment2', comments[1]);
180+
testing.expectEqual('comment3', comments[2]);
181+
testing.expectEqual('comment4', comments[3]);
182+
testing.expectEqual('comment5', comments[4]);
183+
testing.expectEqual('comment6', comments[5]);
184+
}
185+
</script>
186+
187+
<script id=reactLikeScenario>
188+
{
189+
// Test a React-like scenario with comment markers
190+
const div = document.createElement('div');
191+
div.innerHTML = `
192+
<a href="/">
193+
<!--$-->
194+
<svg viewBox="0 0 10 10">
195+
<path d="M0,0 L10,10" />
196+
</svg>
197+
<!--/$-->
198+
</a>
199+
`;
200+
201+
const walker = document.createTreeWalker(
202+
div,
203+
NodeFilter.SHOW_COMMENT,
204+
null,
205+
false
206+
);
207+
208+
const comments = [];
209+
let node;
210+
while (node = walker.nextNode()) {
211+
comments.push(node.data);
212+
}
213+
214+
// Should find both React markers even though they're nested inside <a>
215+
testing.expectEqual(2, comments.length);
216+
testing.expectEqual('$', comments[0]);
217+
testing.expectEqual('/$', comments[1]);
218+
}
219+
</script>

0 commit comments

Comments
 (0)