Bug 231491 - Implement parsing and animation support for offset-distance, offset-position, offset-anchor
Summary: Implement parsing and animation support for offset-distance, offset-position,...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 203847
  Show dependency treegraph
 
Reported: 2021-10-10 13:20 PDT by Kiet Ho
Modified: 2021-10-21 15:30 PDT (History)
17 users (show)

See Also:


Attachments
Patch (36.05 KB, patch)
2021-10-10 13:34 PDT, Kiet Ho
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (36.21 KB, patch)
2021-10-10 13:45 PDT, Kiet Ho
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (35.96 KB, patch)
2021-10-10 13:52 PDT, Kiet Ho
no flags Details | Formatted Diff | Diff
WIP patch to see how many tests are still failing (134.29 KB, patch)
2021-10-12 02:34 PDT, Kiet Ho
no flags Details | Formatted Diff | Diff
WIP to see if the patch applies cleanly (157.89 KB, patch)
2021-10-13 12:19 PDT, Kiet Ho
no flags Details | Formatted Diff | Diff
WIP patch to make sure all tests are passing (322.94 KB, patch)
2021-10-13 18:07 PDT, Kiet Ho
no flags Details | Formatted Diff | Diff
WIP (again) (356.95 KB, patch)
2021-10-13 21:32 PDT, Kiet Ho
no flags Details | Formatted Diff | Diff
WIP (again) (again) (328.27 KB, patch)
2021-10-13 23:30 PDT, Kiet Ho
no flags Details | Formatted Diff | Diff
Patch for review (328.62 KB, patch)
2021-10-14 11:19 PDT, Kiet Ho
no flags Details | Formatted Diff | Diff
Patch (418.10 KB, patch)
2021-10-16 14:47 PDT, Kiet Ho
no flags Details | Formatted Diff | Diff
Patch for review (418.05 KB, patch)
2021-10-16 15:48 PDT, Kiet Ho
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kiet Ho 2021-10-10 13:20:16 PDT
Implement parsing support for offset-distance, offset-position, offset-anchor. offset-path support is planned for an upcoming patch.
Comment 1 Kiet Ho 2021-10-10 13:34:06 PDT
Created attachment 440733 [details]
Patch
Comment 2 EWS Watchlist 2021-10-10 13:35:00 PDT
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
Comment 3 Kiet Ho 2021-10-10 13:45:09 PDT
Created attachment 440734 [details]
Patch
Comment 4 Kiet Ho 2021-10-10 13:52:36 PDT
Created attachment 440736 [details]
Patch
Comment 5 Simon Fraser (smfr) 2021-10-11 11:35:21 PDT
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.
Comment 6 Kiet Ho 2021-10-12 02:24:13 PDT
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.
Comment 7 Kiet Ho 2021-10-12 02:24:15 PDT
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.
Comment 8 Kiet Ho 2021-10-12 02:25:13 PDT
(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.
Comment 9 Kiet Ho 2021-10-12 02:34:01 PDT
Created attachment 440912 [details]
WIP patch to see how many tests are still failing
Comment 10 Simon Fraser (smfr) 2021-10-12 09:22:12 PDT
"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.
Comment 11 Kiet Ho 2021-10-13 12:19:42 PDT
Created attachment 441119 [details]
WIP to see if the patch applies cleanly
Comment 12 Kiet Ho 2021-10-13 18:07:52 PDT
Created attachment 441169 [details]
WIP patch to make sure all tests are passing
Comment 13 Kiet Ho 2021-10-13 21:32:18 PDT
Created attachment 441183 [details]
WIP (again)
Comment 14 Kiet Ho 2021-10-13 23:30:57 PDT
Created attachment 441186 [details]
WIP (again) (again)
Comment 15 Kiet Ho 2021-10-14 11:19:07 PDT
Created attachment 441243 [details]
Patch for review
Comment 16 Antoine Quint 2021-10-14 11:34:06 PDT
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.
Comment 17 Kiet Ho 2021-10-14 11:35:11 PDT
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.
Comment 18 Kiet Ho 2021-10-16 14:47:08 PDT
Created attachment 441505 [details]
Patch
Comment 19 Kiet Ho 2021-10-16 15:48:33 PDT
Created attachment 441507 [details]
Patch for review
Comment 20 Radar WebKit Bug Importer 2021-10-17 13:21:16 PDT
<rdar://problem/84350000>
Comment 21 EWS 2021-10-18 03:16:47 PDT
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].