processing / p5.js

p5.js is a client-side JS platform that empowers artists, designers, students, and anyone to learn to code and express themselves creatively on the web. It is based on the core principles of Processing. http://twitter.com/p5xjs —
http://p5js.org/
GNU Lesser General Public License v2.1
21.74k stars 3.34k forks source link

mouseButton lags one click behind in canvas.mousePressed #3087

Closed j6k4m8 closed 6 years ago

j6k4m8 commented 6 years ago

Nature of issue?

Most appropriate sub-area of p5.js?

Which platform were you using when you encountered this?

Details about the bug:

It appears that the mouseButton lags one click behind when attaching the mousePressed listener to the canvas rather than the global p5 namespace (as per examples here and explanation here)

Minimal reproduction:

// Create canvas to populate later
let canvas;

function setup() {
  canvas = createCanvas(100, 100);

  // Prevent right-clicks from opening context menu.
  // Note: This is optional; bug persists either way.
  canvas.elt.oncontextmenu = (function() { return false });

  // Attach mousePressed to canvas, not global p5
  canvas.mousePressed(function() {
    console.log(mouseButton);
  })
}

function draw() {
  // Doesn't matter what goes here...could even be empty.
  background(0, 10, 10);
}

At first, I thought this was a race condition but it doesn't appear to be; I think mouseButton might just not be set yet when this function runs. Thoughts?

Spongman commented 6 years ago

can you try this with 0.6.1?

j6k4m8 commented 6 years ago

Confirmed on 0.6.1: https://alpha.editor.p5js.org/j6m8/sketches/rJzZPF5QX

Spongman commented 6 years ago

ok, it looks like it's because the canvas.mousePressed call is just setting the event listener functions on the canvas element, and this is bypassing (or rather just happening before) the mousePressed logic.

j6k4m8 commented 6 years ago

oh interesting. sounds like that's not an easy fix on this end then?

Spongman commented 6 years ago

it may be quite tricky, yes. I'd recommend using the p5 instance/global mousePressed event instead if you can...

Alternatively, If you need to find which button was pressed when using the element event listener, you can inspect the event object that's passed in to your callback, I believe the button info is in there somewhere.

j6k4m8 commented 6 years ago

Unfortunately can't use the global event in this project — need to filter events by not-canvas :(

I suppose this is a temporary fix:

let canvas;
buttons = {
  2: 'right',
  0: 'left'
};

function setup() {
  canvas = createCanvas(100, 100);
  canvas.elt.oncontextmenu = (function() { return false });

  canvas.mousePressed(function(ev) {
    mouseButton = buttons[ev.button];
  })
}

function draw() {
  background(255, 0, 255);
}

...but it's pretty inelegant. Those (dict keys) numbers are just my empirical guesses — I assume LMB/MMB/RMB is 0, 1, 2, respectively?

If you pointed me to the right vicinity in the p5 source, I might be able to take a look and see if I could write a fix.

Spongman commented 6 years ago

those numbers are defined here: https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/buttons, and they show the combination of currently pressed buttons. if you want to know which button was most-recently pressed, use this: https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/button

j6k4m8 commented 6 years ago

Got it. Would it be helpful for me to poke around the p5 codebase and contribute a fix?

j6k4m8 commented 6 years ago

If I'm reading this correctly, then _setMouseButton is called on :583 — before mousePressed is called on :586.

https://github.com/processing/p5.js/blob/432527aa63bc8e61270107d51e92330790d965a0/src/events/mouse.js#L579-L596

But in the p5.Element handler context, mouseButton is never set:

https://github.com/processing/p5.js/blob/a98e3fa1a13add5c40bad5c4c15f603d3ec3e6ea/src/core/p5.Element.js#L234-L237

That is, fxn will be user-based, and probably no user will think to set the mouseButton variable.

is this fix as simple as adding a _setMouseButton call inside this listener? If so, I'll submit a PR :) If not, I'll still submit a PR but might need some help hunting down the source of this bug.

lmccart commented 6 years ago

fixed with https://github.com/processing/p5.js/pull/3102

matt-hoiland commented 5 years ago

This patch doesn't pass event through to the user's fxn when called. The context rebinding in d76c5a33d6bbb3e767a7df67f6afd60e598c903c also doesn't patch that. The MouseEvent never makes it back to the user. It broke my parabola sketch :disappointed: