Right click fix #2241

alexcjohnson · 2018-01-10T10:23:34Z

fixes #2101
supersedes #2105 - @AlexVvx I started with your PR, thinking I could just add the right tests to it and be done... unfortunately I found that we had some deeper problems in both the source code and in the tests so it became a much bigger project.

The test problem is that assets/click was unrealistic - it generated a sequence of events that's fine for regular clicks, but right clicks and ctrl-clicks (which we were using all over the place to test "modified click behavior", making a bunch of trace types look like they supported right-click when they really didn't) take a different pathway, and importantly for some users, it also matters whether the contextmenu event is canceled.

The source problem is that we're inconsistent with what we call a click - we have parallel systems handling drag and click events. This isn't only a problem for right clicks (which don't trigger click events at all, so it's a huge problem for them.), it's also inconsistent in subtle ways for our other interactions. For example, independent systems might see both a drag (to make a pan, zoom, or select) AND a click; or they might see neither one! Neither situation is desirable: users should be able to expect that one mouse action will create one response. So I changed the way dragElement works: it now takes an extra option, clickFn, ie what to do in the event of a click. Now you know that the sequence of events in a dragElement action will be either:

prepFn -> moveFn 1 or more times -> doneFn (a drag, even if it ends exactly where it started)

or:

prepFn -> clickFn (a click)

This has a nice side effect that doneFn can be much simpler, it only has one argument (the original event) and doesn't need all the if(dragged) branches it had before. clickFn is also pretty simple, and to my eye keeping the two separate makes them much more readable.

There are a whole bunch of TODOs scattered about in here. Before we merge this we should probably collect them into a new issue or two and 🔪 them from the code - most are about right-click scenarios we could support but currently do not, there's at least one about an unrelated interaction issue I encountered along the way. I don't think any are necessary for 1.32 though.

@etpinard please take a look!

also fix a bunch of incorrect tests because assets/click didn't have the right behavior and document places we don't have any right-click support presently

etpinard · 2018-01-10T15:40:27Z

src/components/legend/draw.js

-                            }
+                }
+            },
+            clickFn: function(numClicks, e) {


Wow. This looks so much cleaner by splitting done and click logic. 💯

etpinard · 2018-01-10T15:43:15Z

src/plots/cartesian/graph_interact.js

@@ -84,10 +84,6 @@ module.exports = function initInteractions(gd) {
                dragElement.unhover(gd, evt);
            };

-            maindrag.onclick = function(evt) {
-                Fx.click(gd, evt, subplot);


this is now defined in cartesian/dragbox.js, correct?

this is now defined in cartesian/dragbox.js, correct?

yep, now it's here: https://github.com/plotly/plotly.js/pull/2241/files#diff-1bd3dd52cddb8638876bbe17cef28a0dR175

etpinard · 2018-01-10T15:47:38Z

src/traces/pie/plot.js

@@ -170,6 +170,11 @@ module.exports = function plot(gd, cdpie) {
                }

                function handleClick() {
+                    // TODO: this does not support right-click. If we want to support it, we
+                    // would likely need to change mapbox to use dragElement instead of straight


we would need to change pie to use dragElement,

correct?

Perhaps we could discuss future dragElement generalizations in #2057?

table, sankey and parcoords would also benefit from using dragElement.

comment added to #2057 - I'll leave the TODOs present in the code then unless you object.

etpinard · 2018-01-10T16:01:09Z

test/jasmine/tests/click_test.js

        });

        it('should contain the correct fields', function() {
            click(pointPos[0], pointPos[1]);
            expect(futureData.points.length).toEqual(1);
+            expect(clickPassthroughs).toBe(2);


so, in a real world scenario, this here would be .toBe(0)?

i.e. plotly_click eats up gd.on('click')?

real world it would be 1. dragElement eats up one click event but generates a new one. That was important in certain cases when we were using that synthetic event to trigger the selection.on('click') to trigger the plotly_click. We could consider removing it now, per this TODO so it doesn't bubble up the tree after we've already generated our own events from it, but leaving it in is the most backward-compatible way to handle this.

etpinard · 2018-01-10T16:03:05Z

test/jasmine/tests/click_test.js

+            // then we pass right-click on data points through to plotly_click events.
+            // Devs using this need to be aware then to check eventData.event
+            // and figure out if it had a button or ctrlKey etc.
+            it('should contain the correct fields', function() {


Amazing. Thanks for 🔒 this down!

etpinard · 2018-01-10T16:09:15Z

test/jasmine/tests/dragelement_test.js

+                expect('should not call moveFn').toBe(true);
+            },
+            doneFn: function() {
+                expect('should not call doneFn').toBe(true);


Oh nice. I forget we had unit tests for dragElement 👌

etpinard · 2018-01-10T16:11:58Z

test/jasmine/tests/select_test.js

-// event data is undefined
-var BOXEVENTS = [1, 2, 1];
+// deselect used to give an extra plotly_selected event on the first click
+// with undefined event data - but now that's gone, since `clickFn` handles this.


Oh. Some users might currently rely on plotly_selected sending undefined as event data.

put back the old behavior in 974cebf and commented in #420 about removing it later.

etpinard · 2018-01-10T16:15:50Z

Well done. I'm a big fan of the doneFn / clickFn split in dragElement 🎉

I'm a little concern with how that affects plotly_selected event listeners that expect undefined (see #2241 (comment)). I hope there's a way to patch cartesian/select.js to leave the select_test.js suite unchanged, I don't think we can consider this a bug fix (in v1 at least).

to be removed for good in v2

etpinard · 2018-01-10T19:29:14Z

Looks amazing 💃

Alex Vinober and others added 3 commits January 9, 2018 12:44

Restore dispatching 'click' on right-click

426ffa5

Refactored

7917640

refactor dragElement to handle clicks

5b6a1fa

also fix a bunch of incorrect tests because assets/click didn't have the right behavior and document places we don't have any right-click support presently

alexcjohnson added bug something broken status: reviewable labels Jan 10, 2018

alexcjohnson added this to the v1.32.0 milestone Jan 10, 2018

etpinard reviewed Jan 10, 2018

View reviewed changes

etpinard mentioned this pull request Jan 10, 2018

Some folks are using right-click on data points in their apps #2101

Closed

etpinard reviewed Jan 10, 2018

View reviewed changes

alexcjohnson added 2 commits January 10, 2018 09:52

tyop

149dd74

put back in the plotly_selected->undefined event

974cebf

to be removed for good in v2

This was referenced Jan 10, 2018

wishlist for potential breaking changes since v1 #420

Closed

Centralize drag handlers #2057

Closed

alexcjohnson merged commit 8cc5f55 into master Jan 10, 2018

alexcjohnson deleted the right-click-fix branch January 10, 2018 20:30

etpinard referenced this pull request Jan 17, 2018

Add select/lasso click event test

091883c

This was referenced Jan 18, 2018

plotly_click doesn't fire when fixedrange is true #2265

Closed

Fixedrange click #2279

Merged

ixxie mentioned this pull request Mar 31, 2025

Right click triggers drag event on some machines & fails to fire plotly_click #7398

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Right click fix #2241

Right click fix #2241

alexcjohnson commented Jan 10, 2018

etpinard Jan 10, 2018

etpinard Jan 10, 2018 •

edited

Loading

alexcjohnson Jan 10, 2018

etpinard Jan 10, 2018

etpinard Jan 10, 2018

alexcjohnson Jan 10, 2018

etpinard Jan 10, 2018

etpinard Jan 10, 2018 •

edited

Loading

alexcjohnson Jan 10, 2018

etpinard Jan 10, 2018

etpinard Jan 10, 2018

etpinard Jan 10, 2018

alexcjohnson Jan 10, 2018

etpinard commented Jan 10, 2018

etpinard commented Jan 10, 2018

Right click fix #2241

Right click fix #2241

Conversation

alexcjohnson commented Jan 10, 2018

Choose a reason for hiding this comment

etpinard Jan 10, 2018 • edited Loading

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

etpinard Jan 10, 2018 • edited Loading

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

etpinard commented Jan 10, 2018

etpinard commented Jan 10, 2018

etpinard Jan 10, 2018 •

edited

Loading

etpinard Jan 10, 2018 •

edited

Loading