Unify HTML DOM collection traversal (#358)

docs/developers-guide.md

@@ -77,6 +77,14 @@ restores the separator row with widths derived from the final table body.
   use this macro and supply a descriptive expect message that identifies the
   pattern whose compilation failed.
 
+`src/html.rs`:
+
+- `collect_matching`: Performs the canonical pre-order, depth-first DOM
+  traversal and clones each matching handle into the supplied output vector.
+  `collect_tables` and `collect_rows` delegate their predicates to this helper;
+  all new DOM collection must reuse it rather than introduce another recursive
+  walker.
+
 `src/textproc.rs`:
 
 - `leading_indent(s: &str) -> &str`: Returns the leading whitespace prefix of
@@ -874,6 +882,10 @@ cleared. `flush_raw` exists for the fenced-block escape path: it emits the
 buffered lines verbatim without conversion, so raw HTML inside a fenced code
 block is preserved unchanged.
 
+DOM conversion uses `collect_tables` and `collect_rows` as semantic entry
+points. Both delegate traversal to the canonical `collect_matching` walker, so
+pre-order traversal, handle cloning, and child borrowing remain consistent.
+
 ### 1.4. `DefinitionScanState` (`src/footnotes/renumber/definitions.rs`)
 
 `DefinitionScanState` accumulates the footnote-definition rewrite plan during a

src/html.rs

@@ -102,24 +102,31 @@ fn is_element(handle: &Handle, tag: &str) -> bool {
 /// Returns `true` if `handle` represents a `<td>` or `<th>` element.
 fn is_table_cell(handle: &Handle) -> bool { is_element(handle, "td") || is_element(handle, "th") }
 
-/// Walks the DOM tree collecting `<table>` nodes under `handle`.
-fn collect_tables(handle: &Handle, tables: &mut Vec<Handle>) {
-    if is_element(handle, "table") {
-        tables.push(handle.clone());
+/// DOM input is unbounded; bounded model checking is not a good fit for
+/// verifying traversal over arbitrary tree depth. Property tests cover
+/// invariants via random small-tree generation instead.
+///
+/// Walks the DOM tree in pre-order, cloning nodes that satisfy `pred` into `out`.
+fn collect_matching<F>(handle: &Handle, pred: F, out: &mut Vec<Handle>)
+where
+    F: Fn(&Handle) -> bool + Copy,
+{
+    if pred(handle) {
+        out.push(handle.clone());
     }
     for child in handle.children.borrow().iter() {
-        collect_tables(child, tables);
+        collect_matching(child, pred, out);
     }
 }
 
+/// Walks the DOM tree collecting `<table>` nodes under `handle`.
+fn collect_tables(handle: &Handle, tables: &mut Vec<Handle>) {
+    collect_matching(handle, |node| is_element(node, "table"), tables);
+}
+
 /// Collects all `<tr>` nodes beneath `handle`.
 fn collect_rows(handle: &Handle, rows: &mut Vec<Handle>) {
-    if is_element(handle, "tr") {
-        rows.push(handle.clone());
-    }
-    for child in handle.children.borrow().iter() {
-        collect_rows(child, rows);
-    }
+    collect_matching(handle, |node| is_element(node, "tr"), rows);
 }
 
 fn is_bold_tag(tag: &str) -> bool {

src/html_tests.rs

@@ -49,9 +49,51 @@ mod proptest_tests {
     //! These generated cases complement the parent test module by checking
     //! `HtmlTableState` behaviour across varied open and close sequences.
 
+    use std::rc::Rc;
+
+    use html5ever::{driver::ParseOpts, parse_document, tendril::TendrilSink};
+    use markup5ever_rcdom::{Handle, RcDom};
     use proptest::prelude::*;
 
-    use super::HtmlTableState;
+    use super::{HtmlTableState, collect_matching, is_element};
+
+    fn html_fragment_strategy() -> impl Strategy<Value = String> {
+        (
+            proptest::collection::vec(proptest::collection::vec(0usize..=4, 0..=6), 0..=4),
+            0usize..=4,
+        )
+            .prop_map(|(tables, nested_depth)| {
+                let nested_tables = (0..nested_depth).fold(String::new(), |nested, level| {
+                    format!("<table><tr><td>level-{level}{nested}</td></tr></table>")
+                });
+                let mut html = tables.into_iter().fold(String::new(), |mut html, rows| {
+                    html.push_str("<table>");
+                    for cell_count in rows {
+                        html.push_str("<tr>");
+                        for index in 0..cell_count {
+                            html.push_str("<td>cell-");
+                            html.push_str(&index.to_string());
+                            html.push_str("</td>");
+                        }
+                        html.push_str("</tr>");
+                    }
+                    html.push_str("</table>");
+                    html
+                });
+                html.push_str(&nested_tables);
+                html
+            })
+    }
+
+    fn parse_html(source: String) -> RcDom {
+        parse_document(RcDom::default(), ParseOpts::default()).one(source)
+    }
+
+    fn collect_tag(document: &Handle, tag: &'static str) -> Vec<Handle> {
+        let mut matches = Vec::new();
+        collect_matching(document, |node| is_element(node, tag), &mut matches);
+        matches
+    }
 
     proptest! {
         #[test]
@@ -92,5 +134,39 @@ mod proptest_tests {
             prop_assert!(!state.in_html());
             prop_assert_eq!(state.depth, 0);
         }
+
+        #[test]
+        fn collect_matching_count_equals_source_tag_count(
+            source in html_fragment_strategy(),
+            tag in prop_oneof![Just("table"), Just("tr"), Just("td")],
+        ) {
+            let expected_count = source.matches(&format!("<{tag}")).count();
+            let dom = parse_html(source);
+
+            prop_assert_eq!(collect_tag(&dom.document, tag).len(), expected_count);
+        }
+
+        #[test]
+        fn collect_matching_order_is_deterministic(
+            source in html_fragment_strategy(),
+            tag in prop_oneof![Just("table"), Just("tr"), Just("td")],
+        ) {
+            let dom = parse_html(source);
+            let first = collect_tag(&dom.document, tag);
+            let second = collect_tag(&dom.document, tag);
+
+            prop_assert_eq!(first.len(), second.len());
+            prop_assert!(first.iter().zip(&second).all(|(left, right)| Rc::ptr_eq(left, right)));
+        }
+
+        #[test]
+        fn collect_matching_returns_empty_for_non_matching_documents(
+            content in "[a-z ]{0,64}",
+            tag in prop_oneof![Just("table"), Just("tr"), Just("td")],
+        ) {
+            let dom = parse_html(format!("<main><p>{content}</p></main>"));
+
+            prop_assert!(collect_tag(&dom.document, tag).is_empty());
+        }
     }
 }

tests/html_snapshots.rs

@@ -0,0 +1,59 @@
+//! Snapshot coverage for representative HTML-to-Markdown table conversions.
+
+use mdtablefix::convert_html_tables;
+use rstest::rstest;
+
+fn snapshot_conversion(name: &str, input: &str) {
+    let lines = input.lines().map(ToString::to_string).collect::<Vec<_>>();
+    let output = convert_html_tables(&lines).join("\n");
+
+    insta::with_settings!({
+        snapshot_path => "snapshots",
+        prepend_module_to_snapshot => false,
+    }, {
+        insta::assert_snapshot!(name, output);
+    });
+}
+
+#[rstest]
+#[case::single_row(
+    "html_single_row_table",
+    "<table>\n<tr><th>Name</th><th>Value</th></tr>\n</table>"
+)]
+#[case::multi_row(
+    "html_multi_row_table",
+    concat!(
+        "<table>\n",
+        "<tr><th>Name</th><th>Value</th></tr>\n",
+        "<tr><td>Alpha</td><td>1</td></tr>\n",
+        "<tr><td>Beta</td><td>22</td></tr>\n",
+        "</table>",
+    )
+)]
+#[case::nested(
+    "html_nested_table",
+    concat!(
+        "<table>\n",
+        "<tr><th>Outer</th><th>Value</th></tr>\n",
+        "<tr><td><table>\n",
+        "<tr><th>Inner</th></tr>\n",
+        "<tr><td>Nested</td></tr>\n",
+        "</table></td><td>Tail</td></tr>\n",
+        "</table>",
+    )
+)]
+#[case::sectioned(
+    "html_sectioned_table",
+    concat!(
+        "<table>\n",
+        "<thead><tr><th>Name</th><th>Value</th></tr></thead>\n",
+        "<tbody>\n",
+        "<tr><td>Alpha</td><td>1</td></tr>\n",
+        "<tr><td>Beta</td><td>2</td></tr>\n",
+        "</tbody>\n",
+        "</table>",
+    )
+)]
+fn snapshots_html_table_conversion(#[case] name: &str, #[case] input: &str) {
+    snapshot_conversion(name, input);
+}

tests/snapshots/html_multi_row_table.snap

@@ -0,0 +1,8 @@
+---
+source: tests/html_snapshots.rs
+expression: output
+---
+| Name  | Value |
+| ----- | ----- |
+| Alpha | 1     |
+| Beta  | 22    |

tests/snapshots/html_nested_table.snap

@@ -0,0 +1,12 @@
+---
+source: tests/html_snapshots.rs
+expression: output
+---
+| Outer | Value |
+| --- | --- |
+| Inner Nested  | Tail |
+| Inner |
+| Nested |
+| Inner  |
+| ------ |
+| Nested |

tests/snapshots/html_sectioned_table.snap

@@ -0,0 +1,8 @@
+---
+source: tests/html_snapshots.rs
+expression: output
+---
+| Name  | Value |
+| ----- | ----- |
+| Alpha | 1     |
+| Beta  | 2     |

tests/snapshots/html_single_row_table.snap

@@ -0,0 +1,6 @@
+---
+source: tests/html_snapshots.rs
+expression: output
+---
+| Name | Value |
+| ---- | ----- |