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

Fixes Tooltips Showing Outside Minimum Time Bounds #5287

Closed
wants to merge 20 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@MPierre9
Contributor

MPierre9 commented Feb 21, 2018

Fixes #5228 by checking if x is outside the minimum border of the graph then allowing no tooltips to be created if so.

JSFiddle

Result

tooltip

@MPierre9

This comment has been minimized.

Contributor

MPierre9 commented Feb 21, 2018

Travis CI test fails even if I comment everything else out and leave variable border.

for (i = 0, len = active.length; i < len; ++i) {
tooltipItems.push(createTooltipItem(active[i]));
var border = active[0]._xScale.left + 7; // 7 used for graph offset
if (existingModel.x > border) {

This comment has been minimized.

@etimberg

etimberg Feb 22, 2018

Member

I think we should check that existingModel.x > chartArea.left && existingModel.y < chartArea.right

This comment has been minimized.

@MPierre9

MPierre9 Feb 22, 2018

Contributor

I think that's a great idea I just keep getting chartArea is not defined when I use it. I think chartArea
only exists in function determineAlignment(tooltip, size).

chartarea

@etimberg

This comment has been minimized.

Member

etimberg commented Feb 22, 2018

Does the test fail if border is set to 0 always? It looks like the tooltip is at a different position after these tests

@MPierre9

This comment has been minimized.

Contributor

MPierre9 commented Feb 22, 2018

It seems to pass only if I comment out the if(existingModel.x > border) and leave border set to 0. If I keep the if I get 20 failed tests with gulp test. I'm not sure what the code is interfering with even with border set to zero it shouldn't obstruct the creation of a tooltip

@MPierre9

This comment has been minimized.

Contributor

MPierre9 commented Feb 22, 2018

After some more testing it seems to be active[0]._xScale.left not being available when testing for certain charts like donut and bubble charts. Is there a way to check what chart type is being used in core.tooltip.js?

@MPierre9

This comment has been minimized.

Contributor

MPierre9 commented Feb 22, 2018

Updated JSFiddle

I changed the location of the if statement and made a global variable so getting the chartArea would be easier.

@@ -845,7 +847,7 @@ module.exports = function(Chart) {
// Find Active Elements for tooltips
if (e.type === 'mouseout') {
me._active = [];
} else {
} else if (e.x > globalChartArea) {

This comment has been minimized.

@etimberg

etimberg Feb 22, 2018

Member

The global variable will cause problems when there are multiple charts on the same page that could potentially have different sizes. Is there a reason that me._chart.chartArea doesn't work?

This comment has been minimized.

@MPierre9

MPierre9 Feb 22, 2018

Contributor

Actually me._chart.chartArea works, I wasn't sure how to call it. Thanks!

@@ -845,7 +845,7 @@ module.exports = function(Chart) {
// Find Active Elements for tooltips
if (e.type === 'mouseout') {
me._active = [];
} else {
} else if (e.x > me._chart.chartArea.left) {

This comment has been minimized.

@etimberg

etimberg Feb 23, 2018

Member

looks good! One last question, should this also check e.x < me._chart.chartArea.right? if so, i'd move me._chart.chartArea out into a local variable for an improvement to minification

This comment has been minimized.

@MPierre9

MPierre9 Feb 23, 2018

Contributor

Good idea! I added it to the latest PR

This comment has been minimized.

@MPierre9

MPierre9 Feb 23, 2018

Contributor

Right now with the right side of the chart the bug doesn't occur but it could be a good fail safe just in case

@etimberg etimberg requested a review from simonbrunel Feb 23, 2018

@simonbrunel

We need unit tests for this change that check mouse outside the chart area and mouse inside the chart area and ideally, 1px around the area boundaries (inside and outside). In your fiddle (updated), I'm noticing strange behavior where the tooltip doesn't show when entering from the left.

chartjs 5287

@@ -839,13 +839,14 @@ module.exports = function(Chart) {
var me = this;
var options = me._options;
var changed = false;
var border = me._chart.chartArea;

This comment has been minimized.

@simonbrunel

simonbrunel Mar 4, 2018

Member

I would rename this variable area

This comment has been minimized.

@MPierre9

MPierre9 Mar 11, 2018

Contributor

Changed in the latest PR

me._lastActive = me._lastActive || [];
// Find Active Elements for tooltips
if (e.type === 'mouseout') {
me._active = [];
} else {
} else if (e.x > border.left && e.x < border.right) {

This comment has been minimized.

@simonbrunel

simonbrunel Mar 4, 2018

Member

I think limits should be included because values can be positioned at the start and end pixels.

Shouldn't we do the same for e.y but also clear active elements if mouse outside?

if (e.type === 'mouseout' ||
    e.x < area.left || e.x > area.right ||
    e.y < area.top || e.y > area.bottom) {
    me._active = []
} else {
    me._active = me._chart.getElementsAtEventForMode(...
}

This comment has been minimized.

@MPierre9

MPierre9 Mar 4, 2018

Contributor

Good idea I forgot about accounting for y. I'll make the changes, thanks!

@MPierre9

This comment has been minimized.

Contributor

MPierre9 commented Mar 4, 2018

Latest fiddle

Result

chartjstooltip

@simonbrunel Seems like the latest commit with your suggestion fixed the issue with the tooltips not showing. How should I go about making unit tests for this fix? I haven't made one for this project yet. Thanks for the help!

@MPierre9

This comment has been minimized.

Contributor

MPierre9 commented Mar 4, 2018

When I do gulp test I'm getting Core.Tooltip index mode Should only use x distance when intersect is false FAILED Thoughts?

Edit: Only seems to occur when the I include e.y < area.top || e.y > area.bottom

@simonbrunel

This comment has been minimized.

Member

simonbrunel commented Mar 4, 2018

@MPierre9 tests are failing because the triggered mouse event is outside the chart area. I would really prefer to update the unit tests with correct mouse position and keep e.y < area.top || e.y > area.bottom.

@MPierre9

This comment has been minimized.

Contributor

MPierre9 commented Mar 4, 2018

@simonbrunel Ok I'll look into rewriting the unit test

@MPierre9

This comment has been minimized.

Contributor

MPierre9 commented Mar 5, 2018

@simonbrunel I updated the unit test so it can pass with e.y < area.top || e.y > area.bottom. I'm not sure if it is correct. A quick review would be much appreciated. Thanks!

@benmccann

This comment has been minimized.

Collaborator

benmccann commented Apr 15, 2018

@MPierre9 would you be able to rebase this PR? thanks!

@MPierre9

This comment has been minimized.

Contributor

MPierre9 commented Apr 16, 2018

Sure thing! @benmccann

@MPierre9

This comment has been minimized.

Contributor

MPierre9 commented Apr 17, 2018

Unfortunately ran into too many problems with git so I made another PR with the updates. #5419

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment