Skip to content

Fix period positioned hover to work in different time zones as well as on grouped bars #5864

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

Merged
merged 14 commits into from
Jul 29, 2021
Merged

Fix period positioned hover to work in different time zones as well as on grouped bars #5864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
01b4836
clear special handling for bars and scatter
archmoj Jul 28, 2021
585ddbd
new fix to work in different time zones
archmoj Jul 28, 2021
528801a
adjust test
archmoj Jul 28, 2021
b400917
run hover label tests in different time zones
archmoj Jul 28, 2021
138707c
log for PR 5864
archmoj Jul 28, 2021
21fa988
do not redefine minPos and maxPos functions for bars with period
archmoj Jul 29, 2021
3bdbedb
declare and reuse a variable for isClosest or period in bar hover
archmoj Jul 29, 2021
3ba7ec7
fixing hover for period group bars issue 5841
archmoj Jul 29, 2021
3e52b94
look into cd0 for the case of box and violin
archmoj Jul 29, 2021
bfed46b
more strict tests to lock issue 5841
archmoj Jul 29, 2021
2c07925
update PR log
archmoj Jul 29, 2021
1b7fba1
bring parts of the previous logic - use (x|y)End instead of pEnd etc.
archmoj Jul 29, 2021
4d2961d
handle undefined start and end periods
archmoj Jul 29, 2021
1890e77
simplification for bar
archmoj Jul 29, 2021
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
32 changes: 32 additions & 0 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,35 @@ jobs:
paths:
- plotly.js

timezone-jasmine:
docker:
# need '-browsers' version to test in real (xvfb-wrapped) browsers
- image: circleci/node:12.22.1-browsers
working_directory: ~/plotly.js
steps:
- attach_workspace:
at: ~/
- run:
name: Run hover_label test in UTC timezone
environment:
TZ: "UTC"
command: date && npm run test-jasmine hover_label
- run:
name: Run hover_label test in Europe/Berlin timezone
environment:
TZ: "Europe/Berlin"
command: date && npm run test-jasmine hover_label
- run:
name: Run hover_label test in Asia/Tokyo timezone
environment:
TZ: "Asia/Tokyo"
command: date && npm run test-jasmine hover_label
- run:
name: Run hover_label test in America/Toronto timezone
environment:
TZ: "America/Toronto"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need all these time zones to be tested?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's pretty quick, might as well keep them all for now.

command: date && npm run test-jasmine hover_label

no-gl-jasmine:
docker:
# need '-browsers' version to test in real (xvfb-wrapped) browsers
Expand Down Expand Up @@ -258,6 +287,9 @@ workflows:
build-and-test:
jobs:
- install-and-cibuild
- timezone-jasmine:
requires:
- install-and-cibuild
- bundle-jasmine:
requires:
- install-and-cibuild
Expand Down
1 change: 1 addition & 0 deletions draftlogs/5864_fix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Fix period positioned hover to work in different time zones as well as on grouped bars [[#5864](https://github.com/plotly/plotly.js/pull/5864)]
25 changes: 22 additions & 3 deletions src/components/fx/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1993,13 +1993,32 @@ function getCoord(axLetter, winningPoint, fullLayout) {
var ax = winningPoint[axLetter + 'a'];
var val = winningPoint[axLetter + 'Val'];

var cd0 = winningPoint.cd[0];

if(ax.type === 'category') val = ax._categoriesMap[val];
else if(ax.type === 'date') {
var period = winningPoint[axLetter + 'Period'];
val = ax.d2c(period !== undefined ? period : val);
var periodalignment = winningPoint.trace[axLetter + 'periodalignment'];
if(periodalignment) {
var d = winningPoint.cd[winningPoint.index];

var start = d[axLetter + 'Start'];
if(start === undefined) start = d[axLetter];

var end = d[axLetter + 'End'];
if(end === undefined) end = d[axLetter];

var diff = end - start;

if(periodalignment === 'end') {
val += diff;
} else if(periodalignment === 'middle') {
val += diff / 2;
}
}

val = ax.d2c(val);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@archmoj Can you explain why the previous code was timezone-dependent, and how this fixes it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous logic used start and end of period directly which appears to depend on the time zone.
But now we only use the difference between two which should not be impacted by time zone.
We will keep #5861 open until we could further investigate other time zone problems.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah ok, I think I understand it now. The previous one had the advantage of front-loading more of the calculation, so let's try and get back toward that. I think what's going on is when you pull out the period with:

var period = winningPoint[axLetter + 'Period'];

This is already in calcdata format - ie a number - so when we run ax.d2c on it we get the legacy convert-from-local-to-UTC behavior. But val is in data format - ie a string - so it needs ax.d2c. So the right solution is to just change the one line:

val = ax.d2c(period !== undefined ? period : val);

To:

val = period !== undefined ? period : ax.d2c(val);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the hint. There was actually a problem in new code which is fixed now.
Unfortunately trying val = period !== undefined ? period : ax.d2c(val); didn't pass the tests.
However xStart and xEnd are coming from the calc step so the changes should be fast.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm OK - well, this seems to work so let's go with it. Thanks for investigating!

}

var cd0 = winningPoint.cd[winningPoint.index];
if(cd0 && cd0.t && cd0.t.posLetter === ax._id) {
if(
fullLayout.boxmode === 'group' ||
Expand Down
8 changes: 5 additions & 3 deletions src/traces/bar/calc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ var calcSelection = require('../scatter/calc_selection');
module.exports = function calc(gd, trace) {
var xa = Axes.getFromId(gd, trace.xaxis || 'x');
var ya = Axes.getFromId(gd, trace.yaxis || 'y');
var size, pos, origPos, pObj, hasPeriod;
var size, pos, origPos, pObj, hasPeriod, pLetter;

var sizeOpts = {
msUTC: !!(trace.base || trace.base === 0)
Expand All @@ -21,11 +21,13 @@ module.exports = function calc(gd, trace) {
origPos = ya.makeCalcdata(trace, 'y');
pObj = alignPeriod(trace, ya, 'y', origPos);
hasPeriod = !!trace.yperiodalignment;
pLetter = 'y';
} else {
size = ya.makeCalcdata(trace, 'y', sizeOpts);
origPos = xa.makeCalcdata(trace, 'x');
pObj = alignPeriod(trace, xa, 'x', origPos);
hasPeriod = !!trace.xperiodalignment;
pLetter = 'x';
}
pos = pObj.vals;

Expand All @@ -39,8 +41,8 @@ module.exports = function calc(gd, trace) {

if(hasPeriod) {
cd[i].orig_p = origPos[i]; // used by hover
cd[i].pEnd = pObj.ends[i];
cd[i].pStart = pObj.starts[i];
cd[i][pLetter + 'End'] = pObj.ends[i];
cd[i][pLetter + 'Start'] = pObj.starts[i];
}

if(trace.ids) {
Expand Down
8 changes: 0 additions & 8 deletions src/traces/bar/cross_trace_calc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,20 +436,12 @@ function setBarCenterAndWidth(pa, sieve) {
var barwidth = t.barwidth;
var barwidthIsArray = Array.isArray(barwidth);

var trace = calcTrace[0].trace;
var isPeriod = !!trace[pLetter + 'periodalignment'];

for(var j = 0; j < calcTrace.length; j++) {
var calcBar = calcTrace[j];

// store the actual bar width and position, for use by hover
var width = calcBar.w = barwidthIsArray ? barwidth[j] : barwidth;
calcBar[pLetter] = calcBar.p + (poffsetIsArray ? poffset[j] : poffset) + width / 2;

if(isPeriod) {
calcBar.wPeriod =
calcBar.pEnd - calcBar.pStart;
}
}
}
}
Expand Down
26 changes: 17 additions & 9 deletions src/traces/bar/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,18 +52,26 @@ function hoverOnBars(pointData, xval, yval, hovermode, opts) {
}

var period = trace[posLetter + 'period'];
var isClosestOrPeriod = isClosest || period;

function thisBarMinPos(di) { return thisBarExtPos(di, -1); }
function thisBarMaxPos(di) { return thisBarExtPos(di, 1); }

function thisBarExtPos(di, sgn) {
var w = (period) ? di.wPeriod : di.w;
var w = di.w;

return di[posLetter] + sgn * w / 2;
}

var minPos = isClosest || period ?
thisBarMinPos :
function periodLength(di) {
return di[posLetter + 'End'] - di[posLetter + 'Start'];
}

var minPos = isClosest ?
thisBarMinPos : period ?
function(di) {
return di.p - periodLength(di) / 2;
} :
function(di) {
/*
* In compare mode, accept a bar if you're on it *or* its group.
Expand All @@ -80,8 +88,11 @@ function hoverOnBars(pointData, xval, yval, hovermode, opts) {
return Math.min(thisBarMinPos(di), di.p - t.bardelta / 2);
};

var maxPos = isClosest || period ?
thisBarMaxPos :
var maxPos = isClosest ?
thisBarMaxPos : period ?
function(di) {
return di.p + periodLength(di) / 2;
} :
function(di) {
return Math.max(thisBarMaxPos(di), di.p + t.bardelta / 2);
};
Expand Down Expand Up @@ -156,7 +167,7 @@ function hoverOnBars(pointData, xval, yval, hovermode, opts) {
// if we get here and we're not in 'closest' mode, push min/max pos back
// onto the group - even though that means occasionally the mouse will be
// over the hover label.
if(!isClosest) {
if(!isClosestOrPeriod) {
minPos = function(di) {
return Math.min(thisBarMinPos(di), di.p - t.bargroupwidth / 2);
};
Expand All @@ -179,9 +190,6 @@ function hoverOnBars(pointData, xval, yval, hovermode, opts) {

var hasPeriod = di.orig_p !== undefined;
pointData[posLetter + 'LabelVal'] = hasPeriod ? di.orig_p : di.p;
if(hasPeriod) {
pointData[posLetter + 'Period'] = di.p;
}

pointData.labelLabel = hoverLabelText(pa, pointData[posLetter + 'LabelVal'], trace[posLetter + 'hoverformat']);
pointData.valueLabel = hoverLabelText(sa, pointData[sizeLetter + 'LabelVal'], trace[sizeLetter + 'hoverformat']);
Expand Down
8 changes: 5 additions & 3 deletions src/traces/funnel/calc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,20 @@ var BADNUM = require('../../constants/numerical').BADNUM;
module.exports = function calc(gd, trace) {
var xa = Axes.getFromId(gd, trace.xaxis || 'x');
var ya = Axes.getFromId(gd, trace.yaxis || 'y');
var size, pos, origPos, pObj, hasPeriod, i, cdi;
var size, pos, origPos, pObj, hasPeriod, pLetter, i, cdi;

if(trace.orientation === 'h') {
size = xa.makeCalcdata(trace, 'x');
origPos = ya.makeCalcdata(trace, 'y');
pObj = alignPeriod(trace, ya, 'y', origPos);
hasPeriod = !!trace.yperiodalignment;
pLetter = 'y';
} else {
size = ya.makeCalcdata(trace, 'y');
origPos = xa.makeCalcdata(trace, 'x');
pObj = alignPeriod(trace, xa, 'x', origPos);
hasPeriod = !!trace.xperiodalignment;
pLetter = 'x';
}
pos = pObj.vals;

Expand Down Expand Up @@ -55,8 +57,8 @@ module.exports = function calc(gd, trace) {

if(hasPeriod) {
cd[i].orig_p = origPos[i]; // used by hover
cd[i].pEnd = pObj.ends[i];
cd[i].pStart = pObj.starts[i];
cd[i][pLetter + 'End'] = pObj.ends[i];
cd[i][pLetter + 'Start'] = pObj.starts[i];
}

if(trace.ids) {
Expand Down
3 changes: 0 additions & 3 deletions src/traces/scatter/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,6 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) {
hovertemplate: trace.hovertemplate
});

if(trace.xperiodalignment === 'end') pointData.xPeriod = di.x;
if(trace.yperiodalignment === 'end') pointData.yPeriod = di.y;

fillText(di, trace, pointData);
Registry.getComponentMethod('errorbars', 'hoverInfo')(di, trace, pointData);

Expand Down
3 changes: 0 additions & 3 deletions src/traces/scattergl/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,9 +202,6 @@ function calcHover(pointData, x, y, trace) {
hovertemplate: di.ht
});

if(trace.xperiodalignment === 'end') pointData2.xPeriod = di.x;
if(trace.yperiodalignment === 'end') pointData2.yPeriod = di.y;

if(di.htx) pointData2.text = di.htx;
else if(di.tx) pointData2.text = di.tx;
else if(trace.text) pointData2.text = trace.text;
Expand Down
8 changes: 5 additions & 3 deletions src/traces/waterfall/calc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,20 @@ function isTotal(a) {
module.exports = function calc(gd, trace) {
var xa = Axes.getFromId(gd, trace.xaxis || 'x');
var ya = Axes.getFromId(gd, trace.yaxis || 'y');
var size, pos, origPos, pObj, hasPeriod;
var size, pos, origPos, pObj, hasPeriod, pLetter;

if(trace.orientation === 'h') {
size = xa.makeCalcdata(trace, 'x');
origPos = ya.makeCalcdata(trace, 'y');
pObj = alignPeriod(trace, ya, 'y', origPos);
hasPeriod = !!trace.yperiodalignment;
pLetter = 'y';
} else {
size = ya.makeCalcdata(trace, 'y');
origPos = xa.makeCalcdata(trace, 'x');
pObj = alignPeriod(trace, xa, 'x', origPos);
hasPeriod = !!trace.xperiodalignment;
pLetter = 'x';
}
pos = pObj.vals;

Expand Down Expand Up @@ -85,8 +87,8 @@ module.exports = function calc(gd, trace) {

if(hasPeriod) {
cd[i].orig_p = origPos[i]; // used by hover
cd[i].pEnd = pObj.ends[i];
cd[i].pStart = pObj.starts[i];
cd[i][pLetter + 'End'] = pObj.ends[i];
cd[i][pLetter + 'Start'] = pObj.starts[i];
}

if(trace.ids) {
Expand Down
40 changes: 38 additions & 2 deletions test/jasmine/tests/hover_label_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5433,14 +5433,14 @@ describe('hovermode: (x|y)unified', function() {
}
})
.then(function(gd) {
_hover(gd, { xpx: 50, ypx: 200 });
_hover(gd, { xpx: 100, ypx: 200 });
assertLabel({title: 'Jan', items: [
'bar : 1',
'one : 1',
'two : 1',
]});

_hover(gd, { xpx: 350, ypx: 200 });
_hover(gd, { xpx: 300, ypx: 200 });
assertLabel({title: 'Feb', items: [
'bar : 2',
'one : 2',
Expand Down Expand Up @@ -5581,12 +5581,48 @@ describe('hovermode: (x|y)unified', function() {

Plotly.newPlot(gd, fig)
.then(function(gd) {
_hover(gd, { xpx: 50, ypx: 200 });
assertLabel({title: 'Jan 1, 1970', items: [
'trace 0 : 11',
'trace 1 : 1'
]});

_hover(gd, { xpx: 100, ypx: 200 });
assertLabel({title: 'Jan 1, 1970', items: [
'trace 0 : 11',
'trace 1 : 1'
]});

_hover(gd, { xpx: 150, ypx: 200 });
assertLabel({title: 'Jul 1, 1970', items: [
'trace 0 : 12',
'trace 1 : 2'
]});

_hover(gd, { xpx: 200, ypx: 200 });
assertLabel({title: 'Jul 1, 1970', items: [
'trace 0 : 12',
'trace 1 : 2'
]});

_hover(gd, { xpx: 250, ypx: 200 });
assertLabel({title: 'Jul 1, 1970', items: [
'trace 0 : 12',
'trace 1 : 2'
]});

_hover(gd, { xpx: 300, ypx: 200 });
assertLabel({title: 'Jan 1, 1971', items: [
'trace 0 : 13',
'trace 1 : 3'
]});

_hover(gd, { xpx: 350, ypx: 200 });
assertLabel({title: 'Jan 1, 1971', items: [
'trace 0 : 13',
'trace 1 : 3'
]});

_hover(gd, { xpx: 400, ypx: 200 });
assertLabel({title: 'Jan 1, 1971', items: [
'trace 0 : 13',
Expand Down