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.

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

Fix ellipse angles definition #24816

Merged
merged 3 commits into from
Feb 24, 2023
Merged

Fix ellipse angles definition #24816

merged 3 commits into from
Feb 24, 2023

Conversation

Kaiido
Copy link
Contributor

@Kaiido Kaiido commented Feb 24, 2023

Description

This fixes a the Canvas2D ellipse() methods angles definition.

As was found in whatwg/html#8408 the ellipse's startAngle and endAngle are not really angles on the ellipse, but angles on the eccentric circle of the ellipse.
These are known as "eccentric angle" (e.g on Wolfram) , though this term might be a bit obscure at first sight, so I wouldn't mind a better proposition if reviewers have one.

Motivation

A couple of questions were opened on StackOverflow over this discrepancy between specs+docs vs implementations. (https://stackoverflow.com/questions/74111375, https://stackoverflow.com/questions/52149077)

Additional details

The specs got fixed in whatwg/html#8495 but as can be seen, the fix was quite verbose and I'm not sure the docs need to get into that much details. So I hope this little fix is both enough to lead the ones that could get surprised by the actual behavior to the proper understanding, and not too obscure to lose in details the ones that wouldn't notice.

Related issues and pull requests

As was found in whatwg/html#8408 the ellipse's `startAngle` and `endAngle` are not really angles on the ellipse, but angles on the eccentric circle of the ellipse.   
These are known as "eccentric angle", though this term *might* be a bit obscure at first sight, so I wouldn't mind a better proposition if reviewers have one.  
The specs got fixed in whatwg/html#8495 but as can be seen, the fix was quite verbose and I'm not sure the docs need to get that much details.  
So I hope this little fix is both enough to lead the ones that could get surprised by the actual behavior to the proper understanding, and not too obscure to lose in details the ones that wouldn't notice.
@Kaiido Kaiido requested a review from a team as a code owner February 24, 2023 01:45
@Kaiido Kaiido requested review from Elchi3 and removed request for a team February 24, 2023 01:45
@github-actions github-actions bot added the Content:WebAPI Web API docs label Feb 24, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 24, 2023

Preview URLs

External URLs (1)

URL: /en-US/docs/Web/API/CanvasRenderingContext2D/ellipse
Title: CanvasRenderingContext2D.ellipse()

(comment last updated: 2023-02-24 07:56:30)

Copy link
Contributor

@teoli2003 teoli2003 left a comment

Choose a reason for hiding this comment

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

Let's link to a good explanation (on an outside website). That way, people who don't know what it is are one click away from a good explanation.

@teoli2003 teoli2003 removed the request for review from Elchi3 February 24, 2023 07:47
@teoli2003
Copy link
Contributor

This is a really good catch, and I learned some math stuff today (and I am giving maths lessons)

Kaiido and others added 2 commits February 24, 2023 16:52
Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
Copy link
Contributor

@teoli2003 teoli2003 left a comment

Choose a reason for hiding this comment

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

👍

@teoli2003 teoli2003 merged commit d9b512d into mdn:main Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants