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

Limit Math.asin inputs to the range [-1, 1] #9410

Merged
merged 1 commit into from Jul 14, 2021

Conversation

etimberg
Copy link
Member

Resolves #9390

@etimberg etimberg added this to the Version 3.5.0 milestone Jul 13, 2021
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

It would be interesting to know, what goes negative in the failure case, and why.

@kurkle kurkle merged commit df49447 into chartjs:master Jul 14, 2021
@danielgindi
Copy link
Contributor

It would be interesting to know, what goes negative in the failure case, and why.

I'll try to create a repro for this. Although it was very random.

@etimberg etimberg deleted the range-error-fix branch July 20, 2021 12:45
@danielgindi
Copy link
Contributor

@kurkle I just managed to make a repro: https://codepen.io/danielgindi/pen/VwbzPoe
Now I took a snapshot of a config that did this, by logging many configs until I found the right one.
I also logged the supported time formats at the time, and written a snippet to format only those and throw if something unexpected happens.
I cleaned up the config a bit to the minimum required to produce the error.
Also note that if the containing div is removed (only 1 containing div remains) - the error does not occur!

@kurkle
Copy link
Member

kurkle commented Jul 20, 2021

Ok, chart.js thinks its attached while its not.
This fails, because it has a container and the container has a container, which is not attached to DOM:

isAttached(canvas) {
const container = _getParentNode(canvas);
return !!(container && _getParentNode(container));
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

new Chart throws RangeError: minimumFractionDigits value is out of range.
3 participants