Implement parsing and animation support for ray() shape accepted by offset-path

• Component: CSS
• Status: RESOLVED FIXED

[Kiet Ho]

Implement parsing and animation support for ray() shape allowed by offset-path

[Kiet Ho]

Created attachment 444511 [details]
WIP

[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 444696 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/85611929>

[Dean Jackson]

Comment on attachment 444696 [details]
Patch

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

> Source/WebCore/css/parser/CSSPropertyParser.cpp:2807
> +    RefPtr<CSSPrimitiveValue> isContaining;
> +    while (!args.atEnd()) {
> +        if (!angle && (angle = consumeAngle(args, context.mode, UnitlessQuirk::Forbid, UnitlessZeroQuirk::Forbid)))
> +            continue;
> +
> +        if (!size && (size = consumeIdent<CSSValueClosestSide, CSSValueClosestCorner, CSSValueFarthestSide, CSSValueFarthestCorner, CSSValueSides>(args)))
> +            continue;
> +
> +        if (!isContaining && (isContaining = consumeIdent<CSSValueContain>(args)))
> +            continue;
> +
> +        return nullptr;
> +    }
> +
> +    // <angle> and <size> must be present.
> +    if (!angle || !size)
> +        return nullptr;
> +
> +    return CSSRayValue::create(angle.releaseNonNull(), size.releaseNonNull(), isContaining);

Given that you end up passing a bool for the isContaining parameter, I wonder if you ever need the RefPtr<CSSPrimitiveValue> for it. Wouldn't it work to have a bool variable, and simply test that consumeIdent<CSSValueContain> returns a value, then set the bool to true before calling continue?

> Source/WebCore/style/StyleBuilderConverter.h:663
> +        RayPathOperation::Size size = RayPathOperation::Size::ClosestCorner;
> +        switch (rayValue.size()->valueID()) {
> +        case CSSValueClosestCorner:
> +            size = RayPathOperation::Size::ClosestCorner;
> +            break;
> +        case CSSValueClosestSide:
> +            size = RayPathOperation::Size::ClosestSide;
> +            break;
> +        case CSSValueFarthestCorner:
> +            size = RayPathOperation::Size::FarthestCorner;
> +            break;
> +        case CSSValueFarthestSide:
> +            size = RayPathOperation::Size::FarthestSide;
> +            break;
> +        case CSSValueSides:
> +            size = RayPathOperation::Size::Sides;
> +            break;
> +        default:
> +            ASSERT_NOT_REACHED();
> +            return nullptr;
> +        }

Not important, but I see people tend to use immediately called lambdas for initialisation of variables? e.g.

auto size = [] (CSSValueID valueID) -> RayPathOperation::Size {
  switch (valueID) {
  case CSSValueClosestCorner:
     return ClosestCorner;
...


}(rayValue.size()->valueID());

Don't make the change though. This is fine.

> LayoutTests/imported/w3c/web-platform-tests/css/motion/parsing/offset-path-computed.html:22
> -test_computed_value("offset-path", "ray(calc(180deg - 45deg) farthest-side)", "ray(calc(135deg) farthest-side)");
> +test_computed_value("offset-path", "ray(calc(180deg - 45deg) farthest-side)", "ray(135deg farthest-side)");

This is making a change to an imported test? Was it intentional? What happens when we update WPT?

[Kiet Ho]

Comment on attachment 444696 [details]
Patch

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

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:2807
>> +    return CSSRayValue::create(angle.releaseNonNull(), size.releaseNonNull(), isContaining);
> 
> Given that you end up passing a bool for the isContaining parameter, I wonder if you ever need the RefPtr<CSSPrimitiveValue> for it. Wouldn't it work to have a bool variable, and simply test that consumeIdent<CSSValueContain> returns a value, then set the bool to true before calling continue?

You're right, I'll change it to just a simple bool.

>> LayoutTests/imported/w3c/web-platform-tests/css/motion/parsing/offset-path-computed.html:22
>> +test_computed_value("offset-path", "ray(calc(180deg - 45deg) farthest-side)", "ray(135deg farthest-side)");
> 
> This is making a change to an imported test? Was it intentional? What happens when we update WPT?

This change is exported here: https://github.com/web-platform-tests/wpt/pull/31658. Once this patch is merged, the bot will automatically merge the WPT pull request.

[Kiet Ho]

Created attachment 444859 [details]
Patch

[EWS]

Committed r286086 (244473@main): <https://commits.webkit.org/244473@main>

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