Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use CSS definition of line height #84

Merged
merged 1 commit into from
Jul 22, 2024

Conversation

nicoburns
Copy link
Contributor

@nicoburns nicoburns commented Jul 20, 2024

Implements CSS style line height computation.

This is now directly on top of main and #76 builds on top of this.

@nicoburns nicoburns requested a review from dfrg July 20, 2024 09:10
@xorgy
Copy link
Member

xorgy commented Jul 20, 2024

I'll take a look at this again after we get InlineBox in. I think that there's some question as to whether this is correct to do at this layer, since it introduces questions about rounding (which is part of the CSS interpretation of line-height).

@nicoburns
Copy link
Contributor Author

I've rebased this to be directly on top of main (so we can potentially land it before #76).

I don't see how we'd do this outside of this layer as it controls the spacing between lines (a key part of layout!). And without this change parley doesn't let the user control that as it computes it from the font metrics (in a slightly different way to this - letting ascent + descent + leading control the line height, rather than letting line_height - (ascent + descent) control the leading).

@xorgy
Copy link
Member

xorgy commented Jul 20, 2024

I took a look and I think it's fine the way it is, except the comment on the line_height property seems to say that it can be a ‘px value’ which is incorrect.

parley/src/layout/line/mod.rs Outdated Show resolved Hide resolved
parley/src/layout/mod.rs Outdated Show resolved Hide resolved
parley/src/resolve/mod.rs Show resolved Hide resolved
parley/src/layout/line/greedy.rs Outdated Show resolved Hide resolved
Let line-height be line_height * font_size with no regard for font metrics.
Font metrics are still used to determine the baseline with the line box.
Copy link
Member

@xorgy xorgy left a comment

Choose a reason for hiding this comment

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

:shipit:

@nicoburns nicoburns added this pull request to the merge queue Jul 22, 2024
Merged via the queue into linebender:main with commit 0f2608c Jul 22, 2024
19 checks passed
@nicoburns nicoburns deleted the css-line-height branch July 22, 2024 21:51
github-merge-queue bot pushed a commit that referenced this pull request Sep 19, 2024
- Builds on top of #84 which
should be merged first to preserve history

## Changes made

- ### Entire unresolved style struct
     - Adds an "unresolved style struct" (`TextStyle`) to `style/mod.rs`
- The new `TreeBuilder` allows you to directly pass an entire style
struct (rather than just "changed styles" relative to a previous node in
the tree) as this is easier for integrating with Stylo which already
resolves inherited styles. So I have added an "unresolved" version of
the style struct to allow this API to still plug into the other parts of
Parley's style resolution functionality.
- Adds a corresponding`resolve_entire_style_set` method to
`LayoutContext` to convert `TextStyle` into `ResolvedStyle`.
- ### Moved code
- Moves the `RangedBuilder` from `context.rs` to a new `builder.rs`
module (as there are now two builders, justifying a separate module).
- Extract most of `RangedBuilder::build_into` into a standalone
`build_into_layout` function (`builder.rs`) that can be shared between
the ranged and tree builders.
- Moves the `RangedStyle` and `RangedProperty` types from
`resolve/range.rs` to `resolve/mod.rs`. These types are shared between
the `RangedBuilder` and the `TreeBuilder`.
- ### Tree builder
- Adds a `TreeBuilder` (also to `builder.rs`). This mostly delegates to
`TreeStyleBuilder`
- Add a `TreeStyleBuilder` (`resolve/tree.rs`). This is the vast
majority of the new code
- The `TreeStyleBuilder` implements HTML-style whitespace collapsing
(opt-in). This probably ought to become a style rather than being a flag
on the style builder.
- Updated swash example to optionally use the tree builder (depending on
command line opt)
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