Integrate motion path transforms in transformation pipeline

• Component: CSS
• Status: RESOLVED FIXED

[Kiet Ho]

Integrate motion path transforms in transformation pipeline

[Kiet Ho]

Created attachment 444367 [details]
WIP

[Kiet Ho]

Created attachment 444419 [details]
WIP

[Simon Fraser (smfr)]

Comment on attachment 444419 [details]
WIP

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

> Source/WebCore/platform/graphics/Path.cpp:567
> +    apply([&lastElementIsClosed](const WebCore::PathElement& element) {
> +        lastElementIsClosed = (element.type == PathElement::Type::CloseSubpath);
> +    });

This is a bit subtle; I didn't understand at first reading. Maybe add a comment, or find a more efficient way to do this.

> Source/WebCore/rendering/style/RenderStyle.cpp:1459
> +static std::optional<Path> getPathFromPathOperation(const FloatRect& box, const PathOperation* operation)

It looks like the only caller null-checks already, so it would be nicer if this took a const PathOperation&.

> Source/WebCore/rendering/style/RenderStyle.cpp:1474
> +    auto distanceValue = floatValueForLength(distance, pathLength);

Do we have enough context here to resolve things like viewport units and calc() in the Length here? I think not. Maybe the caller needs to resolve the length first. If this is broken, it should gain some tests.

> Source/WebCore/rendering/style/RenderStyle.cpp:1487
> +    if (path.isClosed()) {
> +        if (pathLength) {
> +            resolvedLength = fmod(distanceValue, pathLength);
> +            if (resolvedLength < 0)
> +                resolvedLength += pathLength;
> +        }
> +    } else
> +        resolvedLength = clampTo<float>(distanceValue, 0, pathLength);
> +
> +    ASSERT(resolvedLength >= 0);
> +    return path.traversalStateAtLength(resolvedLength);

Maybe traversalStateAtLength() could do the "isClosed" check and the fmod(), so you don't even need the isClosed() implementation?

> Source/WebCore/rendering/style/RenderStyle.cpp:1508
> +    if (!offsetAnchor().x().isAuto())
> +        anchor = floatPointForLengthPoint(offsetAnchor(), boundingBox.size()) + boundingBox.location();

Why is it OK to only check .x()?

> Source/WebCore/rendering/style/RenderStyle.cpp:1514
> +    FloatPoint shiftToOrigin {
> +        anchor.x() - transformOrigin.x(),
> +        anchor.y() - transformOrigin.y()
> +    };

This could be:
auto shiftToOrigin = anchor - toFloatSize(transformOrigin);

I think it's slightly more logical for shiftToOrigin to be a FloatSize.

> Source/WebCore/rendering/style/RenderStyle.cpp:1520
> +        transform.rotate(traversalState.normalAngle() + rotation.angle());

Are these angles in the correct units (radians vs. degrees)?

> Source/WebCore/rendering/style/RenderStyle.h:649
> +    bool hasTransform() const { return !m_rareNonInheritedData->transform->operations.operations().isEmpty() || offsetPath(); }

This makes me think that we need to examine all callers of hasTransformRelatedProperty() to see if they need offsetPath handling.

[Kiet Ho]

Comment on attachment 444419 [details]
WIP

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

>> Source/WebCore/platform/graphics/Path.cpp:567
>> +    });
> 
> This is a bit subtle; I didn't understand at first reading. Maybe add a comment, or find a more efficient way to do this.

I'll add a comment.

>> Source/WebCore/rendering/style/RenderStyle.cpp:1459
>> +static std::optional<Path> getPathFromPathOperation(const FloatRect& box, const PathOperation* operation)
> 
> It looks like the only caller null-checks already, so it would be nicer if this took a const PathOperation&.

Will fix.

>> Source/WebCore/rendering/style/RenderStyle.cpp:1474
>> +    auto distanceValue = floatValueForLength(distance, pathLength);
> 
> Do we have enough context here to resolve things like viewport units and calc() in the Length here? I think not. Maybe the caller needs to resolve the length first. If this is broken, it should gain some tests.

offset-distance is first parsed into a CSSPrimitiveValue, then it's converted to a Length using BuilderConverter::convertLength. I briefly went through the code, and it properly handles all CSS units and calc().

>> Source/WebCore/rendering/style/RenderStyle.cpp:1487
>> +    return path.traversalStateAtLength(resolvedLength);
> 
> Maybe traversalStateAtLength() could do the "isClosed" check and the fmod(), so you don't even need the isClosed() implementation?

My justification is that Path::isClose() might be useful to someone in the future, but if you want to, I can move the check into this function.

>> Source/WebCore/rendering/style/RenderStyle.cpp:1508
>> +        anchor = floatPointForLengthPoint(offsetAnchor(), boundingBox.size()) + boundingBox.location();
> 
> Why is it OK to only check .x()?

If offset-anchor is auto, then both x() and y() are auto. It's not possible to have an auto x() and non-auto y(), and vice versa.

>> Source/WebCore/rendering/style/RenderStyle.cpp:1514
>> +    };
> 
> This could be:
> auto shiftToOrigin = anchor - toFloatSize(transformOrigin);
> 
> I think it's slightly more logical for shiftToOrigin to be a FloatSize.

Will fix.

>> Source/WebCore/rendering/style/RenderStyle.cpp:1520
>> +        transform.rotate(traversalState.normalAngle() + rotation.angle());
> 
> Are these angles in the correct units (radians vs. degrees)?

Both of them are in degrees.

>> Source/WebCore/rendering/style/RenderStyle.h:649
>> +    bool hasTransform() const { return !m_rareNonInheritedData->transform->operations.operations().isEmpty() || offsetPath(); }
> 
> This makes me think that we need to examine all callers of hasTransformRelatedProperty() to see if they need offsetPath handling.

Most certainly. I did try to go through every caller of hasTransform(), but I get overwhelmed rather quickly.

[Kiet Ho]

Created attachment 444752 [details]
Patch

[Kiet Ho]

Created attachment 444816 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/85611888>

[Dean Jackson]

Comment on attachment 444816 [details]
Patch

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

> Source/WebCore/rendering/style/RenderStyle.cpp:1468
> +    // FIXME: support Reference and Box type.

Can you file a bugzilla for this and mention it here?

> Source/WebCore/rendering/style/RenderStyle.cpp:1505
> +    // Default value of offset-anchor is transform-origin

Nit: This comment should end with a period. However, I don't think you need the comment anyway :)

> Source/WebCore/rendering/style/RenderStyle.cpp:1507
> +    // Handle the case if offset-anchor is not auto

Same with this. The comment isn't necessary.

[Kiet Ho]

Created attachment 444851 [details]
Patch

[EWS]

Committed r286085 (244472@main): <https://commits.webkit.org/244472@main>

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