Add absolute and metrics-relative line height styles #362

valadaptive · 2025-05-20T11:29:22Z

This implements @dfrg's proposal, since most people agreed it was the way forward.

If you don't want FontSizeRelative, I can remove it--MetricsRelative(1.0) is a sensible default that works at all font sizes. But some people leaned towards keeping FontSizeRelative.

The default line height has been changed from FontSizeRelative(1.2) (what I believe CSS uses) to MetricsRelative(1.0). This means a lot of the snapshot tests will change as well.

nicoburns

This generally looks good to me. A few minor points in comments, none of which I'd consider blocking.

My main question would be why so many (all?) of the snapshot tests have changed. Has the default line-height changed? In theory it seems like nothing here should change existing snapshots as we are only adding additional line-height options.

parley/src/style/mod.rs

examples/swash_render/src/main.rs

valadaptive · 2025-05-20T12:04:58Z

Has the default line-height changed?

Yes, Chad suggested that in the original proposal. I can revert that if it touches too many tests.

nicoburns · 2025-05-20T12:08:59Z

Has the default line-height changed?

Yes, Chad suggested that in the original proposal. I can revert that if it touches too many tests.

I see. I don't feel particularly strongly about what the default is (I'm not planning to use the default anyway!). But could you document what the new default is and the previous default was (in the PR description), and perhaps add a note that this is why the snapshots have changed?

valadaptive · 2025-05-20T12:13:36Z

Done.

nicoburns

Apologies, I have a couple more changes to request:

A changelog entry:
- Should note the introduction of the Lineheight enum
- Should note the change of default line-height
Suggestions in comments are optional but recommended.

nicoburns · 2025-05-20T12:30:29Z

parley/src/resolve/mod.rs

+            LineHeight(value) => match (self.line_height, *value) {
+                (crate::LineHeight::MetricsRelative(a), crate::LineHeight::MetricsRelative(b))
+                | (
+                    crate::LineHeight::FontSizeRelative(a),
+                    crate::LineHeight::FontSizeRelative(b),
+                )
+                | (crate::LineHeight::Absolute(a), crate::LineHeight::Absolute(b)) => {
+                    nearly_eq(a, b)
+                }
+                _ => false,


A couple of notes on this one:

Consider importing crate::LineHeight at the top of the file (partly for consistency with surrounding code, partly because I think the match would format better like that)

Consider making this a method on LineHeight.

nicoburns · 2025-05-20T12:31:00Z

parley/src/resolve/mod.rs

+            line_height: match self.line_height {
+                crate::LineHeight::MetricsRelative(value) => {
+                    crate::LayoutLineHeight::MetricsRelative(value)
+                }
+                crate::LineHeight::FontSizeRelative(value) => {
+                    crate::LayoutLineHeight::Absolute(value * self.font_size)
+                }
+                crate::LineHeight::Absolute(value) => crate::LayoutLineHeight::Absolute(value),
+            },


Same as nearly_eq above (considering use crate::LineHeight + make this a method).

xStrom

Great to see absolute line height support moving forward!

I have a few inline comments.

On a more general note, this PR really shows that using default line height for snapshot tests is a bad idea. This type of PR would benefit greatly from zero changed snapshots. Anyway, the actual line height tests did manually specify a line height and still work, so the validity is just hidden in noise. I would prefer setting the old default line height as the default choice for tests to reduce the snapshot churn. However it's not a blocker.

parley/src/layout/mod.rs

parley/src/style/mod.rs

xStrom · 2025-05-20T13:49:16Z

parley/src/layout/line/greedy.rs

@@ -523,7 +523,6 @@ impl<'a, B: Brush> BreakLines<'a, B> {
                        // Compute the run's vertical metrics
                        line.metrics.ascent = line.metrics.ascent.max(run.metrics.ascent);
                        line.metrics.descent = line.metrics.descent.max(run.metrics.descent);
-                        line.metrics.leading = line.metrics.leading.max(run.metrics.leading);


Why aren't we calculating the line's leading any more? Unclear to me.

Ah, I probably should've mentioned this in the PR itself so more people could take a look at it. I believe that this is dead code now, since leading is always calculated here, and both of the removed leading-calculation lines should flow into that code unconditionally.

Thanks and I think you're correct. Good catch!

xStrom

This moves us forward nicely, thanks for your continued efforts!

dfrg · 2025-05-20T14:35:06Z

This moves us forward nicely, thanks for your continued efforts!

+1, much appreciated!

valadaptive · 2025-05-21T01:50:42Z

Updated the tests to use a default line height of FontSizeRelative(1.0).

One last (semi-related) thing: it seems the default line height used to be 1.2 if you used the TreeStyleBuilder, but 1.0 if you used the RangedStyleBuilder. This is seemingly due to a discrepancy between the Default impl for TextStyle and the one for ResolvedStyle.

In the future, we should probably find a way to avoid introducing further discrepancies of that sort, but for now I've listed it in the changelog. Only a couple tests used TreeStyleBuilder, and their snapshots now differ.

nicoburns · 2025-05-21T09:57:24Z

Only a couple tests used TreeStyleBuilder, and their snapshots now differ.

Perhaps we ought to set the line-height explicitly in those specific tests?

DJMcNab

Code changes look really good. The only actionable changes are some docs/examples suggestions, which should be straightforward to implement.

examples/swash_render/src/main.rs

examples/vello_editor/src/text.rs

parley/src/layout/mod.rs

parley/src/lib.rs

parley/src/style/mod.rs

parley/src/tests/test_lines.rs

parley/src/tests/utils/env.rs

parley/tests/snapshots/lines_line_height_size_relative-0.png

parley/src/layout/mod.rs

parley/src/style/mod.rs

xStrom

It looks like some commits got lost when you force pushed without pulling.

The previous situation of the different builders having different line height defaults is crazy, glad that's no longer the case.

Great to also see less snapshot churn here. We might want to eventually change the test default, but then it can happen independently of any code changes.

CHANGELOG.md

parley/src/layout/mod.rs

parley/src/style/mod.rs

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

Co-authored-by: Kaur Kuut <strom@nevermore.ee>

valadaptive · 2025-05-22T00:03:29Z

Whoops--I need to get into the habit of using --force-with-lease instead of --force.

I've implemented most of the suggestions, and expanded the doc comments for the LineHeight type quite a bit. I've also implemented Default for LineHeight.

I'd rather update the few tests whose snapshots changed; I can visually confirm they still pass, and it's not a big deal since there's not as much snapshot churn as updating the entire test suite would be.

xorgy · 2025-05-22T04:24:10Z

parley/tests/snapshots/leading_whitespace-preserve.png

I feel like since this changes the default, it's okay to update the test to its prior equivalent rather than just letting it change arbitrarily for this.

I disagree here - this PR is fixing the bug that TreeBuilder had a different default, and so is updating the snapshots which used that.

DJMcNab

A few minor docs things, none of which are blocking.
I have tested this out in Vello Editor, and the new default works well.

Thanks!

parley/src/resolve/mod.rs

parley/src/style/mod.rs

DJMcNab · 2025-05-22T08:41:17Z

parley/tests/snapshots/leading_whitespace-preserve.png

I disagree here - this PR is fixing the bug that TreeBuilder had a different default, and so is updating the snapshots which used that.

xStrom

Excellent work, thanks!

xStrom · 2025-05-22T08:49:32Z

parley/src/style/mod.rs

+impl Default for LineHeight {
+    fn default() -> Self {
+        Self::MetricsRelative(1.0)
+    }
+}


parley/src/resolve/mod.rs

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

valadaptive · 2025-05-24T02:48:18Z

I think I've addressed all the review comments (fingers crossed). Apologies if I've missed any.

The two builders (ranged/tree) use different sources of truth to achieve a default style. This is already a bad idea, as evidenced by them having a [different default line height](#362 (comment)) until very recently. It gets even worse, because the ranged builder uses `ResolvedStyle::default()` which means it has a dummy font and doesn't respect scale. This PR makes both builders use a single source of truth - a `TextStyle` which then gets resolved. This means that we can confidently change our default style in only one place - the `TextStyle::default` method. Those default values then also get properly resolved according to available fonts, scale, etc. Note that I didn't change the public API on purpose. I do think that being able to immediately provide a custom root style to the ranged builder, like it is possible with the tree builder, is probably worth doing. However, I want to do any potential public API changes as a focused follow-up PR. The `PlainEditor::ime_cursor_area` method needed a slight modification as well. It was depending on `ResolvedStyle::default()` for a font size based calculation. That did not respect scale, so I fixed it by just keeping track of the font size that was provided to the `PlainEditor` constructor. I replaced the custom `ResolvedStyle::default()` implementation with a simple derive. The only remaining non-derivable part was setting the font size to 16. Given that it doesn't respect scale, that custom implementation just provides a false sense of utility. A zero font size is perfectly fine for a state clearing default, which is how we're still using it. I also added a test to ensure that the default styles remain in sync between the different builders.

valadaptive added 5 commits May 20, 2025 06:06

Allow specifying absolute line height

6fa2991

impl From<LineHeight> for StyleProperty

55b9d6a

Add metrics-relative line height

da69621

Add tests for line height styles

a8ab21c

Make MetricsRelative the default line height

bc066b5

valadaptive requested review from waywardmonkeys and xorgy May 20, 2025 11:29

nicoburns reviewed May 20, 2025

View reviewed changes

parley/src/style/mod.rs Outdated Show resolved Hide resolved

examples/swash_render/src/main.rs Outdated Show resolved Hide resolved

nicoburns requested changes May 20, 2025

View reviewed changes

nicoburns approved these changes May 20, 2025

View reviewed changes

valadaptive force-pushed the line-height-styles branch from d7dfe8b to fc13ebb Compare May 20, 2025 12:56

xStrom reviewed May 20, 2025

View reviewed changes

xStrom approved these changes May 20, 2025

View reviewed changes

Rename SizeRelative -> FontSizeRelative

832efb4

valadaptive force-pushed the line-height-styles branch 2 times, most recently from e25ca63 to dbb2d58 Compare May 21, 2025 01:37

valadaptive added 4 commits May 20, 2025 21:38

Specify font-size-relative line height in tests

971a22f

Simplify examples

c66e8b7

Move some stuff to methods on LineHeight

c04f1d7

Add line height stuff to changelog

c48e094

valadaptive force-pushed the line-height-styles branch from dbb2d58 to c48e094 Compare May 21, 2025 01:38

valadaptive requested a review from xStrom May 21, 2025 01:50

DJMcNab approved these changes May 21, 2025

View reviewed changes

xStrom reviewed May 21, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

parley/src/layout/mod.rs Outdated Show resolved Hide resolved

parley/src/style/mod.rs Outdated Show resolved Hide resolved

valadaptive and others added 9 commits May 21, 2025 19:26

Remove line height setting from some examples

70c20c1

Improve LineHeight doc comments

a8e385b

impl Default for LineHeight

02d9ff6

Use LineHeight::Absolute for test_lines

2dc8969

"Resolved" -> "Partially resolved" line height

ef143a5

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

Explain line height style in test env

5a7888f

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

Update changelog sentence formatting

13d9058

Co-authored-by: Kaur Kuut <strom@nevermore.ee>

Make LayoutLineHeight Copy

51300aa

Co-authored-by: Kaur Kuut <strom@nevermore.ee>

Rename scale param -> font_size

0d77845

Co-authored-by: Kaur Kuut <strom@nevermore.ee>

valadaptive requested review from xStrom and DJMcNab May 22, 2025 00:03

xorgy reviewed May 22, 2025

View reviewed changes

DJMcNab approved these changes May 22, 2025

View reviewed changes

xStrom approved these changes May 22, 2025

View reviewed changes

xStrom reviewed May 22, 2025

View reviewed changes

parley/src/resolve/mod.rs Show resolved Hide resolved

xStrom and others added 3 commits May 22, 2025 14:33

Fix line height scale.

1e4b186

Stylistic changes

8360805

Comment LineHeight::scale

fb46c60

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

valadaptive added this pull request to the merge queue May 24, 2025

Merged via the queue into linebender:main with commit 3a501b8 May 24, 2025
24 checks passed

valadaptive deleted the line-height-styles branch May 24, 2025 02:51

This was referenced May 26, 2025

Use the same default style for both builders. #368

Merged

Allow specifying line-height in absolute units more conveniently #320

Closed

xStrom added this to the 0.5 Release milestone Jun 1, 2025

Add absolute and metrics-relative line height styles #362

Add absolute and metrics-relative line height styles #362

Uh oh!

Conversation

valadaptive commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicoburns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

valadaptive commented May 20, 2025

Uh oh!

nicoburns commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

valadaptive commented May 20, 2025

Uh oh!

nicoburns left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xStrom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xStrom left a comment

Choose a reason for hiding this comment

Uh oh!

dfrg commented May 20, 2025

Uh oh!

valadaptive commented May 21, 2025

Uh oh!

nicoburns commented May 21, 2025

Uh oh!

DJMcNab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xStrom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

valadaptive commented May 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DJMcNab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valadaptive commented May 20, 2025 •

edited

Loading

nicoburns commented May 20, 2025 •

edited

Loading