-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merge duplicate SVGGeometryElement+SVGPathElement entries #9479
Merge duplicate SVGGeometryElement+SVGPathElement entries #9479
Conversation
This was tested by checking for the 3 members on all 7 types of elements which currently inherit from SVGGeometryElement: http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=9076 ```js var tags = ['circle', 'ellipse', 'line', 'path', 'polygon', 'polyline', 'rect']; var members = ['getPointAtLength', 'getTotalLength', 'pathLength']; tags.forEach(function(localName) { var e = document.createElementNS('http://www.w3.org/2000/svg', localName); members.forEach(function(member) { w(localName + '.' + member + ': ' + typeof e[member]); }); }); ``` In Chromium, getTotalLength and getPointAtLength were moved in M56, and pathLength was moved in M57: https://bugs.chromium.org/p/chromium/issues/detail?id=596043 https://storage.googleapis.com/chromium-find-releases-static/887.html#887bbf6c0edcabfa012099379c2d68a3dc4afd3f https://storage.googleapis.com/chromium-find-releases-static/6fd.html#6fd1cbb3c5a636ee3830729e4b26ca1d5d78c8d6 In Gecko, the SVGGeometryElement was introduced in 53, but elements other than <path> were made to inherit in 61: https://bugzilla.mozilla.org/show_bug.cgi?id=1239100 https://bugzilla.mozilla.org/show_bug.cgi?id=1325320 In WebKit the change was made at trunk version 606.1.14: https://trac.webkit.org/changeset/230829/webkit https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/Configurations/Version.xcconfig?rev=230829 In Edge 13 and IE 11, only path.getPointAtLength and path.getTotalLength were supported, there was no support for pathLength. In Opera 12.16, all 3 members were supported on path and nowhere else. Fixes mdn#9183.
0247888
to
4897a05
Compare
api/SVGGeometryElement.json
Outdated
@@ -11,13 +11,13 @@ | |||
"version_added": "18" | |||
}, | |||
"edge": { | |||
"version_added": "≤79" | |||
"version_added": "12" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to satisfy the lint it's necessary to say that SVGGeometryElement
was introduced when some of its members were first introduced on SVGPathElement
. I think this is the right thing to do and support for the interface itself isn't very interesting, but should I use partial_implementation
for the range where the interface actually didn't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think this is the right thing to do. I think it's nice to have the symmetry between the partial_implementation
for the subfeatures and the parent. It's a little messy, but the reality is messy too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'm going to split and use notes like the one on BaseAudioContext
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
} | ||
}, | ||
"supportOtherThanPath": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This "supportOtherThanPath" entry is removed because it's all represented on the individual entries now.
"firefox_android": { | ||
"version_added": "61" | ||
}, | ||
"version_added": "79" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a mistake. pathLength
wasn't supported in IE or EdgeHTML, confirmed by testing.
], | ||
"opera_android": [ | ||
{ | ||
"version_added": "43" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a mistake. The off-by-13 rule held up to Opera 42 on both desktop and mobile, then things got out of sync. Opera desktop 43 was Blink 56, but Android 43 skipped to 59.
These were moved from SVGPathElement to SVGGeometryElement in all implementations, and continuing to document them in two places is not necessary. A similar merge is being done in BCD: mdn/browser-compat-data#9479
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, @foolip. I think the only change needed here is to extend the partial implementation detail to the parent feature. Thank you!
api/SVGGeometryElement.json
Outdated
@@ -11,13 +11,13 @@ | |||
"version_added": "18" | |||
}, | |||
"edge": { | |||
"version_added": "≤79" | |||
"version_added": "12" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think this is the right thing to do. I think it's nice to have the symmetry between the partial_implementation
for the subfeatures and the parent. It's a little messy, but the reality is messy too.
], | ||
"firefox": [ | ||
{ | ||
"version_added": "53" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is when SVGGeometryElement
itself was introduced in Firefox. It was only later that all interfaces were made to inherit from it. But doing that as 3 entries here does not seem like it would make the situation more clear to anyone. The subfeatures are what matter more here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. I like this pragmatic approach. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @foolip! 🎉
This was tested by checking for the 3 members on all 7 types of elements
which currently inherit from SVGGeometryElement:
http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=9076
In Chromium, getTotalLength and getPointAtLength were moved in M56,
and pathLength was moved in M57:
https://bugs.chromium.org/p/chromium/issues/detail?id=596043
https://storage.googleapis.com/chromium-find-releases-static/887.html#887bbf6c0edcabfa012099379c2d68a3dc4afd3f
https://storage.googleapis.com/chromium-find-releases-static/6fd.html#6fd1cbb3c5a636ee3830729e4b26ca1d5d78c8d6
In Gecko, the SVGGeometryElement was introduced in 53, but elements
other than were made to inherit in 61:
https://bugzilla.mozilla.org/show_bug.cgi?id=1239100
https://bugzilla.mozilla.org/show_bug.cgi?id=1325320
In WebKit the change was made at trunk version 606.1.14:
https://trac.webkit.org/changeset/230829/webkit
https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/Configurations/Version.xcconfig?rev=230829
In Edge 13 and IE 11, only path.getPointAtLength and path.getTotalLength
were supported, there was no support for pathLength.
In Opera 12.16, all 3 members were supported on path and nowhere else.
Fixes #9183.