Skip to content

Prevent negative width or height on rect #2482

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

Closed
wants to merge 3 commits into from
Closed

Prevent negative width or height on rect #2482

wants to merge 3 commits into from

Conversation

vdh
Copy link
Contributor

@vdh vdh commented Mar 20, 2018

When working with responsive container sizes for charts, I've encountered Error: <rect> attribute height: A negative value is not valid. DOM errors (Chrome 65.0.3325.162). Because the container area is sometimes too small for a reasonable chart, the _length calculations sometimes go into the negatives.

This PR is just a simple guard to avoid setting invalid negative values on the rect elements.

Prevent "Error: <rect> attribute height: A negative value is not valid." DOM errors when very small chart sizes causes `_length` calculations to go into the negatives.
@etpinard
Copy link
Contributor

Thanks very much for bringing this up! I'm not convinced your patch is the best way to fix this bug though. _length shouldn't be negative in the first place when reaching this part of the code.

Would you mind sharing a reproducible example that exhibits the DOM errors you're referring to?

@vdh
Copy link
Contributor Author

vdh commented Mar 21, 2018

@etpinard
I was able to recreate it in CodePen ( https://codepen.io/vdh/pen/WzpOoe ), although it may be connected to react-plotly.js's useResizeHandler option (or the Plotly.Plots.resize call it wraps…?).

@vdh
Copy link
Contributor Author

vdh commented Mar 21, 2018

I've managed to make a smaller demo https://codepen.io/vdh/pen/OvpjmX
For some reason useResizeHandler / Plotly.Plots.resize is causing it, but I haven't been able to dig deeper yet as it would require reverse-engineering the code of react-plotly.js to recreate the Plotly calls.

@n-riesco
Copy link
Contributor

@vdh I see the same issue with react-chart-editor (see my comment here). In my case, an easy way to trigger this issue is to set display: none in the container node and trigger a resize.

@alexcjohnson
Copy link
Collaborator

I'm assuming what's happening here is the margins are larger than the plot area - ie margin.l + margin.r > width, either because width is very small or we can't determine it at all (invisible node cc @n-riesco ). These situations are going to arise from time to time, and we shouldn't barf of course... if width or height is <=0 we should bail out of plotting entirely (and early), and if the margins are too big to display anything... dunno what makes most sense here, either shrink the margins so we still have plotting space, or just display the title and bail?

@vdh
Copy link
Contributor Author

vdh commented Apr 16, 2018

I also discovered two additional places where rects are given negative widths for tiny charts (and added guards to the PR):

  • drawing.setRect / drawing.setSize in src/components/drawing/index.js
  • axes.makeClipPaths in src/plots/cartesian/axes.js

@etpinard @alexcjohnson
I know these guards don't directly address the code that generates the negative dimensions, but is it really so bad to guard against the generation of invalid SVG attributes? These DOM errors create a lot of noise in the console.

@etpinard
Copy link
Contributor

I also discovered two additional places where rects are given negative widths for tiny charts

Right. This is exactly why we're hoping to find the root cause of this problem.

Instead of patching every piece of code that might write negative svg width/height, we should (at least try to) make sure _length and other positive definite fields don't become negative in the first place.

@etpinard
Copy link
Contributor

etpinard commented Apr 16, 2018

... to do so, we'll need some sort of reproducible example in plotly.js.

@grahambryan
Copy link

grahambryan commented May 1, 2018

Has there been any resolution to this problem? I have been running into the same problem within react-plotly.js within my react app.

Error: attribute width: A negative value is not valid. ("-50") @ plotly.js:17171

@vdh
Copy link
Contributor Author

vdh commented May 2, 2018

@etpinard

... to do so, we'll need some sort of reproducible example in plotly.js.

So are you implying that the CodePen examples I provided earlier are inadequate? Is there a better way to create an example? I apologise that I don't have the skills or familiarity with Plotly to separate react-plotly.js from the examples, but on Chrome 66.0.3359.139 I can reproduce the same console errors using those CodePens.

@etpinard
Copy link
Contributor

etpinard commented May 2, 2018

@vdh as far as I know no one has been able to reproduce this issue using only plotly.js (i.e. no react, no react-plotly.js). This doesn't mean plotly.js isn't doing something wrong here. But it would help you pin down why theses values are negative in the first place.

@etpinard
Copy link
Contributor

etpinard commented May 24, 2018

Ok. I was able to reproduce this bug in pure plotly.js.

See https://codepen.io/etpinard/pen/KRjKpg?editors=0010 and open the console to see the logs.

It seems to be a problem with Plots.resize (which is the plotly.js method called when react-plotly.js' useResizeHandler option is turned on).

Closing this PR and moving the discussion to #2663

@etpinard etpinard closed this May 24, 2018
@vdh vdh deleted the patch-1 branch June 28, 2018 00:24
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