Skip to content
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.

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

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

Closed
etpinard opened this issue Oct 18, 2017 · 16 comments · Fixed by #2241
Closed

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

etpinard opened this issue Oct 18, 2017 · 16 comments · Fixed by #2241
Labels
regression this used to work
Milestone

Comments

@etpinard
Copy link
Contributor

etpinard commented Oct 18, 2017

... and PR #2087 broke that 💣

See https://community.plot.ly/t/plotly-click-event-stopped-firing-after-update-on-right-click/6353 and https://community.plot.ly/t/annotation-right-click/6352.

What should we do?

@Melmoth-the-Wanderer
Copy link

Melmoth-the-Wanderer commented Oct 19, 2017

It's already on the production (right-click detection using plotly_click) and would be nice if we aren't stuck with Plotly 1.30 forever :)

Config option looks fine for us, however I don't fully understand the purpose of #2087 .
The need for custom contextmenu is obvious, but you can still disable browser's context menu on plotl's placeholder and build custom one on right click, as we do. That's even more powerful if you can use Plotly event with points passed to the handler (necessary for us). Same applies to plotly_onannotationclick etc. This gives us a possibility to create a different contextmenu based on what's clicked.

Am I missing something important here?

@alexcjohnson
Copy link
Collaborator

@Melmoth-the-Wanderer we'll make sure you don't get stuck! Can you show a code snippet of how your custom contextmenu works? Then we can see if that works for @AlexVvx's use case and/or figure out if we can't make something cleaner and easier for everyone.

@etpinard etpinard added this to the v1.32.0 milestone Oct 19, 2017
@AlexVvx
Copy link

AlexVvx commented Oct 19, 2017

The problem was, that right click worked well on legend zone, but not on chart zone. Drag layer placed on top level, in order to catch context menu event we have to listen on the top, e.g. document level, and guess target of the click.
Looks like problem is that onDone not fired and initialTarget.dispatchEvent(e2) not fired too.

@AlexVvx
Copy link

AlexVvx commented Oct 19, 2017

Please take a look at pr #2105

@Melmoth-the-Wanderer
Copy link

Melmoth-the-Wanderer commented Oct 20, 2017

I cannot share the exact code with you but I'll prepare some demo of simplified use case in couple of days.
Thanks for the PR @AlexVvx.

@alexcjohnson
Copy link
Collaborator

@AlexVvx thanks for the PR and apologies for the delay in discussing it. Your solution looks promising, but I think what we need at this point is a clear set of tests covering all the situations people can encounter with right clicks, and how they are expected to behave. Otherwise whatever we do is going to bother somebody and we'll be back here again.

Seems to me the most flexible (though not necessarily the easiest to use) is maybe:

  • right-click on any data point/element - we should emit a plotly_click event and you can look at e.event.button(s) to determine if it was a right click
  • right-click over a subplot but away from data points (@etpinard help me out, I swear we already had an issue for regular left clicks this way?) - ideally we still emit an event, but maybe it can't be plotly_click or maybe it can be, just with empty e.points? Either way you look at e.event.button(s) to determine if it's a right click.
  • right-click on annotations - plotly_clickannotation, e.event.button(s)
  • right-click on legend (see hover events for labels/legends #65)
  • right-click off of subplots (axes, margins...) - passes through to graphDiv.oncontextmenu?
  • anything else?

@etpinard
Copy link
Contributor Author

(@etpinard help me out, I swear we already had an issue for regular left clicks this way?)

I found #145 (comment)

@AlexVvx
Copy link

AlexVvx commented Oct 26, 2017

@alexcjohnson, sounds good, thank you. I wonder what will happen if we'll trigger plotly_click on right click, if handlers ready for it, they might not check whether it is right or left click. This event may cause left click action on right click. On previous version there were no event on right click.
May be it will be better to trigger plotly_contextmenu?
Browser context menu might be cancelled too.

@Melmoth-the-Wanderer
Copy link

Melmoth-the-Wanderer commented Oct 26, 2017

Currently we're capturing plotly_click and manually checking if it's left or right click (event.which). We're also disabling browser's contextmenu on plotly placeholder so we can build our own contextmenu when right click is detected. In this case firing different event called plotly_contextmenu seems just fine, as @AlexVvx proposed.
The question is - is it ok not to fire onclick when user clicks an element with right mouse button? I cannot make up my mind on which is better/preferred/more semantic from user's perspective.

Despite of that, @alexcjohnson's propositions are great and I really like the idea of

  • right-click over a subplot but away from data points (...) emit an event (...) with empty e.points.

We're also capturing click event (standard one) when user clicks away from data points (we than calculate when the click occurs on the graph related to traces), but it would be great to have plotly event data (with empty points list) available.

  • right-click on annotations - plotly_clickannotation, e.event.button(s)

There is already an event called plotly_annotationclick, I believe (or it used to exists). It just doesn't (didn't) fire on rightclick / contextmenu, unless I'm wrong.

  • right-click off of subplots (axes, margins...) - passes through to graphDiv.oncontextmenu?

Would be nice to have a possibility to determine which element has been clicked (was it x or y axis?) - is it even possible? I can imagine that e.axis could be passed alongside with mouse event (same as in case of points) when we click the axis. Use case: with multiple y axes we can build a contextmenu with actions for specific axis. (eg. to move axis right or left, create subplot for this axis etc).

@etpinard
Copy link
Contributor Author

On previous version there were no event on right click.

That's not true: https://codepen.io/etpinard/pen/EbxxLo?editors=0010

@etpinard
Copy link
Contributor Author

right-click over a subplot but away from data points (@etpinard help me out, I swear we already had an issue for regular left clicks this way?) - ideally we still emit an event, but maybe it can't be plotly_click or maybe it can be, just with empty e.points? Either way you look at e.event.button(s) to determine if it's a right click.

This would make a lot of users happy, but I would hold back on this until we can bring support for off-point plotly_click to all subplot types.

@etpinard
Copy link
Contributor Author

right-click on any data point/element - we should emit a plotly_click event and you can look at e.event.button(s) to determine if it was a right click
right-click on annotations - plotly_clickannotation, e.event.button(s)
right-click on legend (see #65)
right-click off of subplots (axes, margins...) - passes through to graphDiv.oncontextmenu?
anything else?

I agree with all of these 👍

@etpinard
Copy link
Contributor Author

Thinking about this some more, I'd say @AlexVvx 's #2105 is probably just what we need for the v1 series. We should for sure revisit some of our assumptions during to our v2 push (cc #145), but at this stage #2015 seems like the more robust solution that preserves backward compatibility and makes everyone here happy.

Any objections?

@alexcjohnson
Copy link
Collaborator

Any objections?

If it works for @AlexVvx that's a great start. I'd love a 👍 /👎 from @Melmoth-the-Wanderer

Are there any other specific conditions folks depend on that we could add a test for, so we're not back here again next release? How many of the propositions in #2101 (comment) have we tested, and do they all behave as proposed?

@Melmoth-the-Wanderer
Copy link

👍

@etpinard
Copy link
Contributor Author

PR #2241 should fix this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression this used to work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants