Open Bug 1388931 Opened 4 years ago Updated 12 days ago

Consider removing SVGPathSeg APIs

Categories

(Core :: SVG, task, P2)

56 Branch
task

Tracking

()

Webcompat Priority ?
Tracking Status
firefox57 --- affected

People

(Reporter: rbyers, Assigned: jwatt)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-needed, site-compat, Whiteboard: [webcompat])

Attachments

(5 files)

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
Duplicate of this bug: 1648823

(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
You need to log in before you can comment on or make changes to this bug.