[SVG2]: Add auto behavior for rx and ry to the SVG <ellipse> and <rect> elements

• Component: SVG
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

Specs: https://www.w3.org/TR/SVG/geometry.html#RX

Test case:

<svg xmlns="http://www.w3.org/2000/svg">
    <ellipse style="rx: auto;" ry="30" cx="50" cy="50" fill="none" stroke="blue" stroke-width="5"/>
</svg>

[Said Abou-Hallawa]

WPT failed tests:

http://web-platform-tests.live/svg/shapes/ellipse-02.svg
http://web-platform-tests.live/svg/shapes/ellipse-03.svg
http://web-platform-tests.live/svg/shapes/ellipse-05.svg
http://web-platform-tests.live/svg/shapes/ellipse-06.svg
http://web-platform-tests.live/svg/shapes/ellipse-07.svg
http://web-platform-tests.live/svg/shapes/ellipse-08.svg

[Said Abou-Hallawa]

http://web-platform-tests.live/svg/shapes/rx-ry-not-inherited.svg

[Said Abou-Hallawa]

Created attachment 379046 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 379046 [details]
Patch

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

Nice tests, thanks Said.

> LayoutTests/ChangeLog:3
> +        [SVG2]: Add auto behavior for rx and ry to the SVG <ellipse> and<rect> elements

and<rect> -> and <rect>

> LayoutTests/ChangeLog:8
> +        Skip the tests with dynmaic chnages till webkit.org/b/201918 is fixed.

typo: dynamic changes.
Maybe add one sentence regarding the new tests?

[Said Abou-Hallawa]

Created attachment 379050 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 379046 [details]
Patch

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

>> LayoutTests/ChangeLog:3
>> +        [SVG2]: Add auto behavior for rx and ry to the SVG <ellipse> and<rect> elements
> 
> and<rect> -> and <rect>

Fixed. I just realized the space key in my keyboard is malfunctioning. It got stuck many times while typing this comment.

>> LayoutTests/ChangeLog:8
>> +        Skip the tests with dynmaic chnages till webkit.org/b/201918 is fixed.
> 
> typo: dynamic changes.
> Maybe add one sentence regarding the new tests?

The typo is fixed and comments were added regarding the new tests.

[Nikolas Zimmermann]

Comment on attachment 379050 [details]
Patch

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

Sorry, just found another typo :/ For me the code changes look just fine.

> Source/WebCore/ChangeLog:8
> +        The sepcs is: https://www.w3.org/TR/SVG2/geometry.html#RxProperty.

sepcs, spec :-)

> Source/WebCore/ChangeLog:11
> +        parse LengthOrAuto for these  properties. Handle the case if one of them

s/these  /these /

[Simon Fraser (smfr)]

Comment on attachment 379050 [details]
Patch

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

> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:92
> +    Length rx = style().svgStyle().rx().isAuto() ? style().svgStyle().ry() : style().svgStyle().rx();

Maybe avoid reading style().svgStyle().rx() twice. 

Length rx = style().svgStyle().rx();
if (rx.isAuto()
  rx = style().svgStyle().ry();

[Said Abou-Hallawa]

Created attachment 379147 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 379147 [details]:

imported/w3c/web-platform-tests/websockets/bufferedAmount-unchanged-by-sync-xhr.any.worker.html bug 202003 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 379147 [details]
Patch

Clearing flags on attachment: 379147

Committed r250103: <https://trac.webkit.org/changeset/250103>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55532838>

[Darin Adler]

Comment on attachment 379147 [details]
Patch

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

> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:98
> +    Length rx = style().svgStyle().rx();
> +    if (rx.isAuto())
> +        rx = style().svgStyle().ry();
> +    
> +    Length ry = style().svgStyle().ry();
> +    if (ry.isAuto())
> +        ry = style().svgStyle().rx();

How about this alternative version?

    Length rx = style().svgStyle().rx();
    Length ry = style().svgStyle().ry();
    if (rx.isAuto())
        rx = ry;
    else if (ry.isAuto())
        ry = rx;

[Said Abou-Hallawa]

Comment on attachment 379147 [details]
Patch

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

>> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:98
>> +        ry = style().svgStyle().rx();
> 
> How about this alternative version?
> 
>     Length rx = style().svgStyle().rx();
>     Length ry = style().svgStyle().ry();
>     if (rx.isAuto())
>         rx = ry;
>     else if (ry.isAuto())
>         ry = rx;

I agree.

-- In the worst case scenario, which happens when rx and ry are both 'auto', my solution will calculate rx/ry four times. It will also call the Length::operator==() four times.
-- Your solution will calculate rx/ry two times and will call Length::operator==() three times.
-- How about this?

    Length rx = style().svgStyle().rx();
    Length ry = style().svgStyle().ry();
    m_radii = FloatSize(
        lengthContext.valueForLength(rx.isAuto() ? ry : rx, SVGLengthMode::Width),
        lengthContext.valueForLength(ry.isAuto() ? rx : ry, SVGLengthMode::Height));

This will calculate rx/ry two times and will call Length::operator==() two times.

[Said Abou-Hallawa]

Reopening to attach new patch.

[Said Abou-Hallawa]

Created attachment 379243 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 379243 [details]
Patch

Clearing flags on attachment: 379243

Committed r250147: <https://trac.webkit.org/changeset/250147>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.