Closed Bug 1388931 Opened 7 years ago Closed 2 years ago

Unship SVGPathSeg APIs

Categories

(Core :: SVG, task, P2)

56 Branch
task

Tracking

()

RESOLVED FIXED
97 Branch
Webcompat Priority ?
Tracking Status
firefox57 --- wontfix
firefox97 --- fixed

People

(Reporter: rbyers, Assigned: boris)

References

(Blocks 4 open bugs)

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [webcompat], [wptsync upstream])

Attachments

(6 files, 1 obsolete file)

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
Component: Untriaged → SVG
Product: Firefox → Core
Note this is already tested by https://w3c-test.org/svg/historical.html (chrome passes 41/41, firefox 29/41 - see http://wpt.fyi)
Flags: needinfo?(jwatt)
Priority: -- → P2
Whiteboard: [webcompat]
Yes, we should do this.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(jwatt)
Tom, would you be interested in working on this?
Flags: needinfo?(wisniewskit)
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.
Flags: needinfo?(wisniewskit)
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.
Flags: needinfo?(bmo)
Flags: needinfo?(bmo) → needinfo?(cku)
(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.
(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.
Thank for commenting, Rick.
(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
(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.
Assignee: nobody → jwatt
Flags: needinfo?(cku)
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
Flags: needinfo?(pbrosset)
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
Flags: needinfo?(bbirtles)
(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?
Flags: needinfo?(pbrosset) → needinfo?(dakatsuka)
FWIW the Chromium bug to add the new API is:

https://bugs.chromium.org/p/chromium/issues/detail?id=545467
(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.
Flags: needinfo?(dakatsuka)
(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.
Flags: needinfo?(bbirtles)
(Mostly done, but still figuring out the DevTools patches.)
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
Attachment #8945129 - Flags: review?(longsonr) → review+
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
Attachment #8945132 - Flags: review?(longsonr) → review+
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 ?
Attachment #8945131 - Flags: review?(longsonr) → review+
Flags: needinfo?(jwatt)
Comment on attachment 8945130 [details]
Bug 1388931 part 2 - Gut the WebIDL interfaces related to SVG paths.

https://reviewboard.mozilla.org/r/215364/#review221166
Attachment #8945130 - Flags: review?(amarchesini) → review+
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
Attachment #8945129 - Flags: review?(amarchesini) → review+
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.
(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.
Flags: needinfo?(jwatt)
Depends on: 1436438

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.

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

Flags: webcompat?
Flags: needinfo?(jwatt)

(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?

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

Mike, did they get back to you?

Flags: needinfo?(jwatt) → needinfo?(miket)

(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)?

Flags: needinfo?(miket) → needinfo?(astevenson)

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

Flags: needinfo?(astevenson)

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

Webcompat Priority: --- → ?

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

Type: defect → task

(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.

Blocks: 1714238

(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?

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

(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.

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.

(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. :)

Assignee: jwatt → boris.chiou
Severity: normal → S3
Blocks: 1745060

This patch removes the following APIs:

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

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.

Blocks: 1745078
Attachment #9254424 - Attachment description: Bug 1388931 - Obsolete SVGPathSeg APIs. → Bug 1388931 - Remove SVGPathSeg APIs.
Attachment #9254424 - Attachment description: Bug 1388931 - Remove SVGPathSeg APIs. → Bug 1388931 - Unship SVGPathSeg APIs behind a preference.
Attachment #9254425 - Attachment is obsolete: true
Summary: Consider removing SVGPathSeg APIs → Unship SVGPathSeg APIs
Blocks: 1745149
No longer blocks: 1714238
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/78dad725f8a2
Unship SVGPathSeg APIs behind a preference. r=emilio
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/31989 for changes under testing/web-platform/tests
Whiteboard: [webcompat] → [webcompat], [wptsync upstream]
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 97 Branch
Upstream PR merged by moz-wptsync-bot

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,

(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

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

(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 :-)

You need to log in before you can comment on or make changes to this bug.