Bug 72401 – [SVG2]: Implement support for the 'pathLength' attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Jonathan Watt]

It seems like WebKit lacks support for the <path> element's 'pathLength' attribute:

http://www.w3.org/TR/SVG11/paths.html#PathLengthAttribute

[Jonathan Watt]

Created attachment 115207 [details]
testcase

The dashing of the two lines in this testcase should NOT be the same (the values in the stroke-dasharray of the second path should be scaled down by a half, since the user specified 'pathLength' is twice the actual length of the path).

[Jonathan Watt]

Note pathLength should affect offsets along a textPath too.

[Dirk Schulze]

(In reply to comment #1)
> Created an attachment (id=115207) [details]
> testcase
> 
> The dashing of the two lines in this testcase should NOT be the same (the values in the stroke-dasharray of the second path should be scaled down by a half, since the user specified 'pathLength' is twice the actual length of the path).

We do not support it. But the specification seems vague as well. Does pathLength mean we change the offset of markers too?

[Jonathan Watt]

Markers are not positioned at a distance specified by the author. They are on the vertices, and should stay on the vertices regardless of pathLength.

[Dirk Schulze]

(In reply to comment #4)
> Markers are not positioned at a distance specified by the author. They are on the vertices, and should stay on the vertices regardless of pathLength.

Beside glyph/element positioning and dash array, what else is effected? line caps?

[Cameron McCormack (:heycam)]

(In reply to comment #4)
> Markers are not positioned at a distance specified by the author. They are on the vertices, and should stay on the vertices regardless of pathLength.

Although the new positioned markers in SVG 2 should be affected by pathLength.

(In reply to comment #5)
> Beside glyph/element positioning and dash array, what else is effected? line caps?

The DOM methods on SVGPathElement that take an offset along the path, too.

[Said Abou-Hallawa]

Created attachment 242696 [details]
Another test case

This test case is originally from Mozilla test suite for SVG. I changed it a little and I modified the comment accordingly.

[Radar WebKit Bug Importer]

<rdar://problem/19165803>

[Said Abou-Hallawa]

Failed WPT tests: http://web-platform-tests.live/svg/shapes/line-dasharray.svg

[Said Abou-Hallawa]

More failed WPT tests:

http://web-platform-tests.live/svg/shapes/reftests/pathlength-001.svg
http://web-platform-tests.live/svg/shapes/reftests/pathlength-002.svg

[Said Abou-Hallawa]

https://wpt.fyi/results/svg/path/distance/pathLength-positive.svg?label=master&label=experimental

[Said Abou-Hallawa]

https://wpt.fyi/results/svg/path/distance/pathLength-positive.svg?label=master&label=experimental
https://wpt.fyi/results/svg/path/distance/pathLength-zero-percentage.svg?label=master&label=experimental
https://wpt.fyi/results/svg/path/distance/pathLength-zero.svg?label=master&label=experimental

[Said Abou-Hallawa]

Created attachment 383900 [details]
Patch

[Kyle Bavender]

Came here via https://css-tricks.com/a-trick-that-makes-drawing-svg-lines-way-easier/ , which recommends use of pathLength (which appears to work in Chrome). FWIW, this is a compatibility consideration.

[Simon Fraser (smfr)]

Comment on attachment 383900 [details]
Patch

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

> Source/WebCore/rendering/svg/RenderSVGTextPath.h:38
> +    const SVGLengthValue& startOffset() const;

We seem to usually return SVGLengthValue by value.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:421
> +    Element* element = renderer.element();
> +    ASSERT(element);

Maybe we should be defensive and return if element is null.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:446
> +            if (float pathLength = downcast<SVGGeometryElement>(element)->pathLength())
> +                scaleFactor = downcast<RenderSVGShape>(renderer).getTotalLength() / pathLength;

Are we sure that pathLength is always non-zero here? Otherwise you have a divide by zero.

> Source/WebCore/rendering/svg/SVGTextLayoutEngine.cpp:178
> +        m_textPathStartOffset = startOffset.valueInSpecifiedUnits() * m_textPathLength / 100;

Why / 100 ?

> Source/WebCore/rendering/svg/SVGTextLayoutEngine.cpp:184
> +            if (float pathLength = tragetElement->pathLength())
> +                m_textPathStartOffset *= m_textPathLength / pathLength;

Same comment about zero pathLength.

[Said Abou-Hallawa]

Comment on attachment 383900 [details]
Patch

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

>> Source/WebCore/rendering/svg/RenderSVGTextPath.h:38
>> +    const SVGLengthValue& startOffset() const;
> 
> We seem to usually return SVGLengthValue by value.

I checked the code, and I found out most of the time similar functions return const reference to SVGLengthValue.

>> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:421
>> +    ASSERT(element);
> 
> Maybe we should be defensive and return if element is null.

Done.

>> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:446
>> +                scaleFactor = downcast<RenderSVGShape>(renderer).getTotalLength() / pathLength;
> 
> Are we sure that pathLength is always non-zero here? Otherwise you have a divide by zero.

Yes since we check if (float pathLength = ...) before this statement. 

In the comment above, I was referring to this section in the spec: 

"A value of zero is valid and must be treated as a scaling factor of infinity. A value of zero scaled infinitely must remain zero, while any non-percentage value greater than zero must become +Infinity."

But since unspecified SVG properties fall back to the default (which is zero for pathLength) I was not able to differentiate between this case and having pathLength="0".

>> Source/WebCore/rendering/svg/SVGTextLayoutEngine.cpp:178
>> +        m_textPathStartOffset = startOffset.valueInSpecifiedUnits() * m_textPathLength / 100;
> 
> Why / 100 ?

Because SVGLengthValue stores the percentage in the range [0..100]; see SVGLengthValue::valueAsPercentage(). For clarity, I changed this statement to

    m_textPathStartOffset = startOffset.valueAsPercentage() * m_textPathLength;

Since this is what RenderSVGTextPath::startOffset() used to return before changing it to return an SVGLengthValue.

[Said Abou-Hallawa]

Created attachment 387813 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 387813 [details]
Patch

Rejecting attachment 387813 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 387813, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/13305069

[Said Abou-Hallawa]

Created attachment 387870 [details]
Patch

[WebKit Commit Bot]

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

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 387870 [details]
Patch

Clearing flags on attachment: 387870

Committed r254657: <https://trac.webkit.org/changeset/254657>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jonathan Bedard]

This looks like it broke a number of our non-apple ports:
https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fsvg%2Fimport%2Fpaths-dom-01-f-manual.svg

[Said Abou-Hallawa]

(In reply to Jonathan Bedard from comment #23)
> This looks like it broke a number of our non-apple ports:
> https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-
> platform-tests%2Fsvg%2Fimport%2Fpaths-dom-01-f-manual.svg

The expected results of these tests need to be rebase-lined like what I did for macOS and iOS.

[Kyle Bavender]

I have just tried out Safari Technology Preview 99 ("WebKit revisions 253789-254696") but I am not seeing improvement in a case that was mentioned ( https://css-tricks.com/a-trick-that-makes-drawing-svg-lines-way-easier/ — compare the behavior of the Pen to Chrome or Firefox). Perhaps there is some other issue in play (the pen also seems to have a difference in how the stroke-dashoffset is rendered)? Or is it possible this fix for pathLength has not yet landed in Technology Preview?

[Simon Fraser (smfr)]

This patch landed in r254657, STP 99 covers r253789-r254696 so it should be included.