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

ensure getComputedStyle always has a valid canvas provided #11809

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

ensure getComputedStyle always has a valid canvas provided #11809

wants to merge 4 commits into from

Conversation

DAcodedBEAT
Copy link
Contributor

Expected behavior

getComputedStyle() from helpers.dom.js should be called with valid canvas values provided.

Current behavior

TypeError: Cannot read properties of null (reading 'ownerDocument')
  at getComputedStyle(../../node_modules/chart.js/dist/chunks/helpers.segment.js:1841:47)
  at getMaximumSize(../../node_modules/chart.js/dist/chunks/helpers.segment.js:1933:17)
  at DomPlatform.getMaximumSize(../../node_modules/chart.js/dist/chart.js:3435:12)
  at _a._resize(../../node_modules/chart.js/dist/chart.js:5700:35)
  at detached(../../node_modules/chart.js/dist/chart.js:6267:12)
  at _a.bindResponsiveEvents(../../node_modules/chart.js/dist/chart.js:6273:16)
  at _a.bindEvents(../../node_modules/chart.js/dist/chart.js:6215:12)
  at _a._checkEventBindings(../../node_modules/chart.js/dist/chart.js:5911:12)
  at _a.update(../../node_modules/chart.js/dist/chart.js:5860:10)
  at this._doResize(../../node_modules/chart.js/dist/chart.js:5628:46)
  at sentryWrapped(../../node_modules/@sentry/browser/esm/helpers.js:36:17)

From sentry:

image

Reproducible sample

.

Optional extra steps/info to reproduce

No response

chart.js version

4.4.3

Browser name and version

Experienced on Chrome >=124.0.0 according to my project's Sentry, but probably impacts others since this is an issue with the provided parameter.

Link to your project

No response

@DAcodedBEAT
Copy link
Contributor Author

@etimberg / @LeeLenaleee I used optional chaining and null coalescing because of this discussion - #11651 (comment), which is why the CI pipeline failed. Give the word and I'll remove the rule so this pipeline can pass.

LeeLenaleee
LeeLenaleee reviewed Jun 12, 2024
View reviewed changes
src/helpers/helpers.dom.ts
Comment on lines -108 to -109
* @param event
* @param chart
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess that this was not meant to be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was removed since it didn't provide any additional information than the function signature already provided (not even variable types). If you still want this, I can revert this change.

src/platform/platform.base.js Outdated Show resolved Hide resolved
src/helpers/helpers.dom.ts Outdated Show resolved Hide resolved
@DAcodedBEAT
Copy link
Contributor Author

@etimberg / @LeeLenaleee can you re-review this?

@LeeLenaleee
Copy link
Collaborator

It feels kind of hacky to just return semi default values if the canvas does not exist.

Can you reproduce the errors because it seems to me that if you remove the parents and keep chart.js alive. But if you destroy the chart first these errors also should not happen

@DAcodedBEAT
Copy link
Contributor Author

The canvas probably got unmounted as a result of React's rerendering (I was using Chartjs via https://github.com/reactchartjs/react-chartjs-2). Since we've hit enough issues with Chartjs in our react application, my team has decided to move away from chartjs.

Feel free to close or update this pull request as you see fit.

@DAcodedBEAT DAcodedBEAT mentioned this pull request Jul 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LeeLenaleee LeeLenaleee LeeLenaleee left review comments

@timwhite06 timwhite06 timwhite06 approved these changes

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@DAcodedBEAT @LeeLenaleee @timwhite06