Skip to content

Handle line heights changing within atoms - #864

Merged
tomcur merged 4 commits into
linebender:mainfrom
tomcur:line-heights-within-run-take-two
Oct 1, 2026
Merged

tomcur merged 4 commits into
linebender:mainfrom
tomcur:line-heights-within-run-take-two

Conversation

@tomcur

@tomcur tomcur commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

LLM Contributions: Investigation, initial implementation, review.

On top of #863. If the line height changes within an atom, we add the spans of all the atom's characters to the line. I believe this matches Blink; but note we do still diverge a bit from Blink regarding the "strut" and accounting styles per-character instead of per-span.

Supercedes #761, #828, #839.

Without this change, the test renders as follows.

line_height_change_inside_ligature-0

Performance

Together with #863, this benches as follows (but there may be some placement noise again).

Line Breaking - arabic 8000 characters, narrow     [ 221.7 us ... 203.0 us ]      -8.44%*
Line Breaking - arabic 8000 characters, wider      [ 121.7 us ... 109.7 us ]      -9.84%*
Line Breaking - arabic 8000 characters, unwrapped  [  92.1 us ...  83.4 us ]      -9.44%*
Line Breaking - latin 8000 characters, narrow      [ 176.7 us ... 168.0 us ]      -4.96%*
Line Breaking - latin 8000 characters, wider       [  86.3 us ...  80.5 us ]      -6.78%*
Line Breaking - latin 8000 characters, unwrapped   [  54.8 us ...  52.8 us ]      -3.77%*
Line Breaking - japanese 8000 characters, narrow   [ 224.7 us ... 213.7 us ]      -4.88%*
Line Breaking - japanese 8000 characters, wider    [ 149.8 us ... 143.3 us ]      -4.35%*
Line Breaking - japanese 8000 characters, unwrapped [ 127.4 us ... 121.2 us ]      -4.89%*
Line Breaking - arabic 8000 characters, wrapped + spacing [ 130.7 us ... 116.8 us ]     -10.67%*
Line Breaking - latin 8000 characters, wrapped + spacing [  94.0 us ...  88.2 us ]      -6.16%*
Line Breaking - japanese 8000 characters, wrapped + spacing [ 155.5 us ... 147.7 us ]      -5.01%*
Page Break - arabic 8000 characters                [  98.2 us ...  88.0 us ]     -10.38%*
Page Break - latin 8000 characters                 [  63.1 us ...  60.2 us ]      -4.58%*
Page Break - japanese 8000 characters              [ 141.0 us ... 131.6 us ]      -6.68%*

Changelog

Fixed

  • All line height changes within a run now contribute to the line box.

Comment on lines +371 to +373
for character in characters.iter().skip(1) {
self.add_style(character.style_index, &data.style_metrics, contributed);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: this could probably skip consecutive chars with the same style?

@tomcur tomcur Oct 1, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It could, but I'm not sure it would save a lot except for very pathological cases. The add_style call already does a scan to check if the style has been added previously. So then, three things have to line up for this to matter: a lot of atoms with more than one character, actually having styles crossing a grapheme or a ligature across styles, and having a lot of styles.

What might be useful is to check per-atom whether it has mixed styles, but the version I tried which did that during line breaking regresses it by 4%.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I ended up adding a harmless-enough guard that should catch most of those pathological cases, by checking whether the style is different from the atom's first style.

Comment thread parley/src/layout/line_break.rs Outdated
self.add_style(character.style_index, &data.style_metrics, contributed);
}
// Take this path for the run's next atom too, which could also have mixed styles.
self.last_text = (usize::MAX, 0);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could we document the use of sentinel values here?

@tomcur tomcur Oct 1, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point; I've instead simplified it a bit, by checking RunData::has_mixed_style_atoms directly. (There's one remaining use of (usize::MAX, 0) not introduced by this PR, in the Default impl, but I think that one is clear enough as-is.)

@tomcur
tomcur force-pushed the line-heights-within-run-take-two branch from 3e7861f to ed7f002 Compare October 1, 2026 10:51
@tomcur
tomcur added this pull request to the merge queue Oct 1, 2026
@tomcur
tomcur removed this pull request from the merge queue due to a manual request Oct 1, 2026
@tomcur
tomcur enabled auto-merge October 1, 2026 11:20
@tomcur
tomcur added this pull request to the merge queue Oct 1, 2026
Merged via the queue into linebender:main with commit 5794c74 Oct 1, 2026
24 checks passed
@tomcur
tomcur deleted the line-heights-within-run-take-two branch October 1, 2026 11:27
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