Support rendering url(), CSS basic shapes other than path(), and coord-box for offset-path

• Component: CSS
• Status: RESOLVED FIXED

[Kiet Ho]

Support rendering url(), CSS basic shapes other than path(), and coord-box for offset-path. Right now offset-path will parse them just fine, but will ignore any shapes other than path().

[Radar WebKit Bug Importer]

<rdar://problem/85772826>

[Nikos Mouchtaris]

Created attachment 452482 [details]
WIP

[Nikos Mouchtaris]

Created attachment 452496 [details]
WIP

[Dean Jackson]

Comment on attachment 452496 [details]
WIP

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

> Source/WebCore/rendering/style/RenderStyle.cpp:1474
> +            // how to get current render box- want something like computeRoundedRectForBoxShape
> +            // marginBoxRect();pathForReferenceRect
> +            // return downcast<BoxPathOperation>(operation).pathForReferenceRect(box);

Did you intend to leave this comment in?

[Nikos Mouchtaris]

Created attachment 452611 [details]
WIP

[Nikos Mouchtaris]

Created attachment 452827 [details]
Patch

[Nikos Mouchtaris]

Created attachment 455706 [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

[Nikos Mouchtaris]

Created attachment 455707 [details]
Patch

[Dean Jackson]

Comment on attachment 455707 [details]
Patch

would like to see more tests - shame WPT doesn't have them.

[Nikos Mouchtaris]

Created attachment 455812 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 455707 [details]
Patch

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

> Source/WebCore/rendering/PathOperation.h:71
> +    static Ref<ReferencePathOperation> create(const String&, const String&, const RefPtr<SVGElement>);

You need to name the string parameters in the header so the reader knows what they are.

> Source/WebCore/rendering/PathOperation.h:85
> +    ReferencePathOperation(const String&, const String&, const RefPtr<SVGElement>);

Ditto

> Source/WebCore/rendering/RenderLayer.cpp:1345
> +        if (is<RenderBox>(renderer())) {
> +            auto cssBox = transformBoxToCSSBoxType(renderer().style().transformBox());
> +            if (auto pathOperation = renderer().style().offsetPath()) {
> +                if (pathOperation->type() == PathOperation::Box)
> +                    cssBox = downcast<BoxPathOperation>(pathOperation)->referenceBox();
> +            }
> +
> +            referenceBox = snapRectToDevicePixels(downcast<RenderBox>(renderer()).referenceBox(cssBox), renderer().document().deviceScaleFactor());
> +        }

This doesn't seem right. The presence of an offset path shouldn't affect transform rendering. This needs a testcase.

> Source/WebCore/rendering/style/RenderStyle.cpp:1528
> +    switch (operation.type())
> +    {

Brace on the previous line.

> Source/WebCore/rendering/style/RenderStyle.cpp:1532
> +        case PathOperation::Reference:
> +        {

Ditto

> Source/WebCore/rendering/style/RenderStyle.cpp:1538
> +        case PathOperation::Box:
> +        {

Ditto

> Source/WebCore/rendering/style/RenderStyle.cpp:1542
> +        case PathOperation::Ray:
> +        {

Ditto

[Nikos Mouchtaris]

Created attachment 456422 [details]
Patch

[Tim Nguyen (:ntim)]

Comment on attachment 456422 [details]
Patch

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

Looks like there are some trivial style errors.

> Source/WebCore/ChangeLog:16
> +        Tests: imported/w3c/web-platform-tests/css/motion/offset-path-shape.html
> +               imported/w3c/web-platform-tests/css/motion/offset-path-url.html

Can you please export these tests? They're not upstream

[Simon Fraser (smfr)]

Comment on attachment 456422 [details]
Patch

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

> Source/WebCore/rendering/PathOperation.cpp:2
> + * Copyright (C) 2012, 2013 Adobe Systems Incorporated. All rights reserved.

Doesn't seem right for a new file.

> Source/WebCore/rendering/PathOperation.h:74
> +    const RefPtr<SVGElement> element() const;

This can return a SVGElement*

> Source/WebCore/rendering/PathOperation.h:85
> +    ReferencePathOperation(const String& url, const String& fragment, const RefPtr<SVGElement> element);

The argument can be a SVGElement*

> Source/WebCore/rendering/RenderLayer.cpp:1340
> +                if (pathOperation->type() == PathOperation::Box) {

This should use is<BoxPathOperation>()

> Source/WebCore/rendering/style/RenderStyle.cpp:1537
> +        case PathOperation::Box: {
> +            return downcast<BoxPathOperation>(operation).getPath();
> +        }

You only need braces in case statements if you declare local variables. None of these cases do.

[Nikos Mouchtaris]

Created attachment 456649 [details]
Patch

[Tim Nguyen (:ntim)]

Comment on attachment 456649 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/css/motion/offset-path-geometry-box.html:6
> +    <link rel="match" href="offset-path-shape-ref.html">

The ref is wrong here. I'll fix it upstream in the WPT PR, but please fix it in this patch.

[Tim Nguyen (:ntim)]

Comment on attachment 456649 [details]
Patch

Please also fix the webkit-style errors on EWS, I think it's complaining specifically about `const RefPtr<SVGElement> element` in each constructor, I think it's sensible to remove the parameter names in those cases.

[Tim Nguyen (:ntim)]

Created attachment 456674 [details]
Patch for landing

Addressed my 2 own comments (I copy pasted the previous attachment then did manual edits)

[Tim Nguyen (:ntim)]

Comment on attachment 456674 [details]
Patch for landing

oh looks like you can't just copy an attachment and edit it

[Tim Nguyen (:ntim)]

Created attachment 456675 [details]
Patch for landing

[EWS]

Committed r292382 (249247@main): <https://commits.webkit.org/249247@main>

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