Bug 251911 - [web-animations] line-height should not transition from default value to a number
Summary: [web-animations] line-height should not transition from default value to a nu...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2023-02-08 02:27 PST by Antoine Quint
Modified: 2023-02-08 12:34 PST (History)
3 users (show)

See Also:


Attachments
Reduction (424 bytes, text/html)
2023-02-08 02:27 PST, Antoine Quint
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2023-02-08 02:27:27 PST
Created attachment 464906 [details]
Reduction

We incorrectly interpolate the "line-height" property when it animates from its initial value (normal) to a number. This was reported in https://stackoverflow.com/questions/75151629/css-transitions-in-safari/.
Comment 1 Antoine Quint 2023-02-08 02:27:47 PST
rdar://104346766
Comment 2 Antoine Quint 2023-02-08 02:33:24 PST
The problem is that BuilderConverter::convertLineHeight() loses the `<number>` type here:

    if (primitiveValue.isNumber())
        return Length(primitiveValue.doubleValue() * 100.0, LengthType::Percent);

Additionally, the `normal` value is also converted to a percent value here:

    if (valueID == CSSValueNormal)
        return RenderStyle::initialLineHeight();

Where RenderStyle::initialLineHeight() builds this value:

    // Returning -100% percent here means the line-height is not set.
    static Length initialLineHeight() { return Length(-100.0f, LengthType::Percent); }

This means that when we get to deciding whether it's OK to interpolate between `normal` and `0`, which is what the original Stack Overflow example does, we consider two percent values and decide it's OK to interpolate.

We are going to need to:

1. identify the `normal` value with special logic
2. preserve the `number` type somehow

Finally, looking at the spec for `line-height` I see it takes in `<length-percentage>` as a value and yet the animation wrapper does not specify this. So there's likely another bug here where we would fail to yield a transition between "line-height: 10px" and "line-height: 10%" I expect.
Comment 3 Antoine Quint 2023-02-08 02:38:01 PST
Actually, a transition between "line-height: 10px" and "line-height: 10%" works because we convert the "10%" to a Fixed value as well so we end up having the same type. This code is pretty weird :)
Comment 4 Antoine Quint 2023-02-08 03:16:00 PST
Alright, so all we need to do is to have a custom `canInterpolate()` override for `line-height` and return false if any value maps to RenderStyle::initialLineHeight(). The default LengthPropertyWrapper logic will otherwise apply since <number> values map to LengthType::Percent and <length-percentage> values map to LengthType::Fixed.
Comment 5 Antoine Quint 2023-02-08 04:55:26 PST
Pull request: https://github.com/WebKit/WebKit/pull/9807
Comment 6 Antoine Quint 2023-02-08 04:55:53 PST
Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/38416
Comment 7 EWS 2023-02-08 12:34:04 PST
Committed 260028@main (221eb1d58b67): <https://commits.webkit.org/260028@main>

Reviewed commits have been landed. Closing PR #9807 and removing active labels.