search

1j01 / jspaint

🎨 Classic MS Paint, ＲＥＶＩＶＥＤ + ✨Extras
https://jspaint.app/about
MIT License
7.27k stars 571 forks source link

Fix quick undo for mouse. #133

Closed Yanrishatum closed 5 years ago

Yanrishatum commented 5 years ago

Fixes L+R undo for mouse inputs. Only input method that does not fully works with quick undo is pen. Based on documentation, pen devices should send mousemove with buttons mask containing 32 when pressing cancel button, which I was unable to reproduce. Maybe it was my driver issues or something else (One by Wacom). It does work properly if I disable Windows Ink mode, but that's due to pen acting as a mouse device. Tested both on Chrome and Firefox, as well as Chrome for Android.

1j01 commented 5 years ago

Thanks! Merged in https://github.com/1j01/jspaint/commit/435271d5d2e7212db2d705d1595c0b60296d3ffd

Looking at this at first I was wary of how there's only added code, and no modification of the existing Quick Undo code, but on second thought, this looks like a reasonable approach, given that for touch the canceling happens when there are multiple pointers, and for other inputs its when there are multiple buttons pressed. And the fact that the handling is in "pointermove" was suspicious to me as well, but it does work to press without moving the mouse - apparently it's part of the spec that pointermove is triggered on button state changes in some cases. So I added some comments in https://github.com/1j01/jspaint/commit/278ee910c30d12ea519344e1f523696b88bbbd7b to give context.

I think these initial judgements were part of why I let this PR sit for so long, sorry!

Anyway, it still merged cleanly after all this time, and still works even after upgrading jQuery two major versions*. Glad to have this finally fixed!

1j01 commented 5 years ago

I don't think it makes sense to treat the eraser button on a pen as a cancel button, the eraser is often accessed by flipping the pen over - if you flip it over, you will have already ended the drawing operation, so the cancel won't work.

So I've simplified this to just check if the set of buttons has changed (other than middle mouse button, for scrolling) for any devices.

https://github.com/1j01/jspaint/blob/432f1bb645d446f707a8056d2b804a8b02c39ed3/src/app.js#L494-L505

I don't have a (working) pen at the moment to test with, but I think this is how it should work.