Unship SVGPathSeg APIs
Categories
(Core :: SVG, task, P2)
Tracking
()
People
(Reporter: rbyers, Assigned: boris)
References
(Blocks 3 open bugs)
Details
(Keywords: dev-doc-complete, site-compat, Whiteboard: [webcompat], [wptsync upstream])
Attachments
(6 files, 1 obsolete file)
59 bytes,
text/x-review-board-request
|
baku
:
review+
longsonr
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
baku
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
longsonr
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
longsonr
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
Details | |
48 bytes,
text/x-phabricator-request
|
Details | Review |
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
Reporter | ||
Updated•7 years ago
|
Updated•7 years ago
|
Reporter | ||
Comment 1•7 years ago
|
||
Note this is already tested by https://w3c-test.org/svg/historical.html (chrome passes 41/41, firefox 29/41 - see http://wpt.fyi)
Updated•7 years ago
|
Comment 2•7 years ago
|
||
Yes, we should do this.
Updated•7 years ago
|
Updated•7 years ago
|
Comment 3•7 years ago
|
||
Tom, would you be interested in working on this?
Comment 4•7 years ago
|
||
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•7 years ago
|
||
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.
Updated•7 years ago
|
Updated•7 years ago
|
Comment 6•7 years ago
|
||
(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.
Reporter | ||
Comment 7•7 years ago
|
||
(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.
(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•7 years ago
|
||
(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•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
(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•7 years ago
|
||
FWIW the Chromium bug to add the new API is: https://bugs.chromium.org/p/chromium/issues/detail?id=545467
Comment 15•7 years ago
|
||
(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•7 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 28•7 years ago
|
||
mozreview-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/#review221064
Comment 29•7 years ago
|
||
mozreview-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
Comment 30•7 years ago
|
||
mozreview-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 ?
Comment 31•7 years ago
|
||
have we announced an intent to unship on https://groups.google.com/forum/#!forum/mozilla.dev.platform ?
Updated•7 years ago
|
Comment 32•7 years ago
|
||
mozreview-review |
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•7 years ago
|
||
mozreview-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
Comment 34•7 years ago
|
||
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•7 years ago
|
||
(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•7 years ago
|
||
Polyfill for SVGPathSeg: https://github.com/progers/pathseg Polyfill for getPathData: https://github.com/jarek-foksa/path-data-polyfill.js
Comment 37•6 years ago
•
|
||
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•6 years ago
|
||
Do we have finishing out this bug on our radar, jwatt?
Comment 39•6 years ago
|
||
(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•6 years ago
|
||
I've emailed them to ask if they can update to the latest version of their polyfill.
Comment 42•6 years ago
|
||
(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•6 years ago
|
||
Let's try their founder and the head of communications on LinkedIn.
Comment 44•5 years ago
|
||
Migrating Webcompat whiteboard priorities to project flags. See bug 1547409.
Updated•5 years ago
|
Comment 47•4 years ago
|
||
(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.
Assignee | ||
Comment 48•3 years ago
|
||
(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•3 years ago
|
||
We should check first that Chrome still does work that way. It's been 4 years, they may have changed.
Assignee | ||
Comment 50•3 years ago
|
||
(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•3 years ago
|
||
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.
Assignee | ||
Comment 52•3 years ago
|
||
(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 | ||
Comment 53•3 years ago
|
||
This patch removes the following APIs:
- SVGAnimatedPathData
- SVGPathSeg
- SVGPathSegList
- SVGPathElement::getPathSegAtLength()
Assignee | ||
Comment 54•3 years ago
|
||
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.
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Comment 55•3 years ago
|
||
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
Comment 57•3 years ago
|
||
bugherder |
Upstream PR merged by moz-wptsync-bot
Updated•3 years ago
|
Comment 59•3 years ago
|
||
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()
andlength
.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 hidden (off-topic) |
Reporter | ||
Comment 62•3 years ago
|
||
(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•3 years ago
|
||
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
Reporter | ||
Comment 64•3 years ago
|
||
(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/LDYo5CjfBAAJThe 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 :-)
Description
•