Skip to content

Fix character-range highlights and retain debugLineFragments - #26

Open
DrkXo wants to merge 6 commits into
brewkits:mainfrom
DrkXo:fix/get-boxes-for-char-range
Open

DrkXo wants to merge 6 commits into
brewkits:mainfrom
DrkXo:fix/get-boxes-for-char-range

Conversation

@DrkXo

@DrkXo DrkXo commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #17 and replacement for closed PR #19. This updates character-range highlights for text scaling, whitespace, line geometry, and inline atoms, while retaining the requested line-fragment diagnostics with dedicated tests.

Changes

  • Route character-range measurement through _getTextPainter() so highlights respect the active text scaler.
  • Align highlight vertical bounds to rendered line geometry and preserve internal and preformatted leading spaces.
  • Keep highlight rectangles split at inline atoms.
  • Retain debugLineFragments() to report positioned fragments, character ranges, line bounds, ruby text, and ellipsis visibility for laid-out content.

Tests

  • boxes_for_char_range_test.dart: character-range geometry, preformatted whitespace, and an inline-image boundary that asserts two separate rectangles.
  • debug_line_fragments_test.dart: wrapped fragment geometry, character ranges, line metadata, ruby text, ellipsis visibility, and the distinction from debugFragments().

Validation

  • 7 character-range widget tests passed.
  • 6 debug-line-fragment widget tests passed.
  • Targeted Dart analysis passed.

@vietnguyentuan2019 vietnguyentuan2019 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the follow-up! 🙏 This is in good shape. Every point from the #19 review is addressed. I also checked that both strengthened tests actually catch regressions: with mergeGroup++ removed, the image test fails (one 0→208 rect), and with the isPreformatted guard removed, the pre test fails (80 vs 80). I merged this locally with current main (1.12.0). There are no conflicts, the full root+core suite passes (2526), and analyze is clean.

One thing before merge, in the ellipsis part of debugLineFragments():

ellipsisVisibleLength is always null on the fragment that's actually on the line. When layout truncates, it adds a new truncFrag ("visi…") to the line. But it sets ellipsisVisibleLength on the original fragment, which never reaches _lines (render_hyper_box_layout.dart ~L1322–1335). I reproduced it with:

<p>x</p><div style="overflow:hidden;text-overflow:ellipsis;white-space:nowrap">visible head SECRET TAIL</div>

at width 80. The line fragment reports text: "visi…", charStart: 1, charEnd: 6, ellipsisVisibleLength: null. The expected result is 4 visible chars, so charEnd: 5. The … glyph gets counted as source char 5. That causes two problems:

  • The doc comment says consumers "can tell how much of a truncated fragment reached the screen", but they can't.
  • getBoxesForCharRange(5, 6) returns a rect over the … for the hidden char b.

The current test only checks containsKey('ellipsisVisibleLength'), so it can't catch this.

Suggested fix:

  1. In layout, set ellipsisVisibleLength = clippedText.length on truncFrag. Do the same for the ellipsis-only fragment, with 0.
  2. In debugLineFragments() and getBoxesForCharRange(), use fragment.ellipsisVisibleLength ?? text.length as the fragment's char length. getSelectedText already does this.
  3. Add a test with the HTML above that asserts ellipsisVisibleLength == 4, charEnd == 5, and getBoxesForCharRange(5, 6) is empty. The same harness exists in test/review_fixes_v1_3_2_test.dart.

Small stuff (non-blocking):

  • The image test comment mentions commit f5a313b, which no longer exists after the rebase. Could you drop the hash?
  • Could you add a line to the getBoxesForCharRange doc saying offsets are local to this RenderHyperBox? In virtualized/auto mode (>10k chars), each chunk has its own box, and offsets restart at 0.
  • Optional: most of the per-fragment logic is duplicated from getSelectionRects. A shared helper would be nice, but it's fine to leave as a follow-up.

Thanks again, nearly there!

- Set ellipsisVisibleLength on truncFrag and ellipsisFrag during layout
- Use ellipsisVisibleLength in debugLineFragments to exclude ellipsis glyph
- Unify getSelectionRects and getBoxesForCharRange via shared helper
- Document RenderHyperBox coordinate locality in getBoxesForCharRange
- Drop stale commit hash in boxes_for_char_range_test
- Add regression test for truncated text bounds at narrow width

Refs brewkits#19
@DrkXo

DrkXo commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Thanks so much for the thorough review and for catching this! 🙏

All of your points have been addressed in 5a151ff:

  1. Ellipsis in Layout: truncFrag now gets ellipsisVisibleLength = clippedText.length, and the ellipsis-only fragment gets 0.
  2. Bounds & Fragments: debugLineFragments() and getBoxesForCharRange() now respect fragment.ellipsisVisibleLength ?? text.length, ensuring the trailing … glyph isn't counted as a source character.
  3. Regression Test: Added a test using your exact HTML reproduction snippet in review_fixes_v1_3_2_test.dart verifying ellipsisVisibleLength == 4, charEnd == 5, and that getBoxesForCharRange(5, 6) is empty.
  4. Docs & Cleanup:
    • Added the note to getBoxesForCharRange explaining that offsets are local to the RenderHyperBox (restarting at 0 for each chunk in virtualized/auto mode).
    • Dropped the outdated commit hash from the image test comment.
  5. Shared Helper (Bonus): Went ahead and extracted _getFragmentBoxesForRange so both getSelectionRects() and getBoxesForCharRange() share the exact same per-fragment logic without duplication.

Really appreciate your patience and guidance! Thanks again 🙏

@DrkXo

DrkXo commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor Author

Hello @vietnguyentuan2019 ,

While testing CJK EPUB documents containing large unspaced paragraphs (such as Project Gutenberg's Dream of the Red Chamber / 紅樓夢), I noticed a significant line-breaking performance issue in packages/hyper_render_core/lib/src/core/render_hyper_box_layout.dart that leads to severe layout delays.

I investigated the problem and have prepared a potential fix with passing tests on branch gutenberg_conversion (commit 4ae86b7).

Please let me know if you would prefer that I open a separate issue or pull request to discuss this, or if it is convenient to discuss it here. Thank you for your time and guidance.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants