Pass MouseEvent to the user by n-riesco · Pull Request #9 · n-riesco/plotly.js

n-riesco · 2017-03-15T15:10:51Z

This PR addresses plotly#988 for scatter plots.

* Include the mouse event in the event data returned by `plotly_click`, `plotly_hover` and `plotly_unhover`.

* Test event property in `plotly_click`, `plotly_hover` and `plotly_unhover`.

etpinard · 2017-03-16T02:14:15Z

        if(!gd._dragged) {
-            var e2 = document.createEvent('MouseEvents');
-            e2.initEvent('click', true, true);
+            var e2 = new MouseEvent('click', e);


Oh. I think that will break click in IE9 and even up to IE11.

I'd be ok with something like:

var e2 try { e2 = new MouseEvent('click', e); } catch(e) { e2 = document.createEvent('MouseEvents'); e2.initEvent('click', true, true); }

and hence only passing MouseEvent in browsers that supports it.

@n-riesco what do you think?

@etpinard How about initMouseEvent?

MDN
MSDN

etpinard · 2017-03-16T02:16:10Z

+
+            var evt = futureData.event;
+            expect(evt.clientX).toEqual(pointPos[0]);
+            expect(evt.clientY).toEqual(pointPos[1]);


Maybe we could a test case that has a ctrlKey pressed? ... to really 🔒 down the requirement,

* Keep compatibility with IE9.

n-riesco · 2017-03-16T12:51:13Z

        if(!gd._dragged) {
            var e2 = document.createEvent('MouseEvents');
-            e2.initEvent('click', true, true);
+            e2.initMouseEvent('click', true, true,


@etpinard Shall I keep canBubble and cancelable set to true? Or copy the the values from e?

I don't have a strong opinion here.

Oh, looks like initMouseEvent is deprecated. We should try using new MouseEvent first and then fallback to initMouseEvent in browsers that don't support it.

etpinard · 2017-03-16T14:05:03Z

    if(opts && opts.buttons) {
        fullOpts.buttons = opts.buttons;
    }
+    if(opts && opts.altKey) {


Nice. Thanks!

etpinard · 2017-03-16T14:06:00Z

+            expect(evt.clientX).toEqual(pointPos[0]);
+            expect(evt.clientY).toEqual(pointPos[1]);
+            Object.getOwnPropertyNames(clickOpts).forEach(function(opt) {
+                expect(evt[opt]).toEqual(clickOpts[opt], opt);


etpinard · 2017-03-16T14:11:36Z

    }

    gd.emit('plotly_hover', {
+        event: evt,


Great. This will work for all subplots that call Fx.hover if I remember correctly that means: cartesian, ternary, geo and mapbox subplots. @n-riesco would you mind double checking?

Now, at the very least, we should make sure event is passed in pie chart hover data. Pie hover is a little different but shouldn't make it too hard to implement.

This leaves our gl2d, gl3d and parcoords (and polar but who cares about polar?). I'll be ok leaving them out of the first iteration. We should instead spent some time down the road merging their hover logic with Fx.hover.

@etpinard It doesn't work for pie, ternary, geo or mapbox, because when they call to Fx.click(gd, evt) they make up an evt.

I reckon that to make it work for all these types of subplots, first I need to locate where they call Fx.click, Fx.hover and Fx.unhover and then see if it's possible to send the original event.

For example, here: why do we need to set the event to {target: true}? Is is OK if we just do Fx.click(gd, d3.event);

Is is OK if we just do Fx.click(gd, d3.event);

That sounds ok to me, yes.

* Since `initMouseEvent` is deprecated, we try `new MouseEvent()` first.

* Added click event after mouseup, otherwise click events in pie plots don't get triggered.

n-riesco · 2017-03-17T19:35:52Z

@etpinard I still have to add tests for the geo and mapbox cases. I'll do it later in the evening.

n-riesco · 2017-03-20T10:37:49Z

@etpinard This PR is ready for review again.

etpinard · 2017-03-20T14:32:43Z

-    map.on('click', function() {
-        Fx.click(gd, { target: true });
+    map.on('click', function(evt) {
+        Fx.click(gd, evt.originalEvent);


Very nice here. Thanks!

etpinard · 2017-03-20T14:33:58Z

 var Lib = require('@src/lib');
 var Drawing = require('@src/components/drawing');
 var DBLCLICKDELAY = require('@src/constants/interactions').DBLCLICKDELAY;
+var HOVERMINTIME = 50;


Let's require this value instead of hard-coding it.

etpinard · 2017-03-20T14:35:48Z

 });

+
+describe('Test click interactions on a pie plot:', function() {


While at it, @n-riesco would be mind resolving:

onClick for pies slices doesn't return any trace info plotly/plotly.js#1456 and

Custom hover event for pie chart fail when hoverinfo set to 'none' plotly/plotly.js#902

This should be pretty easy. Thanks in advance! ✅

etpinard · 2017-03-20T14:36:37Z

+});
+
+
+describe('Test click interactions on a geo plot:', function() {


Let's move these to https://github.com/plotly/plotly.js/blob/master/test/jasmine/tests/geo_test.js

etpinard · 2017-03-20T14:38:00Z

 });

+
+describe('Test click interactions on a pie plot:', function() {


Also, can you move these cases to https://github.com/plotly/plotly.js/blob/master/test/jasmine/tests/hover_pie_test.js

I like separating our test cases per subplot types.

etpinard · 2017-03-20T14:38:45Z

+});
+
+
+describe('Test click interactions on a ternary plot:', function() {


Similarly, move to https://github.com/plotly/plotly.js/blob/master/test/jasmine/tests/ternary_test.js

etpinard · 2017-03-20T14:39:35Z


+var mouseEvent = require('../assets/mouse_event');
+var click = require('../assets/click');
+var HOVERMINTIME = 50;


(repeat) require from cartesian contants.js

etpinard · 2017-03-20T14:40:16Z

 });
+
+
+describe('@noCI Test plotly events on a scattermapbox plot:', function() {


Looks good. Thanks!

etpinard · 2017-03-20T14:41:53Z

@n-riesco This PR looks great! PR away in the main repo whenever you're ready 🚀

* To document the need for an if-block.

n-riesco · 2017-03-20T17:14:41Z

@etpinard I'll address plotly#1456 and plotly#902 tomorrow.

I'll also squash some of the commits before opening a PR in the main repo.

* Test that 'plotly_hover' is triggered when `hoverinfo` is set to `"none"`.

Fixes plotly#902

* Test that 'plotly_click' doesn't emit an undefined trace.

Fixes plotly#1456

n-riesco added 2 commits March 15, 2017 12:30

events: plotly_click, plotly_hover, plotly_unhover

580d5fe

* Include the mouse event in the event data returned by `plotly_click`, `plotly_hover` and `plotly_unhover`.

events: update tests

fe4239b

* Test event property in `plotly_click`, `plotly_hover` and `plotly_unhover`.

etpinard reviewed Mar 16, 2017

View reviewed changes

n-riesco added 2 commits March 16, 2017 12:43

events: test modified clicks

7a90972

events: do not use new MouseEvent(evt)

aea866e

* Keep compatibility with IE9.

n-riesco commented Mar 16, 2017

View reviewed changes

etpinard reviewed Mar 16, 2017

View reviewed changes

n-riesco added 9 commits March 16, 2017 14:52

events: try-catch new MouseEvent(type, evt)

e809b23

* Since `initMouseEvent` is deprecated, we try `new MouseEvent()` first.

test: update helper function click

2e6fb16

* Added click event after mouseup, otherwise click events in pie plots don't get triggered.

events: test plotly_click in a pie plot

550a257

events: fix plotly_click in pie plots

d7e56cf

events: test plotly_hover in pie plots

40dc522

events: add event to plotly_hover

85f72ff

events: test plotly_unhover in pie plots

5f5bb9e

events: add event to plotly_unhover

8d5697b

events: test plotly events in ternary plots

9cf50c2

n-riesco added 4 commits March 17, 2017 23:51

events: fix plotly events in geo plots

27eb8c3

events: test plotly events in geo plots

3584bba

events: fix plotly events in mapbox plots

60ee153

events: test plotly events in mapbox plots

dcdda6f

etpinard reviewed Mar 20, 2017

View reviewed changes

n-riesco added 6 commits March 20, 2017 15:32

events: define variable hasUserCalledHover

194243d

* To document the need for an if-block.

events: do not hard-code HOVERMINTIME

20ff1a0

events: move geo tests to get_test.js

3c41961

event: fix regression in pie interactions

00baba0

events: move pie tests to hover_pie_test.js

5cdf009

events: move ternary tests to ternary_test.js

76e382e

n-riesco added 4 commits March 21, 2017 13:38

pie: test issue plotly#902

f679bbf

* Test that 'plotly_hover' is triggered when `hoverinfo` is set to `"none"`.

pie: emit plotly_hover even if hoverinfo is none

91cc175

Fixes plotly#902

pie: test issue plotly#1456

499c393

* Test that 'plotly_click' doesn't emit an undefined trace.

pie: Fix undefined trace in 'plotly_click'

9e10119

Fixes plotly#1456

		});


		describe('Test click interactions on a pie plot:', function() {

		});


		describe('Test click interactions on a geo plot:', function() {

		});


		describe('Test click interactions on a ternary plot:', function() {

		});


		describe('@noCI Test plotly events on a scattermapbox plot:', function() {

Conversation

n-riesco commented Mar 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etpinard Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n-riesco Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n-riesco commented Mar 17, 2017

Uh oh!

n-riesco commented Mar 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etpinard commented Mar 20, 2017

Uh oh!

n-riesco commented Mar 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

etpinard Mar 16, 2017 •

edited

Loading

n-riesco Mar 16, 2017 •

edited

Loading