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

Hover with adding and removing datasets may fail #9653

Closed
joshkel opened this issue Sep 15, 2021 · 1 comment · Fixed by #9656
Closed

Hover with adding and removing datasets may fail #9653

joshkel opened this issue Sep 15, 2021 · 1 comment · Fixed by #9656

Comments

@joshkel
Copy link
Contributor

joshkel commented Sep 15, 2021

Expected Behavior

Assigning to chart.data.datasets and then using chart.update to refresh the chart should work.

Current Behavior

Under (roughly) the following sequence of events, an error is triggered:

  • The chart is drawn with data.
  • The user hovers over a data point.
  • The chart is redrawn once (and only once) with no data.
  • The chart is redrawn with data.

The specific error is "Cannot read properties of null (reading 'buildOrUpdateElements')" on this line.

From exploring in the debugger, the sequence of events appears to be

  • chart.update() goes to replay the last event.
  • For a mouse move event, that eventually calls updateHoverStyle, which tries to get the metaset for the dataset index for the hovered point.
  • Since that dataset is no longer valid, getDatasetMeta creates an empty metaset that's never properly updated.

Possible Solution

Should updateHoverStyle check whether the dataset index is valid before trying to retrieve the metaset?

Alternatively, or as well, I believe that _destroyDatasetMeta has a bug; the current code should probably be

    if (meta) {
      if (meta.controller) {
        meta.controller._destroy();
      }
      delete me._metasets[datasetIndex];
    }

(I made this change in my local copy, and it appears to fix the bug.)

Or, because deleting a non-existent element should be harmless:

    if (meta.controller) {
      meta.controller._destroy();
    }
    delete me._metasets[datasetIndex];

I wasn't sure if changing updateHoverStyle fits with Chart.js's design; I can submit a PR for whatever fix seems best.

Steps to Reproduce

https://codesandbox.io/s/optimistic-perlman-705ce?file=/src/index.js

Mouse over the chart while data is visible, then wait for it to disappear and reappear.

Environment

  • Chart.js version: 3.5.1
  • Browser name and version: Observed in Safari and in Chrome 93
joshkel added a commit to joshkel/Chart.js that referenced this issue Sep 16, 2021
I believe it's a mistake to only delete the metaset if it has a valid controller; see f191f2f for where this behavior was introduced.

This is a minimal fix for chartjs#9653; as discussed there, it may also be worth updating `updateHoverStyle`.

As of chartjs#7030, `this._metasets` should always be defined, so checking whether it's undefined is no longer necessary.
@kurkle
Copy link
Member

kurkle commented Sep 17, 2021

Does this issue depend on animations being enabled or not?

The updateHoverStyle can be triggered by external code, so I think it would be good to validate the index.

@kurkle kurkle added this to the Version 3.6.0 milestone Sep 17, 2021
@etimberg etimberg linked a pull request Sep 17, 2021 that will close this issue
joshkel added a commit to joshkel/Chart.js that referenced this issue Oct 14, 2021
kurkle pushed a commit that referenced this issue Oct 23, 2021
* Fix cleaning up metasets

I believe it's a mistake to only delete the metaset if it has a valid controller; see f191f2f for where this behavior was introduced.

This is a minimal fix for #9653; as discussed there, it may also be worth updating `updateHoverStyle`.

As of #7030, `this._metasets` should always be defined, so checking whether it's undefined is no longer necessary.

* Add a test covering metaset behavior

* Add a regression test for #9653; fix `toHaveSize` usage

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

Successfully merging a pull request may close this issue.

2 participants