LibWeb: Improve parsing and serialization of some font properties #3482
Conversation
This is a weird behaviour specific to `font` - it can reset some properties that it never actually sets. As such, it didn't seem worth adding this concept to the code generator, but just manually stuffing the ShorthandStyleValue with them during parsing.
Anything that's default shouldn't be included.
Without this, getting a property's value from `element.style.foo` would fail if `foo` is a shorthand property which has a longhand that is also a shorthand. For example, `border` expands to `border-width` which expands to `border-top-width`. This is because we used `property()` to get a longhand's value, but this returns nothing if the property is a shorthand. This commit solves that by moving most of get_property_value() into a separate method that returns a StyleProperty instead of a String, and which calls itself recursively for shorthands. Also move the manual shorthand construction out of ResolvedCSSStyleDeclaration so that all CSSStyleDeclarations can use it.
Without this, we'd happily parse `font-variant-caps: small-caps potato` as just `small-caps` and ignore the fact that unused tokens were left over. This fix gets us some WPT subtest passes, and removes the need for a bespoke parsing function for font-variant-caps.
The spec wants these keywords to appear in a particular order when serialized, so let's just put them in that order during parsing. This also fixes a bug where we didn't reject `font-variant-east-asian` that contains `normal` alongside another value. Also, rather than always parsing them as a StyleValueList, parse single values on their own, and then support that in the to_font_variant_foo() methods.
It also needs special handling when reading it from CSSStyleDeclaration.
Now that these are only called from ComputedProperties getters, we can put the code there directly.
|
Changes:
|
| Fail e.style['font-variant'] = "super proportional-width jis83 stacked-fractions tabular-nums oldstyle-nums historical-forms all-small-caps no-contextual no-historical-ligatures no-discretionary-ligatures no-common-ligatures" should set the property value |
There was a problem hiding this comment.
Curious why this regressed, unintentionally I guess?
There was a problem hiding this comment.
It's sensitive to the exact order. The test checks for two valid serializations, which confuses me a bit because the general rule is there should be 1, and it should match the order in the grammar. This parsing-change commit made it fail, but the serialization-change commit that follows made it pass again. It was quite confusing working on it. 😅
| }; | ||
| if (font_style->to_keyword() != Keyword::Normal && font_style->to_keyword() != Keyword::Initial) | ||
| append(font_style->to_string(mode)); | ||
| if (auto variant_string = font_variant->to_string(mode); variant_string != "normal"sv && variant_string != "initial"sv) |
There was a problem hiding this comment.
I believe this part includes too many font-variant properties, since https://drafts.csswg.org/css-fonts/#font-prop mentions:
Values for the font-variant property can also be included but only those supported in CSS 2.1; none of the font-variant values added in CSS Fonts Levels 3 or 4 can be used in the font shorthand
[<font-variant-css2>] = normal || small-caps
Perhaps a FIXME at least?
There was a problem hiding this comment.
Ah good catch! I'll take a look.
A lot of the diff is test imports. I may have gone a little overboard. 😅
Highlights:
font's "implicitly reset" propertiesOtherwise, it's adjustments to
fontandfont-variant*so that we pass more WPT tests. There's certainly more to do there!