1388931 - Unship SVGPathSeg APIs

Status: RESOLVED FIXED

(Core :: SVG, task, P2)

Description

[Rick Byers]

User Agent: Mozilla/5.0 (X11; CrOS x86_64 9592.71.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.80 Safari/537.36

Steps to reproduce:

The SVGPathSeg interfaces were removed from the SVG specification.  We removed them from Chrome over a year ago: https://groups.google.com/a/chromium.org/d/msg/blink-dev/EDC3cBg9mCU/OvElJgOWCgAJ

There were a small number of upset users, but most (perhaps all) were happy to switch to using the polyfill we provided: https://github.com/progers/pathseg

Comment 1

[Rick Byers]

Note this is already tested by https://w3c-test.org/svg/historical.html (chrome passes 41/41, firefox 29/41 - see http://wpt.fyi)

Comment 2

[Jonathan Watt [:jwatt]]

Yes, we should do this.

Comment 3

[Mike Taylor [:miketaylr]]

Tom, would you be interested in working on this?

Comment 4

[Thomas Wisniewski]

Sure, but when I last took a look this proved to be quite a lot of work, so I'm not sure how quickly I could get it done. It's been on my backlog for a few months now.

Comment 5

[Robert Longson [:longsonr]]

We really should consider replacing this with the new API https://www.w3.org/TR/svg-paths/#InterfaceSVGPathData rather than just removing the old one. The new API doesn't seem to allow getting animated/base values separately though.

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson [:longsonr] from comment #5)
> We really should consider replacing this with the new API
> https://www.w3.org/TR/svg-paths/#InterfaceSVGPathData rather than just
> removing the old one.

As far as I'm aware, there are no plans to add support for the new interface to Chromium or Edge. The Chromium bug to remove support for the old interface mentions only a polyfill for the new interface, not an actual intent to implement it:

https://bugs.chromium.org/p/chromium/issues/detail?id=539385#c22

Before putting work into implementing the new interface I think the first step would be to find out whether or not Chromium and Edge plan on implementing.

At any rate, since we're now the only implementation left with an implementation of the old interface I think we should remove it regardless.

> The new API doesn't seem to allow getting
> animated/base values separately though.

In principle, the SVGPathDataSettings dictionary that can be passed to getPathData could be extended to allow for that.

Comment 7

[Rick Byers]

(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #6)
> Before putting work into implementing the new interface I think the first
> step would be to find out whether or not Chromium and Edge plan on
> implementing.

We (chromium) have no such plans at the moment as far as I know.

Comment 8

[Jonathan Watt [:jwatt]]

Thank for commenting, Rick.

Comment 9

[itsupport]

(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #6)
> (In reply to Robert Longson [:longsonr] from comment #5)
> > We really should consider replacing this with the new API
> > https://www.w3.org/TR/svg-paths/#InterfaceSVGPathData rather than just
> > removing the old one.
> 
> As far as I'm aware, there are no plans to add support for the new interface
> to Chromium or Edge. The Chromium bug to remove support for the old
> interface mentions only a polyfill for the new interface, not an actual
> intent to implement it:
> 
> Before putting work into implementing the new interface I think the first
> step would be to find out whether or not Chromium and Edge plan on
> implementing.
> 
> At any rate, since we're now the only implementation left with an
> implementation of the old interface I think we should remove it regardless.

Why remove support and break existing pages, just because the Chrome developers were short-sighted enough to do so?
Implement the new interface first, then deprecate the old one if you must.

I only discovered the issue with Chrome when I opened a page in it instead of Firefox and found it didn't work. 
When Chrome removed support, 
> There were a small number of upset users, but most (perhaps all) were happy to switch to...
Firefox

Comment 10

[Jonathan Watt [:jwatt]]

(In reply to itsupport from comment #9)
> Why remove support and break existing pages, just because the Chrome
> developers were short-sighted enough to do so?

Because it adds significant complexity to the implementation of SVG <path>, and because something like 1 in 10,000,000 pages were using it according to their metrics. They were simply making a pragmatic decision.

Comment 11

[Jonathan Watt [:jwatt]]

Patrick, there are a bunch of tests that use the 'pathSegList' property from SVGPathElement under DevTools, but it doesn't look like the DevTools itself actually uses it (any more?). Can the SVGPathSeg APIs (and these tests) be removed without affecting DevTools?

https://dxr.mozilla.org/mozilla-central/search?q=pathSegList+path%3Adevtools

Comment 12

[Jonathan Watt [:jwatt]]

Brian, please be aware of this bug, since it affects SMIL. We will also need to disable/remove/polyfill the test at:

https://dxr.mozilla.org/mozilla-central/source/dom/svg/test/test_pathAnimInterpolation.xhtml

Comment 13

[Patrick Brosset <:pbro>]

(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #11)
> Patrick, there are a bunch of tests that use the 'pathSegList' property from
> SVGPathElement under DevTools, but it doesn't look like the DevTools itself
> actually uses it (any more?). Can the SVGPathSeg APIs (and these tests) be
> removed without affecting DevTools?
> 
> https://dxr.mozilla.org/mozilla-central/search?q=pathSegList+path%3Adevtools
Hm, I can't remember exactly. Daisuke would remember better I think. Did we use to use the pathSegList API in the animationinspector's code, or was that always only used in tests?

Comment 14

[Jonathan Watt [:jwatt]]

FWIW the Chromium bug to add the new API is:

https://bugs.chromium.org/p/chromium/issues/detail?id=545467

Comment 15

[Daisuke Akatsuka (:daisuke)]

(In reply to Patrick Brosset <:pbro> from comment #13)
> (In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #11)
> > Patrick, there are a bunch of tests that use the 'pathSegList' property from
> > SVGPathElement under DevTools, but it doesn't look like the DevTools itself
> > actually uses it (any more?). Can the SVGPathSeg APIs (and these tests) be
> > removed without affecting DevTools?
> > 
> > https://dxr.mozilla.org/mozilla-central/search?q=pathSegList+path%3Adevtools
> Hm, I can't remember exactly. Daisuke would remember better I think. Did we
> use to use the pathSegList API in the animationinspector's code, or was that
> always only used in tests?

We are using pathSegList in the tests only. (New animation inspector is as well)
So, we can simply replace to the polyfill instead, I think.

Comment 16

[Brian Birtles (:birtles)]

(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #12)
> Brian, please be aware of this bug, since it affects SMIL. We will also need
> to disable/remove/polyfill the test at:
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/svg/test/
> test_pathAnimInterpolation.xhtml

Ok, sounds fine to me. Implementing the new interface would be good and looks straightforward (although why is SVGPathSegment an interface and not a dictionary?) but it doesn't need to block this.

Comment 17

[Jonathan Watt [:jwatt]]

Attachment: Bug 1388931 part 1 - Remove the main WebIDL entry points into SVGPathElement data.

Review commit: https://reviewboard.mozilla.org/r/215362/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215362/

Comment 18

[Jonathan Watt [:jwatt]]

Attachment: Bug 1388931 part 2 - Gut the WebIDL interfaces related to SVG paths.

Review commit: https://reviewboard.mozilla.org/r/215364/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215364/

Comment 19

[Jonathan Watt [:jwatt]]

Attachment: Bug 1388931 part 3 - Remove all the code to implement SVG path data DOM interfaces.

Review commit: https://reviewboard.mozilla.org/r/215366/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215366/

Comment 20

[Jonathan Watt [:jwatt]]

Attachment: Bug 1388931 part 4 - Remove the tests that test SVG path data DOM interfaces.

Review commit: https://reviewboard.mozilla.org/r/215368/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215368/

Comment 21

[Jonathan Watt [:jwatt]]

Attachment: Bug 1388931 part 5 - Fix up DevTools test that rely on the now removed SVG path data DOM.

Review commit: https://reviewboard.mozilla.org/r/215370/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215370/

Comment 22

[Jonathan Watt [:jwatt]]

(Mostly done, but still figuring out the DevTools patches.)

Comment 23

[Jonathan Watt [:jwatt]]

Comment on attachment 8945129 [details]
Bug 1388931 part 1 - Remove the main WebIDL entry points into SVGPathElement data.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215362/diff/1-2/

Comment 24

[Jonathan Watt [:jwatt]]

Comment on attachment 8945130 [details]
Bug 1388931 part 2 - Gut the WebIDL interfaces related to SVG paths.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215364/diff/1-2/

Comment 25

[Jonathan Watt [:jwatt]]

Comment on attachment 8945131 [details]
Bug 1388931 part 3 - Remove all the code to implement SVG path data DOM interfaces.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215366/diff/1-2/

Comment 26

[Jonathan Watt [:jwatt]]

Comment on attachment 8945132 [details]
Bug 1388931 part 4 - Remove the tests that test SVG path data DOM interfaces.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215368/diff/1-2/

Comment 27

[Jonathan Watt [:jwatt]]

Comment on attachment 8945133 [details]
Bug 1388931 part 5 - Fix up DevTools test that rely on the now removed SVG path data DOM.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215370/diff/1-2/

Comment 28

[Robert Longson [:longsonr]]

Comment on attachment 8945129 [details]
Bug 1388931 part 1 - Remove the main WebIDL entry points into SVGPathElement data.

https://reviewboard.mozilla.org/r/215362/#review221064

Comment 29

[Robert Longson [:longsonr]]

Comment on attachment 8945132 [details]
Bug 1388931 part 4 - Remove the tests that test SVG path data DOM interfaces.

https://reviewboard.mozilla.org/r/215368/#review221066

Comment 30

[Robert Longson [:longsonr]]

Comment on attachment 8945131 [details]
Bug 1388931 part 3 - Remove all the code to implement SVG path data DOM interfaces.

https://reviewboard.mozilla.org/r/215366/#review221074

::: dom/svg/SVGAnimatedPathSegList.cpp:36
(Diff revision 2)
>    // nsSVGElement::ParseAttribute under Element::SetAttr,
>    // which takes care of notifying.
>  
>    nsresult rv2 = mBaseVal.CopyFrom(newBaseValue);
> -  if (NS_FAILED(rv2)) {
> -    // Attempting to increase mBaseVal's length failed (mBaseVal is left
> +
> +  return NS_FAILED(rv2)? rv2 : rv;

space before ?

Comment 31

[Robert Longson [:longsonr]]

have we announced an intent to unship on https://groups.google.com/forum/#!forum/mozilla.dev.platform ?

Comment 32

[Andrea Marchesini [:baku]]

Comment on attachment 8945130 [details]
Bug 1388931 part 2 - Gut the WebIDL interfaces related to SVG paths.

https://reviewboard.mozilla.org/r/215364/#review221166

Comment 33

[Andrea Marchesini [:baku]]

Comment on attachment 8945129 [details]
Bug 1388931 part 1 - Remove the main WebIDL entry points into SVGPathElement data.

https://reviewboard.mozilla.org/r/215362/#review221168

Comment 34

[Robert Longson [:longsonr]]

There is one other problem with this. Chrome have changed their SMIL implementation so that it updates attributes, we haven't. Once we do this there will be no way to access SMIL animated values, this is not the case in Chrome if you use the polyfill.

Comment 35

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson [:longsonr] from comment #34)
> There is one other problem with this. Chrome have changed their SMIL
> implementation so that it updates attributes, we haven't.

This is a pain - thanks for pointing this out. Okay, I'm going to spin off a separate bug to land a subset of the 'part 1' patch where I'll just remove the DOM API that makes SVGPathSegList mutable. We can then follow up on the rest of this after that.

Comment 36

[Jonathan Watt [:jwatt]]

Polyfill for SVGPathSeg:
https://github.com/progers/pathseg

Polyfill for getPathData:
https://github.com/jarek-foksa/path-data-polyfill.js

Comment 37

[Mike Taylor [:miketaylr]]

https://github.com/webcompat/web-bugs/issues/23877 reports that parkrun.com is broken because we have parts of these APIs still (they have a polyfill and some other methods that are defined on the prototype of SVGPathSegList, but only if SVGPathSegList doesn't exist.

Comment 38

[Mike Taylor [:miketaylr]]

Do we have finishing out this bug on our radar, jwatt?

Comment 39

[Robert Longson [:longsonr]]

(In reply to Mike Taylor [:miketaylr] from comment #38)

Do we have finishing out this bug on our radar, jwatt?

Comment 34 isn't easy to solve. However newer versions of the polyfill in comment 36 do work with Firefox, can't parkrun.com use that polyfill?

Comment 40

[Mike Taylor [:miketaylr]]

I've emailed them to ask if they can update to the latest version of their polyfill.

Comment 41

[Jonathan Watt [:jwatt]]

Mike, did they get back to you?

Comment 42

[Mike Taylor [:miketaylr]]

(In reply to Jonathan Watt [:jwatt] from comment #41)

Mike, did they get back to you?

Not yet (which doesn't really suprise me). And I just verified they didn't update the polyfill in question.

Adam, do you think you could try to find a better contact (than their contact form)?

Comment 43

[Adam Stevenson [:adamopenweb]]

Let's try their founder and the head of communications on LinkedIn.

Comment 44

[Firefox Bug Husbandry Bot]

Migrating Webcompat whiteboard priorities to project flags. See bug 1547409.

Comment 45

[Firefox Bug Husbandry Bot]

See bug 1547409. Migrating whiteboard priority tags to program flags.

Comment 47

[Cameron McCormack (:heycam)]

(In reply to Mike Taylor [:miketaylr] from comment #37)

https://github.com/webcompat/web-bugs/issues/23877 reports that parkrun.com is broken because we have parts of these APIs still (they have a polyfill and some other methods that are defined on the prototype of SVGPathSegList, but only if SVGPathSegList doesn't exist.

Ciprian checked in again on that issue and it has been fixed.

(In reply to Robert Longson [:longsonr] from comment #34)

There is one other problem with this. Chrome have changed their SMIL implementation so that it updates attributes, we haven't. Once we do this there will be no way to access SMIL animated values, this is not the case in Chrome if you use the polyfill.

Might be worth checking whether this is still the case, or filing an crbug.com issue asking them to consider not doing that, since it's clearly not spec conforming.

Comment 48

[Boris Chiou [:boris]]

(In reply to Cameron McCormack (:heycam) from comment #47)

(In reply to Robert Longson [:longsonr] from comment #34)

There is one other problem with this. Chrome have changed their SMIL implementation so that it updates attributes, we haven't. Once we do this there will be no way to access SMIL animated values, this is not the case in Chrome if you use the polyfill.

Might be worth checking whether this is still the case, or filing an crbug.com issue asking them to consider not doing that, since it's clearly not spec conforming.

Gecko doesn't update the attribute for SMIL, but we map the d attribute to CSS d property, so if SMIL updates the d attribute, it should also update the CSS d property as well, and so it's still possible to get the SMIL animated values by the CSSOM (e.g. getComputedStyle(div).d). This bug blocks Bug 1744599, so I would like to finish this bug first. If we would like to do a similar thing as Chrome, how about filing a separate bug to change the SMIL implementation so we also update attributes?

Comment 49

[Robert Longson [:longsonr]]

We should check first that Chrome still does work that way. It's been 4 years, they may have changed.

Comment 50

[Boris Chiou [:boris]]

(In reply to Robert Longson [:longsonr] from comment #49)

We should check first that Chrome still does work that way. It's been 4 years, they may have changed.

I did a quick check by <animate>:

<svg viewBox="0 0 20 10">
  <path fill="none" stroke="lightgrey" d="M2,5 H 10 Z" id="animTarget">
    <animate attributeName="d"
             from="M2,5 H5Z" to="M2,5 H15Z"
             dur="2s"
             repeatCount="2" />
  </path>
</svg>

And dump the log by things like this:

    window.setTimeout(()=>{
      console.log("style: " + window.getComputedStyle(animTarget).d);
      console.log("attr:  " + animTarget.getAttribute("d"));
    }, 1000);

At 1s, the CSS d property is "path("M 2 5 H 9.98042 Z")", and the SVG d attribute is still "M2,5 H 10 Z". Does this mean Chrome doesn't update the attribute anymore or do I use the wrong way? Thanks.

Comment 51

[Robert Longson [:longsonr]]

As far as I remember (it was 4 years ago) that's roughly what I was doing so I guess they don't and we can stop worrying about that. We just need to ensure that we do what Chrome does now.

Comment 52

[Boris Chiou [:boris]]

(In reply to Robert Longson [:longsonr] from comment #51)

As far as I remember (it was 4 years ago) that's roughly what I was doing so I guess they don't and we can stop worrying about that. We just need to ensure that we do what Chrome does now.

Thanks for these feedback. Let me drop these APIs. :)

Comment 53

[Boris Chiou [:boris]]

Attachment: Bug 1388931 - Unship SVGPathSeg APIs behind a preference.

This patch removes the following APIs:

1. SVGAnimatedPathData
2. SVGPathSeg
3. SVGPathSegList
4. SVGPathElement::getPathSegAtLength()

Comment 54

[Boris Chiou [:boris]]

Attachment: Bug 1388931 - Update devtool tests which using SVGPathSeg APIs.

Basically, I would like to keep these tests, and change the way to check
the d property. However, the path strings returned from d propety are
complicated, so we need a better way to parse the path string and check
if it goes through the vertices we expected. Now we mark these tests
failed, and leave them in Bug 1745060.

Note: the patch should be merged into the previous one, but I use a
separate patch for review. Will merge this into the previous one once we
get accepted.

Comment 55

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/78dad725f8a2
Unship SVGPathSeg APIs behind a preference. r=emilio

Comment 56

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/31989 for changes under testing/web-platform/tests

Comment 57

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/78dad725f8a2

Comment 58

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 59

[Hamish Willee]

FF97 Docs work for this can be tracked in https://github.com/mdn/content/issues/11597

Can you please confirm what is affected by this change? By inspection of IDL I think it just the following that are hidden behind dom.svg.pathSeg.enabled:

• SVGPathSegList (including .numberOfItems, getItem() and length.
• SVGPathElement.getPathSegAtLength()
• SVGAnimatedPathData .pathSegList and .animatedPathSegList

Is that the full set?

Further MDN lists the following Seg APIs but does not document them. The show up in searchfox, but I just want to make sure that they are still something we should mention/evetually document
SVGPathSeg, SVGPathSegClosePath, SVGPathSegMovetoAbs, SVGPathSegMovetoRel, SVGPathSegLinetoAbs, SVGPathSegLinetoRel, SVGPathSegCurvetoCubicAbs, SVGPathSegCurvetoCubicRel, SVGPathSegCurvetoQuadraticAbs, SVGPathSegCurvetoQuadraticRel, SVGPathSegArcAbs
SVGPathSegArcRel, SVGPathSegLinetoHorizontalAbs, SVGPathSegLinetoHorizontalRel , SVGPathSegLinetoVerticalAbs, SVGPathSegLinetoVerticalRel
SVGPathSegCurvetoCubicSmoothAbs, SVGPathSegCurvetoCubicSmoothRel, SVGPathSegCurvetoQuadraticSmoothAbs, SVGPathSegCurvetoQuadraticSmoothRel, SVGPathSegList,

Comment 61

[Angelo Borsotti]

(In reply to Jonathan Watt [:jwatt] from comment #10)

(In reply to itsupport from comment #9)

Why remove support and break existing pages, just because the Chrome
developers were short-sighted enough to do so?

Because it adds significant complexity to the implementation of SVG <path>,
and because something like 1 in 10,000,000 pages were using it according to
their metrics. They were simply making a pragmatic decision.

I wonder how you can tell that there is only one such page.
But, even supposing so, suppose that is a page needed in a surgery hospital,
or in a nuclear plant, does that "pragmatic decision" take into account the
damage of breaking it? I guess that such decisions should take all the
consequences into account, and not only the bare usage statistics.

Comment 62

[Rick Byers]

(In reply to Angelo Borsotti from comment #61)

(In reply to Jonathan Watt [:jwatt] from comment #10)

(In reply to itsupport from comment #9)

Why remove support and break existing pages, just because the Chrome
developers were short-sighted enough to do so?

Because it adds significant complexity to the implementation of SVG <path>,
and because something like 1 in 10,000,000 pages were using it according to
their metrics. They were simply making a pragmatic decision.

I wonder how you can tell that there is only one such page.

Chrome collects anonymized usage stats from the wild, you can see some here: https://chromestatus.com/metrics/feature/popularity

But, even supposing so, suppose that is a page needed in a surgery hospital,
or in a nuclear plant, does that "pragmatic decision" take into account the
damage of breaking it? I guess that such decisions should take all the
consequences into account, and not only the bare usage statistics.

Absolutely that's a consideration, we call that "ease of adaptation" - https://bit.ly/blink-compat. In this case we provided a JavaScript library that worked perfectly as a drop-in replacement in every case we found: https://groups.google.com/a/chromium.org/g/blink-dev/c/EDC3cBg9mCU/m/LDYo5CjfBAAJ

Comment 63

[Angelo Borsotti]

I searched the web for a replacement, and did not find the one mentioned in the Google groups you indicated:
https://groups.google.com/a/chromium.org/g/blink-dev/c/EDC3cBg9mCU/m/LDYo5CjfBAAJ

The only one that one gets from google search is the path-data-polyfill.js, which is an implementation
of the new SVG API: SVGPathData.
This requires to change the existing js code.

If you did not point me to that google group I would have never known that there is such a replacement.

P.S for the ones that have the same problem, the replacement seems to be in:
https://github.com/progers/pathseg/blob/master/pathseg.js

Comment 64

[Rick Byers]

(In reply to Angelo Borsotti from comment #63)

I searched the web for a replacement, and did not find the one mentioned in the Google groups you indicated:
https://groups.google.com/a/chromium.org/g/blink-dev/c/EDC3cBg9mCU/m/LDYo5CjfBAAJ

The only one that one gets from google search is the path-data-polyfill.js, which is an implementation
of the new SVG API: SVGPathData.
This requires to change the existing js code.

If you did not point me to that google group I would have never known that there is such a replacement.

P.S for the ones that have the same problem, the replacement seems to be in:
https://github.com/progers/pathseg/blob/master/pathseg.js

It's a good point that we should probably do more to make this findable, thanks for the feedback. But also note that I provided the GitHub URL in the opening comment of this issue :-)