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

bugfix: issue 11801 #11849

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
+7 −7
Draft

bugfix: issue 11801 #11849

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/plugins/plugin.tooltip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,13 +630,13 @@ export class Tooltip extends Element {
return tooltipItems;
}

update(changed, replay) {
update(changed, replay, inChartArea = true) {
const options = this.options.setContext(this.getContext());
const active = this._active;
let properties;
let tooltipItems = [];

if (!active.length) {
if (!active.length || !inChartArea) {
Copy link
Collaborator

@LeeLenaleee LeeLenaleee Aug 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It seems that this behavior was added intentionally and is not a bug when I look at the code and the pr where this got introduced.

I will look at this later to see if this might break any implementations that rely on that behavior

image

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback.
Correct me if I'm wrong, from my understanding, the !active.length is added to make sure the chart can be interactive even when the cursor is out of it (hovering over the legend and high light the corresponding data in the chart).
Adding the extra check will only effect the visibility of the tooltip in this scenario.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @LeeLenaleee, do you think this is a proper change or do you have any further comments on this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi, I tested the changes, what you said is correct, this does still work with externally calling code to setActiveElements. Although the name is kind of misleading in those cases. Since the bool is called inChartArea, but when you use setActiveElements you are not in the chart area.

Also this change seems to introduce a new bug. When you move your mouse to the legend quickly and return to the same slice the hover does not trigger anymore

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @LeeLenaleee , I tested it and tried to fix that, but looks like it's not a simple change. I will change this PR to draft and work on this continuously.

if (this.opacity !== 0) {
properties = {
opacity: 0
Expand Down Expand Up @@ -1147,7 +1147,7 @@ export class Tooltip extends Element {
const positionChanged = this._positionChanged(active, e);

// Remember Last Actives
const changed = replay || !_elementsEqual(active, lastActive) || positionChanged;
const changed = replay || !inChartArea || !_elementsEqual(active, lastActive) || positionChanged;

// Only handle target event on tooltip change
if (changed) {
Expand All @@ -1159,7 +1159,7 @@ export class Tooltip extends Element {
y: e.y
};

this.update(true, replay);
this.update(true, replay, inChartArea);
}
}

Expand Down
6 changes: 3 additions & 3 deletions test/specs/plugin.tooltip.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -965,7 +965,7 @@ describe('Plugin.Tooltip', function() {

// First dispatch change event, should update tooltip
await jasmine.triggerMouseEvent(chart, 'mousemove', firstPoint);
expect(tooltip.update).toHaveBeenCalledWith(true, undefined);
expect(tooltip.update).toHaveBeenCalledWith(true, undefined, true);

// Reset calls
tooltip.update.calls.reset();
Expand Down Expand Up @@ -1028,15 +1028,15 @@ describe('Plugin.Tooltip', function() {

// First dispatch change event, should update tooltip
await jasmine.triggerMouseEvent(chart, 'mousemove', firstPoint);
expect(tooltip.update).toHaveBeenCalledWith(true, undefined);
expect(tooltip.update).toHaveBeenCalledWith(true, undefined, true);

// Reset calls
tooltip.update.calls.reset();

// Second dispatch change event (same event), should update tooltip
// because position mode is 'nearest'
await jasmine.triggerMouseEvent(chart, 'mousemove', secondPoint);
expect(tooltip.update).toHaveBeenCalledWith(true, undefined);
expect(tooltip.update).toHaveBeenCalledWith(true, undefined, true);
});

describe('positioners', function() {
Expand Down