Bug 232752

Summary: Implement parsing and animation support for offset-rotate
Product: WebKit Reporter: Kiet Ho <tho22>
Component: CSSAssignee: Kiet Ho <tho22>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, changseok, esprehn+autocc, ews-watchlist, glenn, graouts, gyuyoung.kim, heycam, joepeck, kondapallykalyan, macpherson, menard, mmaxfield, pdr, ryuan.choi, sergio, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 203847    
Attachments:
Description Flags
Patch
none
Patch. Fix build for platforms using CMake
ews-feeder: commit-queue-
Patch. Fix build for platforms using CMake
none
Patch
none
Patch
none
Patch none

Description Kiet Ho 2021-11-05 04:45:17 PDT
Implement parsing and animation support for offset-path
Comment 1 Kiet Ho 2021-11-05 04:45:54 PDT
(apologies, that's supposed to be offset-rotate, not offset-path)
Comment 2 Kiet Ho 2021-11-05 05:59:46 PDT
Created attachment 443389 [details]
Patch
Comment 3 Antoine Quint 2021-11-05 08:46:49 PDT
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?
Comment 4 Kiet Ho 2021-11-05 09:12:22 PDT
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?
Comment 5 Kiet Ho 2021-11-05 20:40:30 PDT
Created attachment 443472 [details]
Patch. Fix build for platforms using CMake
Comment 6 Kiet Ho 2021-11-05 20:52:23 PDT
Created attachment 443473 [details]
Patch. Fix build for platforms using CMake
Comment 7 Myles C. Maxfield 2021-11-08 13:14:35 PST
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?
Comment 8 Kiet Ho 2021-11-08 13:28:08 PST
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
Comment 9 Cameron McCormack (:heycam) 2021-11-11 12:28:58 PST
(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.
Comment 10 Radar WebKit Bug Importer 2021-11-12 03:46:19 PST
<rdar://problem/85338011>
Comment 11 Kiet Ho 2021-11-12 13:06:54 PST
Created attachment 444091 [details]
Patch
Comment 12 Myles C. Maxfield 2021-11-12 13:12:27 PST
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.
Comment 13 Simon Fraser (smfr) 2021-11-12 13:29:26 PST
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).
Comment 14 Kiet Ho 2021-11-12 13:31:54 PST
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?
Comment 15 Kiet Ho 2021-11-12 16:03:36 PST
Created attachment 444117 [details]
Patch
Comment 16 Kiet Ho 2021-11-14 12:12:07 PST
Created attachment 444185 [details]
Patch
Comment 17 EWS 2021-11-15 12:18:12 PST
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].