Wrong bbox in location for vertical font#735
Conversation
📝 WalkthroughWalkthrough
ChangesVertical text flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@wcag-validation/src/main/java/org/verapdf/gf/model/factory/chunks/TextPieces.java`:
- Around line 31-53: Update TextPieceComparatorH and TextPieceComparatorV to add
a monotonic insertion-order tiebreaker after the existing center/end
comparisons, and assign that ordinal before textPieces.add in TextPieces.add.
Ensure distinct TextPiece instances with identical geometry compare nonzero so
every added piece is retained and indexing remains consistent.
- Around line 88-117: Update TextPieces.addSpaces to calculate the gap
direction-aware, handling descending coordinates produced by
TextPieceComparatorV so spaces are inserted when the absolute or appropriately
oriented distance between adjacent pieces exceeds threshold. Preserve the
existing ascending-order behavior and add gaps only when the directional
distance is positive and above the threshold.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17412848-901d-41c1-828a-cfc8ab1f0b43
📒 Files selected for processing (3)
wcag-validation/src/main/java/org/verapdf/gf/model/factory/chunks/ChunkParser.javawcag-validation/src/main/java/org/verapdf/gf/model/factory/chunks/TextChunksHelper.javawcag-validation/src/main/java/org/verapdf/gf/model/factory/chunks/TextPieces.java
| private final Comparator<? super Double> comparator; | ||
| private final SortedSet<TextPiece> textPieces; | ||
| public final boolean isVertical; | ||
| private int currentIndex; | ||
| private double current; | ||
|
|
||
| TextPieces(boolean isVertical) { | ||
| this.isVertical = isVertical; | ||
| this.comparator = isVertical | ||
| ? (p1, p2) -> -Double.compare(p1, p2) | ||
| : (p1, p2) -> Double.compare(p1, p2); | ||
| textPieces = isVertical | ||
| ? new TreeSet<>(new TextPieceComparatorV()) | ||
| : new TreeSet<>(new TextPieceComparatorH()); | ||
| currentIndex = 0; | ||
| current = 0; | ||
| } | ||
|
|
||
| public void add(TextPiece textPiece) { | ||
| textPieces.add(textPiece); | ||
| textPiece.startIndex = currentIndex; | ||
| currentIndex += textPiece.value.length(); | ||
| currentX = textPiece.endX; | ||
| current = textPiece.end; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
TextPiece equality collapses on ties → silent character loss in the TreeSet.
textPieces is a TreeSet<TextPiece> ordered solely by TextPieceComparatorH/V (center, then end). TreeSet treats elements that compare as 0 as duplicates and silently drops the second add() (Java's contract: ordering must be equals-consistent for Set semantics). Two distinct glyphs with identical start/end — e.g. consecutive zero-width glyphs (the widthX/widthY = 0.0 fallback in ChunkParser.parseString when metrics are missing, or repeated no-unicode replacements at the same cursor position) — will have identical center and end, so the second TextPiece is silently discarded from textPieces while add() still advances currentIndex/current as if it were stored. The result: missing characters in getValue(), and inconsistent getSymbolEnds()/getStreamInfos() indices.
Add a monotonic tiebreaker to both comparators so structurally-identical pieces are never treated as duplicates.
🐛 Proposed fix
public static class TextPiece {
private final String value;
private final double start;
private final double center;
private final double end;
+ private static final java.util.concurrent.atomic.AtomicLong ORDER_GENERATOR = new java.util.concurrent.atomic.AtomicLong();
+ private final long insertionOrder;
private Integer startIndex;
public TextPiece(String value, double start, double end) {
this.value = value;
this.start = start;
this.center = (start + end) / 2;
this.end = end;
+ this.insertionOrder = ORDER_GENERATOR.getAndIncrement();
}
}
public static class TextPieceComparatorH implements Comparator<TextPiece> {
`@Override`
public int compare(TextPiece textPiece1, TextPiece textPiece2){
int res = Double.compare(textPiece1.center, textPiece2.center);
if (res != 0) {
return res;
}
- return Double.compare(textPiece1.end, textPiece2.end);
+ res = Double.compare(textPiece1.end, textPiece2.end);
+ return res != 0 ? res : Long.compare(textPiece1.insertionOrder, textPiece2.insertionOrder);
}
}
public static class TextPieceComparatorV implements Comparator<TextPiece> {
`@Override`
public int compare(TextPiece textPiece1, TextPiece textPiece2){
int res = -Double.compare(textPiece1.center, textPiece2.center);
if (res != 0) {
return res;
}
- return -Double.compare(textPiece1.end, textPiece2.end);
+ res = -Double.compare(textPiece1.end, textPiece2.end);
+ return res != 0 ? res : Long.compare(textPiece1.insertionOrder, textPiece2.insertionOrder);
}
}Also applies to: 143-176
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@wcag-validation/src/main/java/org/verapdf/gf/model/factory/chunks/TextPieces.java`
around lines 31 - 53, Update TextPieceComparatorH and TextPieceComparatorV to
add a monotonic insertion-order tiebreaker after the existing center/end
comparisons, and assign that ordinal before textPieces.add in TextPieces.add.
Ensure distinct TextPiece instances with identical geometry compare nonzero so
every added piece is retained and indexing remains consistent.
| public List<Double> getSymbolEnds() { | ||
| List<Double> ends = new ArrayList<>(); | ||
| double startX = getStartX(); | ||
| double start = getStart(); | ||
| ends.add(0.0d); | ||
| for (TextPiece textPiece : textPieces) { | ||
| TextChunksHelper.updateSymbolEnds(ends, textPiece.endX - textPiece.startX, textPiece.startX - startX, | ||
| TextChunksHelper.updateSymbolEnds(ends, textPiece.end - textPiece.start, textPiece.start - start, | ||
| textPiece.value != null ? textPiece.value.length() : 0); | ||
| } | ||
| return ends; | ||
| } | ||
|
|
||
| public void addSpaces(double threshold) { | ||
| List<TextPiece> spaces = new ArrayList<>(); | ||
| Iterator<TextPiece> validation = textPieces.iterator(); | ||
| if (!validation.hasNext()) { | ||
| return; | ||
| } | ||
| TextPiece prev = validation.next(); | ||
| double previousEnd = prev.getEndX(); | ||
|
|
||
| while (validation.hasNext()) { | ||
| TextPiece piece = validation.next(); | ||
| double currentStart = piece.getStartX(); | ||
| if (currentStart - previousEnd > threshold) { | ||
| spaces.add(new TextPieces.TextPiece(" ", previousEnd, currentStart)); | ||
| } | ||
| previousEnd = piece.getEndX(); | ||
| } | ||
| textPieces.addAll(spaces); | ||
| } | ||
| public void addSpaces(double threshold) { | ||
| List<TextPiece> spaces = new ArrayList<>(); | ||
| Iterator<TextPiece> validation = textPieces.iterator(); | ||
| if (!validation.hasNext()) { | ||
| return; | ||
| } | ||
| TextPiece prev = validation.next(); | ||
| double previousEnd = prev.end; | ||
|
|
||
| while (validation.hasNext()) { | ||
| TextPiece piece = validation.next(); | ||
| double currentStart = piece.start; | ||
| if (currentStart - previousEnd > threshold) { | ||
| spaces.add(new TextPieces.TextPiece(" ", previousEnd, currentStart)); | ||
| } | ||
| previousEnd = piece.end; | ||
| } | ||
| textPieces.addAll(spaces); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and inspect their structure first.
git ls-files | rg 'wcag-validation/src/main/java/org/verapdf/gf/model/factory/chunks/(TextPieces|TextChunk|TextChunksHelper)\.java$|TextPieceComparatorV|TextPieceComparatorH'
printf '\n--- TextPieces outline ---\n'
ast-grep outline wcag-validation/src/main/java/org/verapdf/gf/model/factory/chunks/TextPieces.java --view expanded || true
printf '\n--- TextChunksHelper outline ---\n'
ast-grep outline wcag-validation/src/main/java/org/verapdf/gf/model/factory/chunks/TextChunksHelper.java --view expanded || true
printf '\n--- TextChunk outline ---\n'
ast-grep outline wcag-validation/src/main/java/org/verapdf/gf/model/factory/chunks/TextChunk.java --view expanded || true
printf '\n--- TextPiece comparators search ---\n'
rg -n "TextPieceComparator[HV]|ComparatorV|ComparatorH|compare\\(" wcag-validation/src/main/java/org/verapdf/gf/model/factory/chunks -g '*.java'
printf '\n--- TextPieces relevant lines ---\n'
sed -n '1,220p' wcag-validation/src/main/java/org/verapdf/gf/model/factory/chunks/TextPieces.java
printf '\n--- TextChunksHelper relevant lines ---\n'
sed -n '1,260p' wcag-validation/src/main/java/org/verapdf/gf/model/factory/chunks/TextChunksHelper.java
printf '\n--- TextChunk relevant lines ---\n'
sed -n '1,260p' wcag-validation/src/main/java/org/verapdf/gf/model/factory/chunks/TextChunk.javaRepository: veraPDF/veraPDF-validation
Length of output: 19061
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- usages of TextPieces methods ---\n'
rg -n "getSymbolEnds\\(|addSpaces\\(|new TextPieces\\(" wcag-validation/src/main/java -g '*.java'
printf '\n--- search for symbol-end adjustment / normalization ---\n'
rg -n "adjustSymbolEndsToBoundingBox|updateSymbolEnds|getSymbolEnds|symbolEnds" wcag-validation/src/main/java -g '*.java'
printf '\n--- files related to TextChunk/TextPieces consumers ---\n'
rg -n "class .*TextChunk|TextPieces" wcag-validation/src/main/java/org/verapdf/gf/model/factory -g '*.java'
printf '\n--- locate any TextChunk-like files ---\n'
find wcag-validation/src/main/java/org/verapdf/gf/model/factory -name '*TextChunk*.java' -o -name '*Chunk*.java' | sort
printf '\n--- read the consumers around TextPieces references ---\n'
for f in $(rg -l "TextPieces" wcag-validation/src/main/java/org/verapdf/gf/model/factory -g '*.java'); do
echo "### $f"
sed -n '1,260p' "$f"
echo
doneRepository: veraPDF/veraPDF-validation
Length of output: 20389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- ChunkParser around parseTextShowArgument / parseString / symbol ends ---\n'
sed -n '820,980p' wcag-validation/src/main/java/org/verapdf/gf/model/factory/chunks/ChunkParser.java
printf '\n--- search for adjustSymbolEndsToBoundingBox definition and TextChunk type ---\n'
rg -n "adjustSymbolEndsToBoundingBox\\(|getSymbolEnds\\(" wcag-validation/src/main/java -g '*.java'
printf '\n--- search for vertical spacing / text direction utilities ---\n'
rg -n "isVertical|TEXT_LINE_SPACE_RATIO|addSpaces\\(" wcag-validation/src/main/java -g '*.java'Repository: veraPDF/veraPDF-validation
Length of output: 11374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- find TextChunk implementation ---\n'
fd -a 'TextChunk.java' . || true
fd -a 'TextChunk*.java' . || true
printf '\n--- search for adjustSymbolEndsToBoundingBox in repo ---\n'
rg -n "adjustSymbolEndsToBoundingBox" . -g '*.java'
printf '\n--- search for symbolEnds fields/getters/setters in impl chunk classes ---\n'
rg -n "symbolEnds|getSymbolEnds\\(|setSymbolEnds\\(" wcag-validation/src/main/java -g '*.java'
printf '\n--- inspect TextSerializer ---\n'
sed -n '1,180p' wcag-validation/src/main/java/org/verapdf/gf/model/impl/serializer/TextSerializer.javaRepository: veraPDF/veraPDF-validation
Length of output: 4590
🌐 Web query:
"adjustSymbolEndsToBoundingBox symbolEnds TextChunk vertical text veraPDF"
💡 Result:
The method adjustSymbolEndsToBoundingBox is a component of the veraPDF WCAG algorithms library [1]. It is responsible for adjusting the calculated ends of symbols (text characters or glyphs) to align with their physical bounding box dimensions during the text extraction and analysis process [1]. In the context of the veraPDF library, text extraction relies on processing TextChunks—representative segments of text extracted from the PDF structure [2][3][4]. Because PDF files can encode text in varied ways (sometimes using positioning offsets rather than explicit spaces, or with complex orientation vectors), the library must perform geometric calculations to ensure extracted text accurately reflects its visual layout [5][6]. The adjustSymbolEndsToBoundingBox method is specifically designed to handle potential discrepancies between internal text position metrics and physical bounding boxes [1]. When processing text—including vertical text—the library calculates the boundaries of symbols. If the coordinate math or multipliers used in these calculations are incorrect, it can lead to precision errors (such as NaN results) or incorrect text merging [2][1]. This method helps ensure that when the software evaluates the spatial relationship between chunks, the boundaries used for decision-making (e.g., determining whether two chunks are neighbors or if a space should be inserted) are correctly mapped to the physical geometry of the characters [2][6]. If you are encountering issues with vertical text or text chunking in veraPDF, it is often due to the library's reliance on specific geometry-based thresholds (like the TEXT_LINE_SPACE_RATIO) to interpret the PDF content [6]. Adjustments within this logic or the underlying bounding box calculations are typically managed within the veraPDF-wcag-algs repository [4][1].
Citations:
- 1: https://github.com/veraPDF/veraPDF-wcag-algs/actions/runs/20170893272
- 2: fix: merge areNeighborsTextChunks() bounding box check to main opendataloader-project/opendataloader-pdf#232
- 3: fix(test): add StreamInfo to test TextChunks for verapdf 1.31.8 compat opendataloader-project/opendataloader-pdf#296
- 4: Add isItalic(), isNotDefaultFontColor(), getRoundedFontWeight() methods to TextChunk veraPDF-wcag-algs#382
- 5: https://github.com/sunrenjie/itext/blob/master/itext/src/main/java/com/itextpdf/text/pdf/parser/LocationTextExtractionStrategy.java
- 6: Make the word spacing threshold (TEXT_LINE_SPACE_RATIO) runtime-settable veraPDF-library#1608
addSpaces() needs a direction-aware gap check for vertical text. With TextPieceComparatorV, pieces are visited in descending coordinate order, so currentStart - previousEnd > threshold never becomes true for vertical runs and spaces are skipped between glyphs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@wcag-validation/src/main/java/org/verapdf/gf/model/factory/chunks/TextPieces.java`
around lines 88 - 117, Update TextPieces.addSpaces to calculate the gap
direction-aware, handling descending coordinates produced by
TextPieceComparatorV so spaces are inserted when the absolute or appropriately
oriented distance between adjacent pieces exceeds threshold. Preserve the
existing ascending-order behavior and add gaps only when the directional
distance is positive and above the threshold.
1 participant
Closes: https://trello.com/c/SCBNY4Sg
pdf.js PR: veraPDF/pdf.js#30
veraPDF-parser PR: veraPDF/veraPDF-parser#708
Summary by CodeRabbit