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

fix: Correctly position font baseline and line-height #599

Merged
merged 3 commits into from
Jun 2, 2024

Conversation

tonsky
Copy link
Contributor

@tonsky tonsky commented Mar 6, 2024

This patch changes logic how baseline and line-height are calculated, to match what browsers do and
tools like Figma.

This is especially noticeable on font IBM Plex Sans due to how its metrics are set up. It was less noticeable on Inter, because previous calculations somehow arrived at almost correct numbers.

Implementation notes:

  • useOS2Table is removed, because it’s not what browsers/Figma seem to be using
  • yMax, yMin are not used in text positioning at all
  • lineHeight is calculated before as a fraction of fontSize, so height just recalculates it back

Before:

Screenshot 2024-03-06 at 19 12 12

After:

Screenshot 2024-03-06 at 19 12 35

Background blue/orange text is a static PNG rendered with Figma, black text is rendered with Satori.

Should solve #577

References: https://iamvdo.me/en/blog/css-font-metrics-line-height-and-vertical-align

Screenshot 2024-03-06 at 19 22 47

And https://www.figma.com/blog/line-height-changes/:

Screenshot 2024-03-06 at 19 27 16

Background image (if needed):

https://github.com/vercel/satori/assets/285292/3c8d6a75-cdca-4774-b285-7bd64bae51ee

@tonsky tonsky requested a review from shuding as a code owner March 6, 2024 18:28
Copy link

vercel bot commented Mar 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
satori-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 8, 2024 0:22am

@tonsky
Copy link
Contributor Author

tonsky commented Mar 7, 2024

Update: default line-height is not 1.2, it’s actually ascender + descender + lineGap, so I fixed that too

Demo:

Screenshot 2024-03-07 at 17 31 42

Reference:

Screenshot 2024-03-07 at 17 30 29

@shuding shuding changed the title Correctly position font baseline and line-height fix: Correctly position font baseline and line-height Jun 2, 2024
@shuding
Copy link
Member

shuding commented Jun 2, 2024

Great work, thank you!

@shuding shuding merged commit d1dfcce into vercel:main Jun 2, 2024
4 of 5 checks passed
@Vizards
Copy link

Vizards commented Jun 26, 2024

@shuding Could you please assist in releasing and publishing the bugfixes on the main branch to npm? The latest version on npm is 0.10.13(5 month ago). I found that the action-run corresponding to this Pull Request has failed, and subsequent commits have skipped the Maybe Release step. It may require some manual intervention. Thank you.

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.

3 participants