Bug 231801 – Implement parsing and animation support for offset-path

• Component: CSS
• Status: RESOLVED FIXED

[Kiet Ho]

Implement parsing and animation support for offset-path

[Kiet Ho]

Created attachment 441355 [details]
WIP

[Kiet Ho]

Created attachment 441360 [details]
WIP

[Kiet Ho]

Okay, the problem is that this patch depends on the previous offset-position/offset-anchor/offset-distance patch, it won't apply cleanly without the mentioned patch. I'll push new patches once the offset-position/offset-anchor/offset-distance patch got merged.

[Kiet Ho]

Created attachment 441588 [details]
WIP

[Kiet Ho]

Created attachment 442124 [details]
Patch for review

[Kiet Ho]

Note for reviewers:

* Two tests in offset-path-parsing-valid.html are still failing. WebKit is able to parse them just fine, however the serialized value returned by getPropertyValue() doesn't match the expected value. Maybe someone can help me determine which is the correct serialization, since Chrome and Firefox also fails this test.

* Some animation tests (in offset-path-interpolation-004.html) are failing. This is probably due to an existing bug in SVGPathBlender before this patch.

[Kiet Ho]

Created attachment 442128 [details]
Patch for review (try 2)

[Kiet Ho]

(please hold off from reviewing this because I found some nasty bugs that surfaces just from changing build configuration (Debug to Release))

[Radar WebKit Bug Importer]

<rdar://problem/84542616>

[Kiet Ho]

Created attachment 442146 [details]
Patch for review (try 3)

[Simon Fraser (smfr)]

Comment on attachment 442146 [details]
Patch for review (try 3)

Please do the PathOperation rename in a first, initial patch.

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #11)
> Comment on attachment 442146 [details]
> Patch for review (try 3)
> 
> Please do the PathOperation rename in a first, initial patch.

I typed the exact same thing when I saw your comment as I submitted mine.

[Kiet Ho]

Created attachment 442227 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 442227 [details]
Patch

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

> Source/WebCore/animation/CSSPropertyAnimation.cpp:279
> +    if (!from || !to) {
> +        // fall back to discrete animation.
> +        return context.progress < 0.5 ? from : to;
> +    }

This is a behavior change that you should separate out into its own patch (and seems testable via animations).

> Source/WebCore/animation/CSSPropertyAnimation.cpp:1039
> +        auto fromPathOperation = value(from), toPathOperation = value(to);

Declare each variable on its own line (no commas).

> Source/WebCore/animation/CSSPropertyAnimation.cpp:1042
> +        if (!fromPathOperation || !toPathOperation)

You can combine the type and null  checks here:

if (!is<ShapePathOperation>(fromPathOperation) || !is<ShapePathOperation>(toPathOperation))
  return false

> Source/WebCore/animation/CSSPropertyAnimation.cpp:1047
> +            const BasicShape& fromShape = downcast<ShapePathOperation>(*fromPathOperation).basicShape();

auto& (const is inferred).

> Source/WebCore/css/BasicShapeFunctions.cpp:112
> +        std::unique_ptr<SVGPathByteStream> path = pathShape.pathData()->copy();

auto. And it's not a path, it's a byte stream.

> Source/WebCore/css/BasicShapeFunctions.cpp:114
> +            auto absolutePath = makeUnique<SVGPathByteStream>();

Shame we have to make another byte stream here. Maybe give copy() an argument that says if the path should be absolutized (or add absoluteCopy()).

> Source/WebCore/css/BasicShapeFunctions.h:43
> +Ref<CSSPrimitiveValue> valueForBasicShape(const RenderStyle&, const BasicShape&, bool forceSVGAbsolutePath = false);

It's bad to use 'bool' where the call sites will pass "true" or "false" - it results in hard to read code. An enum would be better.

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:238
> +    RefPtr<PathOperation> offsetPath;

Move this up next to the other pointer types (to below translate).

> Source/WebCore/svg/SVGPathAbsoluteConverter.h:54
> +    void incrementPathSegmentCount() final;
> +    bool continueConsuming() final;
> +
> +    // Used in UnalteredParsing/NormalizedParsing modes.
> +    void moveTo(const FloatPoint& targetPoint, bool closed, PathCoordinateMode) final;
> +    void lineTo(const FloatPoint& targetPoint, PathCoordinateMode) final;
> +    void curveToCubic(const FloatPoint& point1, const FloatPoint& point2, const FloatPoint& targetPoint, PathCoordinateMode) final;
> +    void closePath() final;
> +
> +    // Only used in UnalteredParsing mode.
> +    void lineToHorizontal(float targetX, PathCoordinateMode) final;
> +    void lineToVertical(float targetY, PathCoordinateMode) final;
> +    void curveToCubicSmooth(const FloatPoint& point2, const FloatPoint& targetPoint, PathCoordinateMode) final;
> +    void curveToQuadratic(const FloatPoint& point1, const FloatPoint& targetPoint, PathCoordinateMode) final;
> +    void curveToQuadraticSmooth(const FloatPoint& targetPoint, PathCoordinateMode) final;
> +    void arcTo(float r1, float r2, float angle, bool largeArcFlag, bool sweepFlag, const FloatPoint& targetPoint, PathCoordinateMode) final;

Can these all be private?

It's weird that PathCoordinateMode is a per-segment thing rather than a mode on the path consumer.

[Kiet Ho]

Comment on attachment 442227 [details]
Patch

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

>> Source/WebCore/animation/CSSPropertyAnimation.cpp:279
>> +    }
> 
> This is a behavior change that you should separate out into its own patch (and seems testable via animations).

I agree, I'll factor this out to a separate patch. (which should be merged first before this one)

>> Source/WebCore/css/BasicShapeFunctions.cpp:114
>> +            auto absolutePath = makeUnique<SVGPathByteStream>();
> 
> Shame we have to make another byte stream here. Maybe give copy() an argument that says if the path should be absolutized (or add absoluteCopy()).

I'll see if there's a way to avoid double copying here.

>> Source/WebCore/css/BasicShapeFunctions.h:43
>> +Ref<CSSPrimitiveValue> valueForBasicShape(const RenderStyle&, const BasicShape&, bool forceSVGAbsolutePath = false);
> 
> It's bad to use 'bool' where the call sites will pass "true" or "false" - it results in hard to read code. An enum would be better.

This enum would be declared in BasicShapeFunctions.h right?

>> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:238
>> +    RefPtr<PathOperation> offsetPath;
> 
> Move this up next to the other pointer types (to below translate).

I put offsetPath here to group the offset* elements together. Is there any benefits to move this next to other pointer members? Struct padding perhaps?

>> Source/WebCore/svg/SVGPathAbsoluteConverter.h:54
>> +    void arcTo(float r1, float r2, float angle, bool largeArcFlag, bool sweepFlag, const FloatPoint& targetPoint, PathCoordinateMode) final;
> 
> Can these all be private?
> 
> It's weird that PathCoordinateMode is a per-segment thing rather than a mode on the path consumer.

You mean everything including the constructor? Is there any benefit to that?

Also, PathCoordinateMode is per-segment because SVG allows mixing absolute and relative draw commands.

[Kiet Ho]

Comment on attachment 442227 [details]
Patch

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

>> Source/WebCore/animation/CSSPropertyAnimation.cpp:279
>> +    }
> 
> This is a behavior change that you should separate out into its own patch (and seems testable via animations).

I agree, I'll factor this out to a separate patch. (which should be merged first before this one)

>> Source/WebCore/css/BasicShapeFunctions.cpp:114
>> +            auto absolutePath = makeUnique<SVGPathByteStream>();
> 
> Shame we have to make another byte stream here. Maybe give copy() an argument that says if the path should be absolutized (or add absoluteCopy()).

I'll see if there's a way to avoid double copying here.

>> Source/WebCore/css/BasicShapeFunctions.h:43
>> +Ref<CSSPrimitiveValue> valueForBasicShape(const RenderStyle&, const BasicShape&, bool forceSVGAbsolutePath = false);
> 
> It's bad to use 'bool' where the call sites will pass "true" or "false" - it results in hard to read code. An enum would be better.

This enum would be declared in BasicShapeFunctions.h right?

>> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:238
>> +    RefPtr<PathOperation> offsetPath;
> 
> Move this up next to the other pointer types (to below translate).

I put offsetPath here to group the offset* elements together. Is there any benefits to move this next to other pointer members? Struct padding perhaps?

>> Source/WebCore/svg/SVGPathAbsoluteConverter.h:54
>> +    void arcTo(float r1, float r2, float angle, bool largeArcFlag, bool sweepFlag, const FloatPoint& targetPoint, PathCoordinateMode) final;
> 
> Can these all be private?
> 
> It's weird that PathCoordinateMode is a per-segment thing rather than a mode on the path consumer.

You mean everything including the constructor? Is there any benefit to that?

Also, PathCoordinateMode is per-segment because SVG allows mixing absolute and relative draw commands.

[Kiet Ho]

Created attachment 442646 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 442646 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

Some words here please.

> Source/WebCore/css/BasicShapeFunctions.cpp:72
> +        return source.copy();

Shame to copy when no conversion is necessary. It looks like this copy is already there but maybe we can avoid it.

> Source/WebCore/css/BasicShapeFunctions.h:48
> +    // Keep the path as is.
> +    None,
> +
> +    // Convert relative commands in a path to absolute.

I don't think you need the comments.

> Source/WebCore/css/BasicShapeFunctions.h:49
> +    ForceAbsolute

Maybe just "Absolute"

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3209
> +            // The computed value of offset-path must only contain absolute draw commands.
> +            // Reference: https://github.com/w3c/fxtf-drafts/issues/225#issuecomment-334322738:
> +            // > RESOLVED: Computed-value normalize case of path commands (to the absolute version).

I don't think this comment is necessary,.

> Source/WebCore/svg/SVGPathAbsoluteConverter.cpp:35
> +    , m_currentPoint()
> +    , m_subpathPoint()

No need to initialize these. They have default constructors.

> Source/WebCore/svg/SVGPathAbsoluteConverter.cpp:44
> +bool SVGPathAbsoluteConverter::continueConsuming()

continueConsuming() const (needs fixing in the base class too).

> Source/WebCore/svg/SVGPathUtilities.cpp:214
> +bool convertSVGPathByteStreamToAbsoluteCoordinates(const SVGPathByteStream& stream, SVGPathByteStream& result)

Would be nicer to write this as std::unique_ptr<SVGPathByteStream> convertSVGPathByteStreamToAbsoluteCoordinates(const SVGPathByteStream& stream) and return nullptr on failure.

[Kiet Ho]

Comment on attachment 442646 [details]
Patch

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

>> Source/WebCore/css/BasicShapeFunctions.h:49
>> +    ForceAbsolute
> 
> Maybe just "Absolute"

SVGPathConversion::Absolute doesn't sound right to me. I think ForceAbsolute is more appropriate, as it sounds like an action compared to just Absolute.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3209
>> +            // > RESOLVED: Computed-value normalize case of path commands (to the absolute version).
> 
> I don't think this comment is necessary,.

I think it's necessary because someone might ask the question of why offset-path needs this but not clip-path. The Motion Path spec doesn't mention this either.

>> Source/WebCore/svg/SVGPathAbsoluteConverter.cpp:35
>> +    , m_subpathPoint()
> 
> No need to initialize these. They have default constructors.

Will remove.

>> Source/WebCore/svg/SVGPathAbsoluteConverter.cpp:44
>> +bool SVGPathAbsoluteConverter::continueConsuming()
> 
> continueConsuming() const (needs fixing in the base class too).

It does seems like continueConsuming() can be made const, but what if something in the future needs it to be non-const?

>> Source/WebCore/svg/SVGPathUtilities.cpp:214
>> +bool convertSVGPathByteStreamToAbsoluteCoordinates(const SVGPathByteStream& stream, SVGPathByteStream& result)
> 
> Would be nicer to write this as std::unique_ptr<SVGPathByteStream> convertSVGPathByteStreamToAbsoluteCoordinates(const SVGPathByteStream& stream) and return nullptr on failure.

Will change.

[Kiet Ho]

Created attachment 443387 [details]
Patch

[Kiet Ho]

Created attachment 443388 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 443388 [details]
Patch

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

> Source/WebCore/ChangeLog:20
> +        Implements parsing and animation support for offset-path based on clip-path.
> +        offset-path additionally supports ray() shape, which will be implemented in a future patch.

This goes above the Tests: list.

> Source/WebCore/css/BasicShapeFunctions.h:44
> +enum class SVGPathConversion {

enum class SVGPathConversion : uint8_t

[Kiet Ho]

Created attachment 443398 [details]
Patch

[EWS]

Committed r285343 (243904@main): <https://commits.webkit.org/243904@main>

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