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>
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
http://web-platform-tests.live/svg/shapes/rx-ry-not-inherited.svg
Created attachment 379046 [details] Patch
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?
Created attachment 379050 [details] Patch
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.
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 /
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();
Created attachment 379147 [details] Patch
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: youennf@gmail.com) The commit-queue is continuing to process your patch.
Comment on attachment 379147 [details] Patch Clearing flags on attachment: 379147 Committed r250103: <https://trac.webkit.org/changeset/250103>
All reviewed patches have been landed. Closing bug.
<rdar://problem/55532838>
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;
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.
Reopening to attach new patch.
Created attachment 379243 [details] Patch
Comment on attachment 379243 [details] Patch Clearing flags on attachment: 379243 Committed r250147: <https://trac.webkit.org/changeset/250147>