Fix unhover event data for gl3d subplots #5954

dwoznicki · 2021-09-29T00:40:41Z

This is a potential fix for #5953. I've modified the plotly_unhover event to use the event data from the last plotly_click/plotly_hover event, which I think was the original intention, but I'm not totally sure.

archmoj · 2021-09-30T15:07:52Z

Thanks very much for the PR.
It is looking good to me.
To lock this, please add an unhover test to https://github.com/plotly/plotly.js/blob/master/test/jasmine/tests/gl3d_hover_click_test.js similar to gl2d/cartesian "click" tests.

You could run the test using the following command:

npm run test-jasmine gl3d_hover_click

dwoznicki · 2021-09-30T21:18:53Z

I'll just quickly note that a bunch of tests were failing for me when running npm run test-jasmine, even before I made any changes. The problem was that the browser window opened by jasmine was too narrow (the tests succeeded when I fullscreened the jasmine browser window). It may be worth mentioning this little quirk in CONTRIBUTING.md or elsewhere.

archmoj · 2021-09-30T21:38:41Z

I'll just quickly note that a bunch of tests were failing for me when running npm run test-jasmine, even before I made any changes. The problem was that the browser window opened by jasmine was too narrow (the tests succeeded when I fullscreened the jasmine browser window). It may be worth mentioning this little quirk in CONTRIBUTING.md or elsewhere.

Yes please feel free to open a PR to improve the CONTRIBUTING.md.

dwoznicki · 2021-09-30T23:12:13Z

@archmoj It looks like the test I wrote is failing probably due to a race condition. Basically, the plotly_unhover event gets triggered every time the gl3d Scene.render function gets called without a last picked trace, which results in multiple unhover events with undefined eventData.

I can modify the test to avoid the race, but this behavior does strike me as a bit unexpected. My expectation is that the plotly_unhover event would only be fired when the user has hovered a point, then moves the mouse off of that point, not when the graph re-renders. This can be accomplished easily enough by changing

Fx.loneUnhover(svgContainer);
gd.emit('plotly_unhover', this.oldEventData);
this.oldEventData = undefined;

to

if (this.oldEventData) {
    Fx.loneUnhover(svgContainer);
    gd.emit('plotly_unhover', this.oldEventData);
    this.oldEventData = undefined;
}

But I'm not sure what the implications of such a change would be, especially if other code relies on plotly_unhover to be fired every time there is a re-render.

archmoj · 2021-10-04T15:45:03Z

test/jasmine/tests/gl3d_hover_click_test.js

@@ -550,6 +550,77 @@ describe('Test gl3d trace click/hover:', function() {
        .then(done, done.fail);
    });

+    it('@gl should emit correct event data on unhover', function(done) {


Let's move this test to the end of Test gl3d trace click/hover: block and it will solve the race condition.

Hmm, well, I tried moving the test to the bottom of the Test gl3d trace click/hover: block, and the race still appears to be present.

To be clear, I don't think there is a race between this test and other tests; as far as I can tell, they're run sequentially in the browser. I do think there's a race to check ptData before it is reassigned by a pseudo unhover event triggered by the graph re-rendering. For example, if I change

gd.on('plotly_unhover', function(eventData) { ptData = eventData.points[0]; });

to

gd.on('plotly_unhover', function(eventData) { console.log("UNHOVER", eventData); if (eventData) { ptData = eventData.points[0]; } else { ptData = {}; } });

and run the full gl3d_hover_click test suite, I get logging output

LOG: 'UNHOVER', undefined LOG: 'UNHOVER', undefined LOG: 'UNHOVER', Object{points: [Object{x: ..., y: ..., z: ..., data: ..., fullData: ..., curveNumber: ..., pointNumber: ..., marker.symbol: ..., marker.size: ..., marker.line.color: ..., marker.color: ..., bbox: ...}]} LOG: 'UNHOVER', undefined LOG: 'UNHOVER', undefined

I assume the undefined event data is due to Scene.prototype.render being called a bunch of times for reasons unrelated to mouse movement.

Interestingly, I don't get any unhover events with undefined event data when I run this test as a standalone using fit.

archmoj · 2021-10-04T15:46:50Z

Please create draftlog/5954_fix.md file as described here for example

 - Fix unhover event data for gl3d subplots [[#5954](https://github.com/plotly/plotly.js/pull/5954)],
   with thanks to @dwoznicki for the contribution!

Thank you!

…over being called without even data remains

archmoj · 2021-10-05T12:45:55Z

src/plots/gl3d/scene.js

@@ -456,10 +454,11 @@ proto.render = function() {
            gd.emit('plotly_hover', eventData);
        }

-        oldEventData = eventData;
+        this.oldEventData = eventData;


What if we change this line to

if(eventData) this.oldEventData = eventData;

?

I don't think this does the trick, unfortunately. Here's the logging output with this change.

LOG: 'UNHOVER', undefined LOG: 'UNHOVER', undefined LOG: 'UNHOVER', Object{points: [Object{x: ..., y: ..., z: ..., data: ..., fullData: ..., curveNumber: ..., pointNumber: ..., marker.symbol: ..., marker.size: ..., marker.line.color: ..., marker.color: ..., bbox: ...}]} LOG: 'UNHOVER', undefined LOG: 'UNHOVER', undefined

this.oldEventData still gets set back to undefined after the plotly_unhover event has been emitted.

gd.emit('plotly_unhover', this.oldEventData); this.oldEventData = undefined;

I could remove this.oldEventData = undefined; and then we could be pretty confident that the unhover event will have event data, as long as the user has hovered at least one point since graph creation (we'll still get undefined event data for renders before a point has been hovered). Here's the logging output.

LOG: 'UNHOVER', undefined LOG: 'UNHOVER', undefined LOG: 'UNHOVER', Object{points: [Object{x: ..., y: ..., z: ..., data: ..., fullData: ..., curveNumber: ..., pointNumber: ..., marker.symbol: ..., marker.size: ..., marker.line.color: ..., marker.color: ..., bbox: ...}]} LOG: 'UNHOVER', Object{points: [Object{x: ..., y: ..., z: ..., data: ..., fullData: ..., curveNumber: ..., pointNumber: ..., marker.symbol: ..., marker.size: ..., marker.line.color: ..., marker.color: ..., bbox: ...}]} LOG: 'UNHOVER', Object{points: [Object{x: ..., y: ..., z: ..., data: ..., fullData: ..., curveNumber: ..., pointNumber: ..., marker.symbol: ..., marker.size: ..., marker.line.color: ..., marker.color: ..., bbox: ...}]}

So this would solve the issue for the test case, but I think this behavior is misleading.

archmoj · 2021-10-07T13:29:29Z

src/plots/gl3d/scene.js

    } else {
        Fx.loneUnhover(svgContainer);
-        gd.emit('plotly_unhover', oldEventData);
+        gd.emit('plotly_unhover', this.oldEventData);


Suggested change

gd.emit('plotly_unhover', this.oldEventData);

if(this.oldEventData) gd.emit('plotly_unhover', this.oldEventData);

Perhaps we shouldn't fire Fx.loneUnhover either if this.oldEventData is nullish?

if (this.oldEventData) { Fx.loneUnhover(svgContainer); gd.emit('plotly_unhover', this.oldEventData); this.oldEventData = undefined; }

Maybe. But that could possibly be done in a separate PR.

@dwoznicki did you try my suggestion above? #5954 (comment)

Yeah, and it solves the issue as far as I can tell.

Then please commit the suggestion :)

Ah, I did here 46c5b8f, but I did it in my local copy instead of through the GitHub UI. Is that okay?

…is present

archmoj · 2021-10-12T12:31:24Z

Great. Would you mind fixing the syntax failure and we should be good to go.

npm run test-syntax

dwoznicki · 2021-10-12T17:45:32Z

Hmm, I'm not sure what the syntax failure is. When I run npm run test-syntax, I don't see any errors.

> plotly.js@2.5.1 test-syntax
> node tasks/test_syntax.js && npm run find-strings -- --no-output

ok no jasmine suites focus/exclude blocks or wrong tag patterns
ok circular dependencies: 0
ok correct headers and contents in lib/ and src/
ok lower case only file names
ok trailing new line character

> plotly.js@2.5.1 find-strings
> node tasks/find_locale_strings.js "--no-output"

ok find_locale_strings - no output requested.

Any ideas?

archmoj · 2021-10-12T17:48:56Z

Please try
npm run lint

archmoj · 2021-10-12T17:52:28Z

These two lines are causing the error:

/home/circleci/plotly.js/test/jasmine/tests/gl3d_hover_click_test.js

1316:19 error Unexpected space(s) after "if" keyword-spacing

1345:8 error Block must not be padded by blank lines padded-blocks

archmoj · 2021-10-12T18:27:25Z

💃
Thanks very much for the contribution 🎖️

Fixed a bug where event data in scatter3d charts was always undefined

cc9b695

archmoj added bug community labels Sep 29, 2021

archmoj requested review from nicolaskruchten and alexcjohnson September 29, 2021 18:03

dwoznicki added 2 commits September 30, 2021 14:08

Added test to make sure gl3d unhover event data is correct

12fc1f7

Removed fit to make jasmine run single unhover test

0ef9afd

archmoj added the status: reviewable label Oct 4, 2021

archmoj changed the title ~~Fix scatter3d unhover data undefined bug~~ Fix unhover event data for gl3d subplots Oct 4, 2021

archmoj reviewed Oct 4, 2021

View reviewed changes

archmoj added the status: has TODOs label Oct 4, 2021

WIP Fixed a null pointer in the test, but the underlying issue of unh…

3ae7ef1

…over being called without even data remains

archmoj reviewed Oct 5, 2021

View reviewed changes

archmoj reviewed Oct 7, 2021

View reviewed changes

dwoznicki added 2 commits October 11, 2021 10:46

Updated gl3d scene to only fire plotly_unhover event when event data …

46c5b8f

…is present

Added draftlog

bac8737

Fixed a couple formatting issues in the new test

6d3b3c0

archmoj removed the status: has TODOs label Oct 12, 2021

archmoj merged commit 683f601 into plotly:master Oct 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

Fix unhover event data for gl3d subplots #5954

Fix unhover event data for gl3d subplots #5954

dwoznicki commented Sep 29, 2021

archmoj commented Sep 30, 2021

dwoznicki commented Sep 30, 2021

archmoj commented Sep 30, 2021

dwoznicki commented Sep 30, 2021

archmoj Oct 4, 2021

dwoznicki Oct 4, 2021

archmoj commented Oct 4, 2021

archmoj Oct 5, 2021

dwoznicki Oct 5, 2021

archmoj Oct 7, 2021

dwoznicki Oct 7, 2021

archmoj Oct 7, 2021

archmoj Oct 8, 2021

dwoznicki Oct 11, 2021

archmoj Oct 11, 2021

dwoznicki Oct 11, 2021

archmoj commented Oct 12, 2021

dwoznicki commented Oct 12, 2021

archmoj commented Oct 12, 2021

archmoj commented Oct 12, 2021

archmoj commented Oct 12, 2021

	gd.emit('plotly_unhover', this.oldEventData);
	if(this.oldEventData) gd.emit('plotly_unhover', this.oldEventData);

Fix unhover event data for gl3d subplots #5954

Fix unhover event data for gl3d subplots #5954

Conversation

dwoznicki commented Sep 29, 2021

archmoj commented Sep 30, 2021

dwoznicki commented Sep 30, 2021

archmoj commented Sep 30, 2021

dwoznicki commented Sep 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archmoj commented Oct 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archmoj commented Oct 12, 2021

dwoznicki commented Oct 12, 2021

archmoj commented Oct 12, 2021

archmoj commented Oct 12, 2021

archmoj commented Oct 12, 2021