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

Chart inside bootstrap 'collapse' element #4968

Closed
wants to merge 3 commits into from

Conversation

zoharshavit
Copy link

@zoharshavit zoharshavit commented Nov 19, 2017

When chart is inside a bootstrap 'collapse' element the width and height are zero and the chart 'draw' function throws an IndexSizeError because the outer/inner radius are computed using a zero width/height and when subtracting padding it results in a negative value.
When element is collapsed it does not show anyway so adding Math.abs() to the radius will have no effect on the drawing itself but will eliminate the error.

here is a jsFiddle to demonstrate the issue https://jsfiddle.net/4s09skz9

When chart is inside a bootstrap 'collapse' element the width and height are zero and the chart 'draw' function throws an IndexSizeError because the outer/inner radius are computed using a zero width/height and when subtracting padding it results in a negative value.
When element is collapsed it does not show anyway so adding Math.abs() to the radius will have no effect on the drawing itself but will eliminate the error.
@etimberg
Copy link
Member

etimberg commented Nov 19, 2017

@zoharshavit is there an open issue or reproduce for this issue? Also, can you combine #4969 and #4970 into a single PR?

@zoharshavit
Copy link
Author

@etimberg i was fixing an issue that i encountered, not an open issue, How do i combine 3 commits into one pull request? I'm really new to this, sorry...

@simonbrunel
Copy link
Member

I agree with @etimberg, you can push all your changes to your patch-1 branch. That will automatically update this PR, (you can close #4969 and #4970). I would also like to see a fiddle that reproduce your issue in order to decide what the fix should be.

Math.abs(radius) is not a good choice since it generates positive radius if radius < 0, you should use Math.max(0, radius) instead. However, I'm not sure those changes actually fix the real issue since simply silent exceptions.

A better fix would be:

  • do not render the chart if chart.width <= 0 || !chart.height <= 0
  • do not render controllers if chartArea.width <= 0 || chartArea.height <= 0
  • [bonus] do not render layout box if box.width <= 0 || box.height <= 0

We should also sanitize those sizes:

  • make sure that chart.width and chart.height are always >= 0
  • make sure that the chart.chartArea size is always >= 0

@zoharshavit
Copy link
Author

Added the other commits to this pull request and canceled them.
Added a jsFiddle to reproduce the issue

@zoharshavit
Copy link
Author

As for @simonbrunel's other comments:
I really don't mind the Math.max(0, radius) solution as it also fixes the problem, if this is more 'correct' than the Math.abs(radius) solution then I'm on board.
As for the rendering solution, it's way over my current understanding of the project ,so if someone were to code it and it will stop the error, I'll support it.

@jcopperfield
Copy link
Contributor

jcopperfield commented Nov 30, 2017

I agree with @simonbrunel that the layout code should sanitize size.
However a simple fix without having to rewrite the layoutService code would be the change the resize event code. With the latest pull a simple check and early return in the resize event on line 191 of the core.controller.js would get rid of the error and prevents unneeded update.

if (newWidth === 0 || newHeight === 0) {
    return;
}

Otherwise I think it would be better to sanitize the me.drawingArea variable in the file scale.radialLinear.js, to get rid of the error.

@benmccann
Copy link
Contributor

@zoharshavit will you be able to respond to the latest comments on this PR?

@zoharshavit
Copy link
Author

@benmccann I have responded to the best of my knowlage, and offered a fix as I understand it.
The other commenters such as @jcopperfield and @simonbrunel have made some suggestions that made a lot of sense to me but I do not feel that I understand the changes they suggested to the extent that I would make them myself.
I will be happy to hear that this issue has been resolved even if the solution will not include my code suggestion.
Thank you

@zoharshavit zoharshavit reopened this Jan 16, 2018
@benmccann
Copy link
Contributor

Thanks for replying. I think we're unlikely to proceed with this PR given it's current state. We'd be happy to accept a fix with the given suggestions, so feel free to reopen if you update with the changes.

I'm not sure that anyone will fix the issue otherwise since there are currently hundreds of open issues in the issue tracker to address

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

Successfully merging this pull request may close these issues.

5 participants