Implement parsing and animation support for offset-distance, offset-position, offset-anchor

• Component: CSS
• Status: RESOLVED FIXED

[Kiet Ho]

Implement parsing support for offset-distance, offset-position, offset-anchor. offset-path support is planned for an upcoming patch.

[Kiet Ho]

Created attachment 440733 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Kiet Ho]

Created attachment 440734 [details]
Patch

[Kiet Ho]

Created attachment 440736 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 440736 [details]
Patch

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

Looks like you need to rebase more tests.

> Source/WebCore/ChangeLog:8
> +        No new tests; WPT parsing tests are sufficient.

If bits of this patch are from external contributors, you should acknowledge them here.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3180
> +        case CSSPropertyObjectPosition:
> +            return valueForPosition(style, style.objectPosition());
> +        case CSSPropertyOffsetDistance:
> +            return cssValuePool.createValue(style.offsetDistance(), style);
> +        case CSSPropertyOffsetPosition:
> +            return valueForPositionOrAuto(style, style.offsetPosition());
> +        case CSSPropertyOffsetAnchor:
> +            return valueForPositionOrAuto(style, style.offsetAnchor());

You can also use your helper for CSSPropertyPerspectiveOrigin, maybe others.

[Kiet Ho]

Comment on attachment 440736 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        No new tests; WPT parsing tests are sufficient.
> 
> If bits of this patch are from external contributors, you should acknowledge them here.

I'll mention WPT tests that cover the implemented properties.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3180
>> +            return valueForPositionOrAuto(style, style.offsetAnchor());
> 
> You can also use your helper for CSSPropertyPerspectiveOrigin, maybe others.

In the current code perspective-origin is parsed as a shorthand for some reason :/ If we ever change perspective-origin to use this helper, then it'd be for another patch.

[Kiet Ho]

Comment on attachment 440736 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        No new tests; WPT parsing tests are sufficient.
> 
> If bits of this patch are from external contributors, you should acknowledge them here.

I'll mention WPT tests that cover the implemented properties.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3180
>> +            return valueForPositionOrAuto(style, style.offsetAnchor());
> 
> You can also use your helper for CSSPropertyPerspectiveOrigin, maybe others.

In the current code perspective-origin is parsed as a shorthand for some reason :/ If we ever change perspective-origin to use this helper, then it'd be for another patch.

[Kiet Ho]

(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 440736 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=440736&action=review
> 
> Looks like you need to rebase more tests.
What do you mean by rebasing here?

> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests; WPT parsing tests are sufficient.
> 
> If bits of this patch are from external contributors, you should acknowledge
> them here.
> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3180
> > +        case CSSPropertyObjectPosition:
> > +            return valueForPosition(style, style.objectPosition());
> > +        case CSSPropertyOffsetDistance:
> > +            return cssValuePool.createValue(style.offsetDistance(), style);
> > +        case CSSPropertyOffsetPosition:
> > +            return valueForPositionOrAuto(style, style.offsetPosition());
> > +        case CSSPropertyOffsetAnchor:
> > +            return valueForPositionOrAuto(style, style.offsetAnchor());
> 
> You can also use your helper for CSSPropertyPerspectiveOrigin, maybe others.

[Kiet Ho]

Created attachment 440912 [details]
WIP patch to see how many tests are still failing

[Simon Fraser (smfr)]

"rebase" = re-run the tests to create new results, possibly with different results for different platforms.

We call a platform result a "baseline", so "rebase" is to create new baselines.

[Kiet Ho]

Created attachment 441119 [details]
WIP to see if the patch applies cleanly

[Kiet Ho]

Created attachment 441169 [details]
WIP patch to make sure all tests are passing

[Kiet Ho]

Created attachment 441183 [details]
WIP (again)

[Kiet Ho]

Created attachment 441186 [details]
WIP (again) (again)

[Kiet Ho]

Created attachment 441243 [details]
Patch for review

[Antoine Quint]

Comment on attachment 441243 [details]
Patch for review

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

Animation bits look good.

> Source/WebCore/animation/CSSPropertyAnimation.cpp:919
> +    bool canInterpolate(const RenderStyle& from, const RenderStyle& to) const override 

I don't know if it makes a different since the class is marked as `final` already, but this could be marked `final` as well.

[Kiet Ho]

Comment on attachment 441243 [details]
Patch for review

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

>> Source/WebCore/animation/CSSPropertyAnimation.cpp:919
>> +    bool canInterpolate(const RenderStyle& from, const RenderStyle& to) const override 
> 
> I don't know if it makes a different since the class is marked as `final` already, but this could be marked `final` as well.

Gotcha, nice to know that.

[Kiet Ho]

Created attachment 441505 [details]
Patch

[Kiet Ho]

Created attachment 441507 [details]
Patch for review

[Radar WebKit Bug Importer]

<rdar://problem/84350000>

[EWS]

Committed r284361 (243146@main): <https://commits.webkit.org/243146@main>

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