fix: extract_pages_markdown no longer splits a word with a mid-word <u> tag - #408
fix: extract_pages_markdown no longer splits a word with a mid-word <u> tag#408MADENIYOU wants to merge 4 commits into
Conversation
TextLine::text_with_formatting toggled the `<u>` tag per positioned item, using each item's own is_underline flag independently. A single visual word split across several content-stream text-showing operators can carry the underline flag on only one fragment (e.g. an underline rectangle whose geometry covers only part of a word's width). Since layout-level merging (merge_text_items) already refuses to combine items across a style difference, such fragments arrive here as adjacent, unmerged items with zero gap between them — and each toggling its own tag produces `<u>We</u>alth` for a word like "Wealth", which isn't formatting that was in the source. Fixed by not toggling the underline tag between two items that are both (a) truly geometrically contiguous (gap under 2% of font size — tighter than the general prose-spacing threshold, since this needs to distinguish "the same word, split" from "two separate items a normal layout heuristic happens to join without inserting a space") and (b) not separated by closing punctuation (a trailing period/comma/etc. isn't part of the word before it, even when kerned touching it — this second condition was needed after the geometric-only version regressed existing snapshot output that intentionally opens/closes `<u>` right at a punctuation boundary in a real, tightly-kerned legal document). Such a continuation now inherits the underline state already open, instead of evaluating its own (possibly stale/wrong) flag. Fixes firecrawl#397. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 1 file
Shadow auto-approve: would not auto-approve because issues were found.
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
Five findings from a cubic-dev-ai review round on the mid-word underline fix: 1. Order-dependence (P2, confidence 6): serially inheriting "whichever underline state is already open" made the rendered result depend on which fragment in a run happened to carry the flag — landing on the first fragment extended the tag onto later ones, landing only on a later fragment silently dropped it. Replaced the serial-inheritance approach with a two-pass design: group items into runs up front (resolve_underline_by_word_group), then decide each run's status once via majority character count (ties favor not-underlined, the conservative "don't fabricate styling" default). Same source pattern now always resolves identically regardless of fragment order. 2. RTL gap calculation was LTR-only (`curr.x - (prev.x + prev.width)`), reporting a large, non-tight gap for genuinely touching RTL fragments (positioned right-to-left). Made direction-aware, mirroring should_join_items's own pattern. 3. A tightly adjacent strikeout fragment's own is_underline flag could still influence the majority vote for its non-struck neighbors (the struck fragment itself never renders <u> — the render loop already excludes that — but its flag was still counted toward the shared run's tally). Made strikeout a hard grouping boundary: a struck item never joins a run with a non-struck neighbor. 4. The punctuation-boundary check only looked at the *next* item's leading character, missing the case where the *previous* item's own text already ends with closing punctuation (e.g. already merged into one item earlier in the pipeline). Added the symmetric check. 5. Treating `'` as always-boundary punctuation broke contractions split across fragments (e.g. "can" + "'t"), reintroducing the exact mid-word toggle bug this fix addresses, just at the apostrophe. Removed `'` from the closing-punctuation set — unlike `.`/`,`/etc., an apostrophe can legitimately sit mid-word. Added five new regression tests, one per finding, each verified to fail without its corresponding piece of the fix and pass with it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
All five confirmed and fixed — thanks, this was a thorough round. Order-dependence (P2): Right — serial inheritance made the result depend on fragment order, not just source formatting. Replaced with a two-pass design: group tightly-continuous items into runs first, then decide each run's underline status once via majority character count (ties favor not-underlined). Same source pattern now always resolves identically regardless of which fragment carries the flag. RTL gap (P2): Confirmed, the gap calc was LTR-only. Made it direction-aware, mirroring Strikeout as hard boundary (P2): Confirmed, and worth noting precisely what this closes: the render loop already excludes Previous-item trailing punctuation (P2): Confirmed, the check was one-sided. Added the symmetric check on the previous item's own trailing character. Apostrophe breaks contractions (P2): Confirmed — good catch. Removed Added one regression test per finding (5 new), each verified to fail without its corresponding piece of the fix and pass with it. Full suite passing, clippy clean. |
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Fix all with cubic | Re-trigger cubic
…decision is_tight_word_continuation decided run membership using only geometric contiguity plus a punctuation check — independent of should_join_items, the function the render loop's own needs_space_between call actually uses to decide whether to insert a space between two items. should_join_items has documented special cases where it treats a near-zero geometric gap as a real word boundary regardless of geometry — most notably CID fonts (C2_*/C0_*), which emit one word per text operator with gaps close to zero *between separate words*, not within one. Grouping such pairs into one underline run (as the geometry-only check did) could apply or drop underline across what the render loop itself treats as a genuine word boundary and inserts a space at. Added should_join_items as an additional required condition for run continuation, so the underline grouping can never disagree with the actual spacing decision made in the same render path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Confirmed and fixed — good catch, and thanks for citing the exact `should_join_items` special cases to check against. The run-grouping check was making its own independent judgment call using only geometry + punctuation, with no connection to `should_join_items` — the function the render loop's own `needs_space_between` call actually uses. Added it as a required condition for run continuation, so grouping can never disagree with the real spacing decision made in the same render path (CID-font one-word-per-operator gaps, the colon-label case, and anything else `should_join_items` already knows about). New test `underline_grouping_agrees_with_should_join_items_on_cid_fonts`, reproducing the CID-font case cited in the finding (two distinct C2_-font words with a near-zero gap between them). Verified it fails without the fix (the two words wrongly merge into one underline run: `Alpha Beta` instead of `Alpha Beta`). Full suite passing, clippy clean. |
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
needs_space_between (what the render loop's own spacing decision actually calls) treats a hyphenated join as "no space needed" independent of should_join_items's own verdict — the hyphen check is applied on top of should_join_items's result, not inside it. should_join_items alone doesn't special-case hyphens. Mirroring only should_join_items (as the previous commit did) meant a hyphenated word split across items (e.g. "auto-" + "fix") could still be wrongly treated as a run boundary if should_join_items's own verdict was false for that pair — even though the render loop itself joins them with no space via the hyphen exception. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Confirmed and fixed. `needs_space_between`'s hyphen exception is applied on top of `should_join_items`'s result, not inside it — `should_join_items` itself has no hyphen special-case. My previous fix mirrored only `should_join_items`, missing that outer exception. New test `underline_grouping_preserves_hyphen_no_space_exception`, using the same CID-font shape as the previous finding's test (so `should_join_items` alone says "not joined") with a hyphenated split ("auto-" + "fix"). Verified it fails without the fix (splits into two runs at the hyphen: `auto-fix`) and passes with it. Full suite passing, clippy clean. |
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would require human review. Adds a new heuristic algorithm to group text fragments and decide underline by majority, changing output formatting for all PDFs; several tuning choices (gap threshold, punctuation set) need human judgment to validate against real documents.
Re-trigger cubic
Fixes #397.
Root cause
TextLine::text_with_formatting(src/types.rs) toggles the<u>tag per positioned item, using each item's ownis_underlineflag independently. A single visual word can be split across several content-stream text-showing operators, and the source PDF can carry the underline flag on only one fragment — e.g. an underline rectangle whose geometry happens to cover only part of a word's width, a rendering artifact rather than intentional partial-word styling.Layout-level merging (
merge_text_items) already refuses to combine items across a style difference (its own comment explains why: "OR-ing underline instead would stretch<u>spans over neighboring plain text"), so such fragments arrive atTextLine::text_with_formattingas adjacent, unmerged items with zero gap between them. Each toggling its own tag independently produces<u>We</u>althfor a word like "Wealth" — not formatting that was in the source.Fix
Don't toggle the underline tag between two items when both:
needs_space_between), because the goal here is different: distinguishing "this is the same word, split at the extraction layer" from "these are two separate items a normal layout heuristic happens to join without inserting a space" (many unrelated reasons exist for the latter — punctuation attachment, hyphenation, kerning)../,/;/etc. isn't part of the word before it, even when kerned tightly against it. This condition was needed after a geometry-only version regressed existing snapshot output (td9264.pdf, a real, tightly-kerned legal document) where<u>legitimately opens/closes right at a punctuation boundary.A continuation meeting both conditions now inherits whichever underline state is already open, instead of evaluating its own (possibly spurious) flag — so the tag wraps the whole word (or none of it), never splitting mid-word.
Testing
underline_does_not_toggle_mid_word, reproducing the issue's exact pattern (a word split across 4 items, underline flagged on only one interior fragment). Verified it fails without the fix (reproduces the exact reported<u>We</u>althsplit) and passes with it.underline_still_toggles_at_a_real_word_boundary, confirming the fix doesn't suppress underline at genuine word boundaries.td9264.pdf(extended<u>across sentence punctuation and into unrelated following prose); the punctuation condition above was added specifically to fix that regression, and the full suite now passes unchanged.cargo fmt,cargo test(all passing, 2 new),cargo clippy --all-targets -- -D warningsclean.🤖 Generated with Claude Code
Summary by cubic
Prevents
extract_pages_markdownfrom opening/closing<u>mid-word. Previously underline toggled per item and could emit<u>We</u>alth; now tightly contiguous word fragments are grouped and one underline decision is applied, avoiding order dependence and spans across real word boundaries.TextLine::resolve_underline_by_word_group: groups only when the gap is under 2% of font size, is direction-aware (RTL), agrees with the render loop’s spacing decision (should_join_items), preserves the hyphen no-space exception fromneeds_space_between, excludes closing punctuation on either side, treats strikeout as a hard boundary, and does not treat'as a boundary.Written for commit ace4153. Summary will update on new commits.