Bug 72401 - [SVG2]: Implement support for the 'pathLength' attribute
Summary: [SVG2]: Implement support for the 'pathLength' attribute
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks: 191292 200143
  Show dependency treegraph
 
Reported: 2011-11-15 11:15 PST by Jonathan Watt
Modified: 2020-06-09 12:26 PDT (History)
22 users (show)

See Also:


Attachments
testcase (357 bytes, image/svg+xml)
2011-11-15 11:16 PST, Jonathan Watt
no flags Details
Another test case (956 bytes, image/svg+xml)
2014-12-05 19:57 PST, Said Abou-Hallawa
no flags Details
Patch (131.59 KB, patch)
2019-11-19 13:45 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (131.59 KB, patch)
2020-01-15 11:37 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (131.58 KB, patch)
2020-01-15 16:39 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Watt 2011-11-15 11:15:05 PST
It seems like WebKit lacks support for the <path> element's 'pathLength' attribute:

http://www.w3.org/TR/SVG11/paths.html#PathLengthAttribute
Comment 1 Jonathan Watt 2011-11-15 11:16:50 PST
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).
Comment 2 Jonathan Watt 2011-11-15 11:23:05 PST
Note pathLength should affect offsets along a textPath too.
Comment 3 Dirk Schulze 2014-05-12 06:57:51 PDT
(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?
Comment 4 Jonathan Watt 2014-05-12 07:06:10 PDT
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.
Comment 5 Dirk Schulze 2014-05-12 09:01:58 PDT
(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?
Comment 6 Cameron McCormack 2014-06-01 18:36:13 PDT
(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.
Comment 7 Said Abou-Hallawa 2014-12-05 19:57:52 PST
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.
Comment 8 Radar WebKit Bug Importer 2014-12-05 19:58:31 PST
<rdar://problem/19165803>
Comment 9 Said Abou-Hallawa 2019-07-25 13:57:55 PDT
Failed WPT tests: http://web-platform-tests.live/svg/shapes/line-dasharray.svg
Comment 13 Said Abou-Hallawa 2019-11-19 13:45:44 PST
Created attachment 383900 [details]
Patch
Comment 14 Kyle Bavender 2020-01-15 07:33:48 PST
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.
Comment 15 Simon Fraser (smfr) 2020-01-15 10:06:48 PST
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.
Comment 16 Said Abou-Hallawa 2020-01-15 11:22:43 PST
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.
Comment 17 Said Abou-Hallawa 2020-01-15 11:37:19 PST
Created attachment 387813 [details]
Patch
Comment 18 WebKit Commit Bot 2020-01-15 15:22:11 PST
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
Comment 19 Said Abou-Hallawa 2020-01-15 16:39:33 PST
Created attachment 387870 [details]
Patch
Comment 20 WebKit Commit Bot 2020-01-15 17:39:39 PST
The commit-queue encountered the following flaky tests while processing attachment 387870 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org)
The commit-queue is continuing to process your patch.
Comment 21 WebKit Commit Bot 2020-01-15 17:40:21 PST
Comment on attachment 387870 [details]
Patch

Clearing flags on attachment: 387870

Committed r254657: <https://trac.webkit.org/changeset/254657>
Comment 22 WebKit Commit Bot 2020-01-15 17:40:23 PST
All reviewed patches have been landed.  Closing bug.
Comment 23 Jonathan Bedard 2020-01-16 15:47:44 PST
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
Comment 24 Said Abou-Hallawa 2020-01-16 16:42:31 PST
(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.
Comment 25 Kyle Bavender 2020-01-23 05:02:05 PST
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?
Comment 26 Simon Fraser (smfr) 2020-01-23 13:39:48 PST
This patch landed in r254657, STP 99 covers r253789-r254696 so it should be included.