Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fold SVGAnimatedPoints (a mixin) into SVGPolygonElement and SVGPolylineElement #5181

Merged
merged 2 commits into from
Jun 3, 2021
Merged

Fold SVGAnimatedPoints (a mixin) into SVGPolygonElement and SVGPolylineElement #5181

merged 2 commits into from
Jun 3, 2021

Conversation

foolip
Copy link
Contributor

@foolip foolip commented May 21, 2021 •
edited
Loading

Redirect it to SVGPolygonElement, an arbitrary choice between the two.

Document animatedPoints in a similar way to animVal here:
https://developer.mozilla.org/en-US/docs/Web/API/SVGAnimatedString

This ignores that the spec requires animatedPoints to be an alias of
points, because that does not match what's actually implemented (yet):
https://svgwg.org/svg2-draft/shapes.html#__svg__SVGAnimatedPoints__animatedPoints
https://wpt.fyi/results/svg/shapes/animatedPoints-non-animated.html?run_id=5709221590990848&run_id=5697986191425536&run_id=5747784122630144&run_id=5742635329257472

@foolip
Copy link
Contributor Author

foolip commented May 21, 2021

@Elchi3 I'm leaving this as a draft pull request since I unfortunately (per habit) looked too closely and discovered something terrible. points and animatedPoints are claimed to be aliases in the spec, but I checked Chromium's implementation and they're not aliases. What do do? 😢

@github-actions
Copy link
Contributor

github-actions bot commented May 21, 2021 •
edited
Loading

Preview URLs

Flaws

URL: /en-US/docs/Web/API/SVGPolylineElement
Title: SVGPolylineElement
on GitHub
Flaw count: 5

  • macros:
    • /en-US/docs/Web/API/SVGPolylineElement/animatedPoints does not exist
    • /en-US/docs/Web/API/SVGPointList does not exist
    • /en-US/docs/Web/API/SVGPolylineElement/points does not exist
    • /en-US/docs/Web/API/SVGPointList does not exist
    • /en-US/docs/Web/API/SVGPointList does not exist

URL: /en-US/docs/Web/API/SVGPolygonElement
Title: SVGPolygonElement
on GitHub
Flaw count: 5

  • macros:
    • /en-US/docs/Web/API/SVGPolygonElement/animatedPoints does not exist
    • /en-US/docs/Web/API/SVGPointList does not exist
    • /en-US/docs/Web/API/SVGPolygonElement/points does not exist
    • /en-US/docs/Web/API/SVGPointList does not exist
    • /en-US/docs/Web/API/SVGPointList does not exist

External URLs

URL: /en-US/docs/Web/API/SVGPolylineElement
Title: SVGPolylineElement
on GitHub

No new external URLs

URL: /en-US/docs/Web/API/SVGPolygonElement
Title: SVGPolygonElement
on GitHub

No new external URLs

(this comment was updated 2021-06-03 07:49:19.021350)

@Elchi3
Copy link
Member

Elchi3 commented May 21, 2021

@Elchi3 I'm leaving this as a draft pull request since I unfortunately (per habit) looked too closely and discovered something terrible. points and animatedPoints are claimed to be aliases in the spec, but I checked Chromium's implementation and they're not aliases. What do do? 😢

Looks like a spec change per https://www.w3.org/TR/SVG2/shapes.html#InterfaceSVGAnimatedPoints

We probably want to show this spec change in the docs and in BCD then. Currently BCD just says "only implements the SVG 1.1 specification of the interface." which is too abstract. We should talk about this specific change instead I assume.

@foolip
Copy link
Contributor Author

foolip commented May 21, 2021

Aha, so this might be what the notes were about! I frowned and sent a PR to remove them in mdn/browser-compat-data#10560, but maybe it should go together with demixing.

@foolip
Copy link
Contributor Author

foolip commented May 21, 2021

Judging by https://wpt.fyi/results/svg/shapes/animatedPoints-non-animated.html no browser has followed the spec change here, it's just something the spec editors hope for at this point. Should we just document what's actually implemented?

@Elchi3
Copy link
Member

Elchi3 commented May 21, 2021

I see. Well, then I guess we should try to document reality and that's SVG 1.1 and implementations and not SVG 2.

@Elchi3 Elchi3 mentioned this pull request May 26, 2021
Closed
@foolip foolip mentioned this pull request Jun 2, 2021
Merged
@foolip foolip force-pushed the rm-SVGAnimatedPoints branch from 86c01c0 to 0783915 Compare June 2, 2021 07:10
@foolip foolip marked this pull request as ready for review June 2, 2021 07:11
@foolip foolip requested review from a team as code owners June 2, 2021 07:11
@foolip foolip requested review from Rumyra and removed request for a team June 2, 2021 07:11
@foolip
Copy link
Contributor Author

foolip commented Jun 2, 2021

@Elchi3 I've tried to document reality, PTAL.

Elchi3
Elchi3 requested changes Jun 2, 2021
View reviewed changes
Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this! There is a macro error but LGTM otherwise

files/en-us/web/api/svgpolygonelement/index.html Outdated

<dl>
<dt>{{domxref("SVGPolygonElement.animatedPoints")}} {{readOnlyInline}}</dt>
<dd>A {{DOMxRef("SVGPointList")}} representing the animated value of the element's {{SVGAttr("points")}} attribute. If the { {SVGAttr("points")}} attribute is not being animated, it contains the same value as the <code>points</code> property.</dd>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the build fails because you have a line break in a macro call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me a while to spot, but yeah I had { {.

@foolip
Fold SVGAnimatedPoints (a mixin) into SVGPolygonElement and SVGPolyli…
1eb7125 
…neElement

Redirect it to SVGPolygonElement, an arbitrary choice between the two.

Document animatedPoints in a similar way to animVal here:
https://developer.mozilla.org/en-US/docs/Web/API/SVGAnimatedString

This ignores that the spec requires animatedPoints to be an alias of
points, because that does not match what's actually implemented (yet):
https://svgwg.org/svg2-draft/shapes.html#__svg__SVGAnimatedPoints__animatedPoints
https://wpt.fyi/results/svg/shapes/animatedPoints-non-animated.html?run_id=5709221590990848&run_id=5697986191425536&run_id=5747784122630144&run_id=5742635329257472
@foolip foolip force-pushed the rm-SVGAnimatedPoints branch from 0783915 to 1eb7125 Compare June 2, 2021 11:33
Elchi3
Elchi3 requested changes Jun 3, 2021
View reviewed changes
Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking forward to the day we are in markdown and with as little kumascript macros as possible. :-)

files/en-us/web/api/svgpolylineelement/index.html Outdated Show resolved Hide resolved
files/en-us/web/api/svgpolygonelement/index.html Outdated Show resolved Hide resolved
@foolip @Elchi3
Apply suggestions from code review
d9e3425 
Co-authored-by: Florian Scholz <fs@florianscholz.com>
@foolip
Copy link
Contributor Author

foolip commented Jun 3, 2021

Looking forward to the day we are in markdown and with as little kumascript macros as possible. :-)

What will xrefs look like with Markdown? Won't it still be a macro that one can mess up?

@Elchi3
Copy link
Member

Elchi3 commented Jun 3, 2021

What will xrefs look like with Markdown? Won't it still be a macro that one can mess up?

Initially, yes, but I hope we won't have xref macros forever.

@Elchi3 Elchi3 merged commit 8d8e80b into mdn:main Jun 3, 2021
@foolip foolip deleted the rm-SVGAnimatedPoints branch June 3, 2021 08:21
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Elchi3 Elchi3 Elchi3 approved these changes

@Rumyra Rumyra Awaiting requested review from Rumyra

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@foolip @Elchi3