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 5 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 [[#5864](https://github.com/plotly/plotly.js/pull/5864)]
24 changes: 19 additions & 5 deletions src/components/fx/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1993,19 +1993,33 @@ function getCoord(axLetter, winningPoint, fullLayout) {
var ax = winningPoint[axLetter + 'a'];
var val = winningPoint[axLetter + 'Val'];

var trace = winningPoint.trace;
var cd = winningPoint.cd;
var d = cd[winningPoint.index];

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 = trace[axLetter + 'periodalignment'];
if(periodalignment) {
var start = d[axLetter + 'Start'];
var end = d[axLetter + 'End'];
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(d && d.t && d.t.posLetter === ax._id) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code has been around for ages but I'm just noticing it now and it confused the heck out of me: normally the t object only exists on the first entry in cd (that's why the var we extracted it from was called cd0) because it contains extra calculated info that applies to the whole trace. And at the calc step that's true of box and violin too. But then during the plot step it appears these traces add t to all cd entries so that if you draw points, the one box the points are for will look like a whole trace to the reused scatter code with which we're drawing the points.

So yes, this works... but only due to a quirk of our drawing code and only for box and violin, so it feels fragile. Nonblocking, but when we need t we should really get it from cd[0].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What you suggesting here is renaming d back to cd0? Or I should simply add a comment above cd0.d?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm saying instead of assuming box & violin will have t in every element of cd (d is cd[winningPoint.index] and winningPoint.index need not be zero) we should be using cd[0].t.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 3e52b94

if(
fullLayout.boxmode === 'group' ||
fullLayout.violinmode === 'group'
) {
val += cd0.t.dPos;
val += d.t.dPos;
}
}

Expand Down
3 changes: 0 additions & 3 deletions src/traces/bar/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,9 +179,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
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
4 changes: 2 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