-
-
Notifications
You must be signed in to change notification settings - Fork 75
Update font-family support and font shorthand support #213
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your quick work here! However now I am wondering if the spec or tests also need updates...
https://wpt.fyi/results/css/cssom/font-family-serialization-001.html?label=experimental&label=master&aligned is interesting in particular.
Re: https://wpt.fyi/results/css/cssom/font-family-serialization-001.html Some thoughts from the test results:
It's just my guess, but from the above results, I don't think any browsers have implemented Also note that |
Thanks for the investigation! Upon reading the spec in more detail, I think the test should update https://github.com/web-platform-tests/wpt/blob/master/css/css-fonts/support/font-family-keywords.js to match the list in https://drafts.csswg.org/css-fonts-4/#generic-font-families . I will send a PR for that. (web-platform-tests/wpt#53015) Analysis of the tests:
For our implementation in cssstyle, I think the minimal change is to just update the keyword list to match https://drafts.csswg.org/css-fonts-4/#generic-font-families . (Maybe omit Eventually, we could write completely spec-compliant parsing and serializing code for font-families. This would involve removing the current uppercase assumption and instead using a parser that can recognize:
But this is probably much harder. |
Just to confirm, you will get a result like this: style.font = "medium foo"; // no generic font family
console.log(style.fontFamily); // `foo` It should be okay then? |
Updated the implementation, but omitted |
978cf65
to
d274842
Compare
This appears to be how browsers behave, so yeah, that seems good! |
Fixed font shorthand too. |
Fix #212
Also fixes jsdom/jsdom#3021
And can close #121
Regression test for jsdom jsdom/jsdom#3889