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

Add support for font-variant #2197

Merged
merged 5 commits into from
Dec 17, 2024
Merged

Conversation

jdahlin
Copy link
Contributor

@jdahlin jdahlin commented Nov 6, 2024

This PR adds support for:

The CSS properties are parsed, styled and passed on to layout which ultimately passes them on to harfbuzz which shapes them based OpenType Tags, using
dlig, clig, calt etc

This PR includes font-feature-settings honoring feature precedence

  • Note: font-variant-alternates lacks support for most css functions such as stylistic(feature-name)
  • Note: font-variant-east-asian requires better script shaping support to be actually useful.
  • Note: font-variant-emoji is only parsed/styled, layout/rendering is a todo.

@jdahlin jdahlin requested a review from AtkinsSJ as a code owner November 6, 2024 12:41
@ladybird-bot
Copy link
Collaborator

Hello!

One or more of the commit messages in this PR do not match the Ladybird code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

@jdahlin jdahlin force-pushed the font-variant branch 2 times, most recently from b8f9c1d to 0d502f3 Compare November 6, 2024 16:17
@jdahlin jdahlin changed the title Add support for font-variant Add support for font-variant-* Nov 6, 2024
@jdahlin
Copy link
Contributor Author

jdahlin commented Nov 7, 2024

wpt css/css-fonts before:

Ran 496 tests finished in 169.8 seconds.
• 206 ran as expected. 0 tests skipped.
• 13 tests crashed unexpectedly
• 172 tests failed unexpectedly
• 14 tests timed out unexpectedly
• 94 tests had unexpected subtest results

wpt css/css-fonts after:

Ran 496 tests finished in 184.2 seconds.
• 336 ran as expected. 0 tests skipped.
• 13 tests crashed unexpectedly
• 42 tests failed unexpectedly
• 14 tests timed out unexpectedly
• 94 tests had unexpected subtest results

This fixes at least 130 tests.

A few of the WPT tests have been imported into the testsuite that had to be tweaked to allow custom fonts to be loaded.

@jdahlin jdahlin force-pushed the font-variant branch 7 times, most recently from 915114a to ff0fe3a Compare November 11, 2024 14:40
@jdahlin jdahlin changed the title Add support for font-variant-* Add support for font-variant Nov 11, 2024
Copy link
Member

@AtkinsSJ AtkinsSJ left a comment

Choose a reason for hiding this comment

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

Hi, thanks for making a start on font features! I've mostly stuck to looking at the CSS code as that's my area.

Your final commit looks like it's mostly moving code that was added earlier in the PR. Please apply those changes to the earlier commits instead, it makes the git history much easier to follow.

Libraries/LibWeb/CSS/Parser/Parser.cpp Outdated Show resolved Hide resolved
Libraries/LibWeb/CSS/Parser/Parser.cpp Show resolved Hide resolved
Libraries/LibWeb/CSS/Parser/Parser.cpp Outdated Show resolved Hide resolved
Libraries/LibWeb/CSS/Parser/Parser.cpp Outdated Show resolved Hide resolved
Libraries/LibWeb/CSS/Parser/Parser.cpp Outdated Show resolved Hide resolved
Libraries/LibWeb/CSS/StyleComputer.cpp Outdated Show resolved Hide resolved
Libraries/LibWeb/CSS/StyleComputer.cpp Outdated Show resolved Hide resolved
Libraries/LibWeb/CMakeLists.txt Outdated Show resolved Hide resolved
Libraries/LibWeb/Layout/InlineLevelIterator.h Outdated Show resolved Hide resolved
UI/Headless/Test.cpp Outdated Show resolved Hide resolved
@jdahlin
Copy link
Contributor Author

jdahlin commented Nov 11, 2024

Hi, thanks for making a start on font features! I've mostly stuck to looking at the CSS code as that's my area.

Your final commit looks like it's mostly moving code that was added earlier in the PR. Please apply those changes to the earlier commits instead, it makes the git history much easier to follow.

Thanks for the review! I had to do significant refactoring to support writing to the shorthand property, that why the last commit looks weird. I'll recreate the different commits to avoid shuffling things around unnecessarily and maintain the git history clean.

@jdahlin jdahlin force-pushed the font-variant branch 2 times, most recently from 1097659 to 3c0347b Compare November 11, 2024 22:21
@jdahlin
Copy link
Contributor Author

jdahlin commented Nov 11, 2024

@AtkinsSJ Thanks for the thorough review, it's been really helpful and is improving the PR significantly. I'm still learning quite a bit of C++ while doing this. Two conversations are unresolved as I could not figure out how to make them work the way you suggested. Everything should be solved now!

@jdahlin jdahlin requested a review from AtkinsSJ November 11, 2024 23:39
@jdahlin jdahlin force-pushed the font-variant branch 6 times, most recently from 9f56fd4 to 67c4de1 Compare November 16, 2024 18:20
Libraries/LibWeb/CSS/Parser/Parser.cpp Outdated Show resolved Hide resolved
Libraries/LibWeb/CSS/Parser/Parser.cpp Outdated Show resolved Hide resolved
Libraries/LibWeb/CSS/Parser/Parser.cpp Show resolved Hide resolved
Libraries/LibGfx/Font/FontVariant.h Outdated Show resolved Hide resolved
Libraries/LibGfx/Font/FontVariant.h Outdated Show resolved Hide resolved
Libraries/LibGfx/Font/FontVariant.h Outdated Show resolved Hide resolved
Libraries/LibWeb/CSS/StyleValues/ShorthandStyleValue.cpp Outdated Show resolved Hide resolved
Libraries/LibWeb/Layout/Node.cpp Outdated Show resolved Hide resolved
Libraries/LibGfx/TextLayout.h Show resolved Hide resolved
@jdahlin jdahlin force-pushed the font-variant branch 4 times, most recently from 9538296 to fc381a3 Compare December 5, 2024 00:24
@jdahlin jdahlin requested a review from AtkinsSJ December 5, 2024 00:24
@jdahlin jdahlin force-pushed the font-variant branch 5 times, most recently from 0957ec5 to 078ea86 Compare December 11, 2024 11:09
Copy link
Member

@AtkinsSJ AtkinsSJ left a comment

Choose a reason for hiding this comment

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

Forgot to mention these before! Otherwise I agree with Jelle, overall this is in decent shape and can be merged.

Libraries/LibWeb/CSS/Keywords.json Show resolved Hide resolved
Libraries/LibWeb/CSS/Keywords.json Outdated Show resolved Hide resolved
@gmta gmta enabled auto-merge (rebase) December 17, 2024 17:38
@gmta gmta merged commit 6fd7b9b into LadybirdBrowser:master Dec 17, 2024
8 checks passed
@jdahlin jdahlin deleted the font-variant branch December 17, 2024 21:19
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.

6 participants