Differently support multi mouse events #5785

asturur · 2019-06-29T15:55:52Z

This PR is a proof of concepts for handling multi touch / multi pointer events
It aims at taking track of all events that gets generated on the canvas and identify the owner of the transformation, while letting everything else flow to eventually brushes and events firing.

Removes old ie < 10 code to handle events ( and so not supported )
Register specific events for touch based
Put a guard against double re-registering of mouse down

asturur · 2019-06-29T15:57:31Z

src/mixins/canvas_events.mixin.js

@@ -16,7 +16,7 @@
      addEventOptions = { passive: false };

  function checkClick(e, value) {
-    return 'which' in e ? e.which === value : e.button === value - 1;
+    return e.button === value - 1;


this was something pre ie10. We do not need it anymore

asturur · 2019-06-29T15:57:52Z

src/mixins/canvas_events.mixin.js

+     */
+    _getEventPrefix: function () {
+      return this.enablePointerEvents ? 'pointer' : 'mouse';
+    },


this is just to allow the minifier to work better and reuse code.

asturur · 2019-06-29T15:58:47Z

src/mixins/canvas_events.mixin.js

+
+      var changedTouches = event.changedTouches;
+      if (changedTouches) {
+        return changedTouches[0] && changedTouches[0].identifier;


need to verify what changeTouches contains and in what order.

asturur · 2019-06-29T16:04:49Z

@dirkluijk @ arcatdmz
I'm trying to get a grasp of the pointer event situation.

Today i digged up my pointer event device and formatted/reinstalled it ( forgot password ) so that i can move on with this.

We need to understand how we can:
Uniform pointer/touch/mouse events in a way that up/down make sense in the same way for every device ( a mouse generate down/up with buttons and PER button , while pointer do not do it, but they generate a move event with different button state )

Define if we want hardcode in fabric that object transformations are done with left mouse button and with no buttons for touch/pointer events ( ok fingers do not have button ).

How touch events and pointer events fire things with multiple finger/pencils that move or goes up ath the same time.

if i have 2 fingers down on the screen and i pull up one, do i get a mouse up events? I did not try yet.

asturur · 2019-06-29T16:07:53Z

src/mixins/canvas_events.mixin.js

+      if (!this._isMainEvent(e)) {
+        return;
+      }
+


this repeated for down/up/move is needed to avoid doing stuff in fabricjs object transformations when we are outside the single pointer movement, main touch changes.

asturur · 2019-06-30T12:51:14Z

For now is broken, but i m here with my multitouch stuff to test it

asturur · 2019-06-30T15:04:10Z

src/mixins/canvas_events.mixin.js

@@ -60,8 +85,7 @@
      functor(canvasElement, 'wheel', this._onMouseWheel);
      functor(canvasElement, 'contextmenu', this._onContextMenu);
      functor(canvasElement, 'dblclick', this._onDoubleClick);
-      functor(canvasElement, 'touchstart', this._onMouseDown, addEventOptions);
-      functor(canvasElement, 'touchmove', this._onMouseMove, addEventOptions);
+      functor(canvasElement, 'touchstart', this._onTouchStart, addEventOptions);


do not register touchMove by default. is not used if not after a touchStart. Use specifct onTouchStart instead of _onMouseDown.

asturur · 2019-06-30T15:06:09Z

src/mixins/canvas_events.mixin.js

+      this._willAddMouseDown = setTimeout(function() {
+        addListener(_this.upperCanvasEl, eventTypePrefix + 'down', _this._onMouseDown);
+        _this._willAddMouseDown = 0;
+      }, 400);


add some clearTimeout logic to avoid registering multiple mousedown when another touchend would happen before 400 ms

asturur · 2019-06-30T15:07:32Z

src/util/dom_event.js

-    return pointer;
-  }
-
-  if (fabric.isTouchSupported) {


remove this. isTouchSupport is randomly broken depending on browsers and security and privacy concerns. If the event has a changedTouches then is a touch event.

asturur · 2019-06-30T15:29:44Z

Ok this starts to work

dirkluijk · 2019-07-01T07:19:38Z

Nice work @asturur ! I will test this PR today and come back at you with feedback.

dirkluijk · 2019-07-01T12:43:24Z

I tested this on a touch screen. The fix does not seem to work.

Steps to reproduce

Start drawing with one finger.
Start drawing with second finger (don't release first finger)
Release either first or second finger.

Expected result
I expect either the second finger to be ignored at all, or a second path to be created which is not connected to the first one.

Actual result

First part of the path (until when the second finger touches) is removed
Remaining part of the path are lines between both fingers

Demo: https://a.uguu.se/ZAbwS2hT3Bs0.mov

asturur · 2019-07-02T05:29:55Z

This is not going to fix brushes.
This allows you to fix the brush in the brush file, without bringing specific pencil logic on the canvas.

You can add to the brush events informations like pointerId, activePointers, and mainPointer andavoid it do stop drawing or drawing too much when there are too many fingers, or when the finger is not the main one.

dirkluijk · 2019-07-02T08:10:37Z

All right, I'll check if this PR is OK for a brush fix. I'll get back at you with feedback.

dirkluijk · 2019-07-02T09:36:22Z

Some comments:

maybe rename activePointers to activePointerIds?
if feels a bit weird to have both activePointers and mainPointerId. For example, the _isMainEvent() method checks them both.

I tried to fix the brush issue in this branch, but I encountered very strange behaviour:

with touch events, I still cannot prevent issues with second pointers. The second path is not drawn, but the touchend of the second pointer is still messing things up.
with pointer events, things get even worse (different behaviour). Everything seems to break.

In pencil_brush.class.js I tried several things, including:

onMouseDown: function(pointer, data) {
    if (this.canvas.activePointerIds.length > 1) {
      return;
    }

  /* ... */
}

onMouseMove: function(pointer, data) {
    if (this.canvas.getPointerId(data.e) !== this.canvas.mainPointerId) {
      return;
    }

  /* ... */
}

onMouseUp: function(data) {
    if (this.canvas.getPointerId(data.e) !== this.canvas.mainPointerId) {
      return;
    }

  /* ... */
}

However, I cannot get it working. I feel a bit more comfortable with my original PR.

Can you take a look?

asturur · 2019-07-02T10:01:44Z

Yes but your original PR makes an hard choice of locking events completely when are from more than one finger. That feels bad to me.
I'll put up the touch device again later today and continue.

asturur · 2019-07-02T10:04:40Z

if feels a bit weird to have both activePointers and mainPointerId. For example, the _isMainEvent() method checks them both.

Can you explain me this a bit more? activePointers give us a glimps of how many touches are there, mainPointerId is just a memo to know what event started the current ongoing event flow.

dirkluijk · 2019-07-02T10:55:04Z

Yes but your original PR makes an hard choice of locking events completely when are from more than one finger. That feels bad to me.

Yeah, I agree with that.

I'll put up the touch device again later today and continue.

Awesome! 🎉

Can you explain me this a bit more? activePointers give us a glimps of how many touches are there, mainPointerId is just a memo to know what event started the current ongoing event flow.

Yeah, my comment was about _isMainEvent():

_isMainEvent: function(evt) {
  return this.activePointers.length <= 1 || this.getPointerId(evt) === this.mainPointerId;
},

I don't understand this function really. What does it really do? Shouldn't it only check this.activePointers.length <= 1? Because is there a difference when I release the "main" pointer before the "second" pointer or vice versa?

asturur · 2019-07-03T11:21:21Z

well i wrote it so it would work most of the time.
I think we can call an experimental phase, we do not know if moving away the main finger should interrupt fabricJS transformation or not.

dirkluijk · 2019-07-08T08:20:00Z

Any chance to take a look at my issue? 😎

asturur · 2019-07-08T11:30:00Z

To be totally honest i didn't. I have been taken by some videogame ( wow, new patch, new farming ).

asturur · 2019-07-13T11:05:58Z

here i am, wired with the touch device, resuming work.

asturur · 2019-07-13T16:08:42Z

I was about to give up today.
Then i decided for this compromise: when pointer events are enabled, touch events are disabled.
This allow for simpler code all around.
Ie10 works nice too.

I did not get to the brush problem yet.

asturur · 2019-07-13T19:33:20Z

This is not perfect by any mean, but the brush now works correctly. There is some strange interaction with firefox but ie10, ie11, edge, chrome work good.

asturur · 2019-07-14T16:18:38Z

Ok i m going to merge it. The pencil works good now with many fingers on the screen.
At works is as broken as before!

dirkluijk · 2019-07-14T16:26:43Z

Thanks @asturur! I'll test it tomorrow morning right away.

dirkluijk · 2019-07-15T09:11:49Z

Works perfect!!

asturur · 2019-07-15T13:42:11Z

I have microsoft surface, a stylus , firefox and i could freedraw both with finger and pencil.

can you start a fiddle with latest 3.3.2 where the freedrawing does not work?

bricejar · 2019-07-15T13:45:46Z

@asturur it works, sorry about that.

durga598 · 2019-08-05T07:30:37Z

src/brushes/pencil_brush.class.js

@@ -49,7 +52,10 @@
     * Inovoked on mouse move
     * @param {Object} pointer
     */
-    onMouseMove: function(pointer) {
+    onMouseMove: function(pointer, options) {


Need to add extra options parameter in doc

just save

6491e68

asturur commented Jun 29, 2019

View reviewed changes

asturur mentioned this pull request Jun 29, 2019

fix(canvas): prevent issues with multitouch events #5699

Closed

asturur added 2 commits June 30, 2019 07:13

make event detection dynamic

2c4cda1

seems better!

61b0ba7

asturur commented Jun 30, 2019

View reviewed changes

asturur added 2 commits June 30, 2019 08:27

seems better!

da3de92

seems better!

b6a53c5

asturur added 2 commits June 30, 2019 09:18

fixed test

4e07704

working queue with add and remove

077a207

asturur mentioned this pull request Jul 13, 2019

Free Drawing does not work in Firefox > 66 with the stylus of a Microsoft surface. #5801

Closed

asturur mentioned this pull request Jul 13, 2019

gesture not working in chrome #5721

Closed

ok better

084ff91

asturur added 2 commits July 13, 2019 11:41

Merge branch 'master' into event-mouse-up

0d33645

pencil working

7864a8b

asturur added 3 commits July 13, 2019 12:34

no console.log

6ea500e

fix tests

d0d4ad3

more test fixes

2514624

This was referenced Jul 14, 2019

Freedrawing with stylus broken in Chrome 75/ Windows 10 #5773

Closed

touchevent ：Cannot read property 'x' of undefined #5743

Closed

asturur merged commit 01a7ec6 into master Jul 14, 2019

asturur mentioned this pull request Jul 14, 2019

Version 3.3.0 #5803

Merged

durga598 reviewed Aug 5, 2019

View reviewed changes

thiagocunha pushed a commit to thiagocunha/fabric.js that referenced this pull request Nov 18, 2019

Differently support multi mouse events (fabricjs#5785)

ef98a58

asturur deleted the event-mouse-up branch January 15, 2020 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Differently support multi mouse events #5785

Differently support multi mouse events #5785

asturur commented Jun 29, 2019 •

edited

Loading

asturur Jun 29, 2019

asturur Jun 29, 2019

asturur Jun 29, 2019

asturur commented Jun 29, 2019

asturur Jun 29, 2019

asturur commented Jun 30, 2019

asturur Jun 30, 2019

asturur Jun 30, 2019

asturur Jun 30, 2019

asturur commented Jun 30, 2019

dirkluijk commented Jul 1, 2019

dirkluijk commented Jul 1, 2019 •

edited

Loading

asturur commented Jul 2, 2019

dirkluijk commented Jul 2, 2019 •

edited

Loading

dirkluijk commented Jul 2, 2019 •

edited

Loading

asturur commented Jul 2, 2019

asturur commented Jul 2, 2019

dirkluijk commented Jul 2, 2019 •

edited

Loading

asturur commented Jul 3, 2019

dirkluijk commented Jul 8, 2019

asturur commented Jul 8, 2019

asturur commented Jul 13, 2019

asturur commented Jul 13, 2019

asturur commented Jul 13, 2019

asturur commented Jul 14, 2019

dirkluijk commented Jul 14, 2019

dirkluijk commented Jul 15, 2019

asturur commented Jul 15, 2019 •

edited

Loading

bricejar commented Jul 15, 2019

durga598 Aug 5, 2019

Differently support multi mouse events #5785

Differently support multi mouse events #5785

Conversation

asturur commented Jun 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur commented Jun 29, 2019

Choose a reason for hiding this comment

asturur commented Jun 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur commented Jun 30, 2019

dirkluijk commented Jul 1, 2019

dirkluijk commented Jul 1, 2019 • edited Loading

asturur commented Jul 2, 2019

dirkluijk commented Jul 2, 2019 • edited Loading

dirkluijk commented Jul 2, 2019 • edited Loading

asturur commented Jul 2, 2019

asturur commented Jul 2, 2019

dirkluijk commented Jul 2, 2019 • edited Loading

asturur commented Jul 3, 2019

dirkluijk commented Jul 8, 2019

asturur commented Jul 8, 2019

asturur commented Jul 13, 2019

asturur commented Jul 13, 2019

asturur commented Jul 13, 2019

asturur commented Jul 14, 2019

dirkluijk commented Jul 14, 2019

dirkluijk commented Jul 15, 2019

asturur commented Jul 15, 2019 • edited Loading

bricejar commented Jul 15, 2019

Choose a reason for hiding this comment

asturur commented Jun 29, 2019 •

edited

Loading

dirkluijk commented Jul 1, 2019 •

edited

Loading

dirkluijk commented Jul 2, 2019 •

edited

Loading

dirkluijk commented Jul 2, 2019 •

edited

Loading

dirkluijk commented Jul 2, 2019 •

edited

Loading

asturur commented Jul 15, 2019 •

edited

Loading