Bug 232752 – Implement parsing and animation support for offset-rotate

• Component: CSS
• Status: RESOLVED FIXED

[Kiet Ho]

Implement parsing and animation support for offset-path

[Kiet Ho]

(apologies, that's supposed to be offset-rotate, not offset-path)

[Kiet Ho]

Created attachment 443389 [details]
Patch

[Antoine Quint]

Comment on attachment 443389 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443389&action=review

> Source/WebCore/animation/CSSPropertyAnimation.cpp:588
> +    ASSERT(from.isAuto() == to.isAuto());

Wouldn't we want to assert that both of those are false as well? I expect this to be `ASSERT(!from.isAuto() && !to.isAuto());`

> Source/WebCore/animation/CSSPropertyAnimation.cpp:683
> +        return value(from).isAuto() == value(to).isAuto();

I don't think you want to interpolate if either of the values is "auto", so maybe `return !value(from).isAuto() && !value(to).isAuto();`. This is how AutoPropertyWrapper::canInterpolate works.

> LayoutTests/imported/w3c/web-platform-tests/css/motion/animation/offset-rotate-interpolation-expected.txt:309
> +FAIL CSS Transitions: property <offset-rotate> from [800deg] to [900deg] at (-3.40282e+38) should be [-3.40282e+38deg] assert_equals: expected "- 3.4e + 38deg " but got "- 3.4e + 40deg "
> +FAIL CSS Transitions with transition: all: property <offset-rotate> from [800deg] to [900deg] at (-3.40282e+38) should be [-3.40282e+38deg] assert_equals: expected "- 3.4e + 38deg " but got "- 3.4e + 40deg "
> +FAIL CSS Animations: property <offset-rotate> from [800deg] to [900deg] at (-3.40282e+38) should be [-3.40282e+38deg] assert_equals: expected "- 3.4e + 38deg " but got "- 3.4e + 40deg "
> +FAIL Web Animations: property <offset-rotate> from [800deg] to [900deg] at (-3.40282e+38) should be [-3.40282e+38deg] assert_equals: expected "- 3.4e + 38deg " but got "- 3.4e + 40deg "

Do you know what is wrong with these four failures?

[Kiet Ho]

Comment on attachment 443389 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443389&action=review

>> Source/WebCore/animation/CSSPropertyAnimation.cpp:683
>> +        return value(from).isAuto() == value(to).isAuto();
> 
> I don't think you want to interpolate if either of the values is "auto", so maybe `return !value(from).isAuto() && !value(to).isAuto();`. This is how AutoPropertyWrapper::canInterpolate works.

offset-rotate is special regarding auto: setting offset-rotate to auto means the angle is dynamically adjusted to make the box perpendicular to the path. Setting offset-rotate to "auto Xdeg" means the angle is the angle to make the box perpendicular, plus Xdeg. So you can have an animation from "auto" (equivalent to "auto 0deg") and "auto 100deg", and it'll change from "auto 0deg" to "auto 1 deg" to ... to "auto 100deg". As such, two angles are interpolable if they are both auto, or they are both not auto.

The WPT has interpolation tests between auto angles and auto/not-auto angles:
>       // Interpolation between auto angles.
>      test_interpolation({
>        property: 'offset-rotate',
>        from: 'auto 10deg',
>        to: 'auto 50deg'
>      }, [
>        {at: -0.3, expect: 'auto -2deg'},
>        {at: 0, expect: 'auto 10deg'},
>        {at: 0.3, expect: 'auto 22deg'},
>        {at: 0.6, expect: 'auto 34deg'},
>        {at: 1, expect: 'auto 50deg'},
>        {at: 1.5, expect: 'auto 70deg'},
>      ]);
>      // No interpolation between auto/reverse and angle.
>      test_no_interpolation({
>        property: 'offset-rotate',
>        from: 'auto 200deg',
>        to: '300deg'
>      });

>> LayoutTests/imported/w3c/web-platform-tests/css/motion/animation/offset-rotate-interpolation-expected.txt:309
>> +FAIL Web Animations: property <offset-rotate> from [800deg] to [900deg] at (-3.40282e+38) should be [-3.40282e+38deg] assert_equals: expected "- 3.4e + 38deg " but got "- 3.4e + 40deg "
> 
> Do you know what is wrong with these four failures?

The test is:

>      // Regression test for crbug.com/918430.
>      test_interpolation({
>        property: 'offset-rotate',
>        from: '800deg',
>        to: '900deg'
>      }, [
>        {at: -3.40282e+38, expect: '-3.40282e+38deg'},
>      ]);

Looking at the Chromium bug report, it seems that they store the angle as a float. The fuzzer finds a test case where the resulting angle exceeds the maximum value of float, and they fixed it by clamping to the maximum value. This patch stores the angle as double, so it doesn't suffer from the same problem. Maybe someone could tell me if we should use double or float like Chromium?

[Kiet Ho]

Created attachment 443472 [details]
Patch. Fix build for platforms using CMake

[Kiet Ho]

Created attachment 443473 [details]
Patch. Fix build for platforms using CMake

[Myles C. Maxfield]

Comment on attachment 443473 [details]
Patch. Fix build for platforms using CMake

View in context: https://bugs.webkit.org/attachment.cgi?id=443473&action=review

Looks like a good start to the feature.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2510
> +        if (rotation.angle() != 0.0)

Is “auto 0” supposed to serialize to just “auto”?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:4046
> +    auto list = CSSValueList::createSpaceSeparated();

Did you consider making a new subclass of CSSValue for this? It always feels icky to me pulling out the 1th item from a list and assuming it has some particular meaning. Or, isn’t there already a CSSValue that holds pairs?

> Source/WebCore/rendering/style/OffsetRotation.h:43
> +    double m_angle;

I don’t quite understand this. How do you differentiate the case between “offset-rotation: auto” and “offset-rotation: auto 30deg”? It seems like you’d need optional<double> m_angle.

Also why double and not float?

> Source/WebCore/style/StyleBuilderConverter.h:1616
> +    for (auto element : list) {

😕

> Source/WebCore/style/StyleBuilderConverter.h:1639
> +        angle += 180.0;

+ 180, rather than * -1?

[Kiet Ho]

Comment on attachment 443473 [details]
Patch. Fix build for platforms using CMake

View in context: https://bugs.webkit.org/attachment.cgi?id=443473&action=review

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2510
>> +        if (rotation.angle() != 0.0)
> 
> Is “auto 0” supposed to serialize to just “auto”?

It's debated here: https://github.com/w3c/fxtf-drafts/issues/340
> Closing as invalid, because the spec doesn't specify serialization at all, and so the general CSSOM rules apply - shortest serialization, in grammar order.

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:4046
>> +    auto list = CSSValueList::createSpaceSeparated();
> 
> Did you consider making a new subclass of CSSValue for this? It always feels icky to me pulling out the 1th item from a list and assuming it has some particular meaning. Or, isn’t there already a CSSValue that holds pairs?

I'll look into something that doesn't abuse CSSValueList. I did consider making a CSSValue subclass, but I scraped it because I thought it's better to reuse as much existing code as possible.

>> Source/WebCore/rendering/style/OffsetRotation.h:43
>> +    double m_angle;
> 
> I don’t quite understand this. How do you differentiate the case between “offset-rotation: auto” and “offset-rotation: auto 30deg”? It seems like you’d need optional<double> m_angle.
> 
> Also why double and not float?

The CSS property is not "offset-rotation" but "offset-rotate". Let me know if you want to rename the class to OffsetRotate.

"offset-rotate: auto" is the same as "offset-rotate: auto 0deg", so in this case the angle is 0.0.

I have no explanation for the double except that double is bigger than float, and I think CSS values are supposed to be infinite precision.

>> Source/WebCore/style/StyleBuilderConverter.h:1639
>> +        angle += 180.0;
> 
> + 180, rather than * -1?

The spec definition is + 180 https://www.w3.org/TR/motion-1/#valdef-offset-rotate-reverse

[Cameron McCormack (:heycam)]

(In reply to Kiet Ho from comment #8)
> I have no explanation for the double except that double is bigger than
> float, and I think CSS values are supposed to be infinite precision.

There's a tradeoff, since double takes more memory.  Searching for "float" and "double" in files under Source/WebCore/rendering/style/, it looks like float is much more common than double.

CSS itself doesn't require a particular precision -- https://drafts.csswg.org/css-values-3/#numeric-ranges says implementations should support a useful range of values.

I would go with float rather than double unless there's a specific need for more precision, to follow the existing code.

[Radar WebKit Bug Importer]

<rdar://problem/85338011>

[Kiet Ho]

Created attachment 444091 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 444091 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444091&action=review

> Source/WebCore/style/StyleBuilderConverter.h:1625
> +    if (auto* modifierValue = offsetRotateValue.modifier()) {

This is much better than your previous `for` loop.

[Simon Fraser (smfr)]

Comment on attachment 444091 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444091&action=review

> Source/WebCore/rendering/style/OffsetRotation.h:44
> +    bool m_hasAuto;
> +    float m_angle;

Probably doesn't matter but there are 3 bytes of padding before m_angle. You could flip them.

> Source/WebCore/style/StyleBuilderConverter.h:1615
> +    float angle = 0;

Whenever you have angles, it's good to disambiguate the units. So angleInDegrees (ideally we'd have a typedef or type-safe class).

[Kiet Ho]

Comment on attachment 444091 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444091&action=review

>> Source/WebCore/style/StyleBuilderConverter.h:1615
>> +    float angle = 0;
> 
> Whenever you have angles, it's good to disambiguate the units. So angleInDegrees (ideally we'd have a typedef or type-safe class).

Would this also apply to the `angle` member in OffsetRotation?

[Kiet Ho]

Created attachment 444117 [details]
Patch

[Kiet Ho]

Created attachment 444185 [details]
Patch

[EWS]

Committed r285822 (244262@main): <https://commits.webkit.org/244262@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444185 [details].