Fix mouse up target while performing drag operation

[kartikanand]

Hi @asturur

I'm trying to fix the bug describer here in #6117. Based on my limited understanding of the code, I feel the bug is triggered because a transform event of "Drag" is fired. The problem here is that the node's x and y positions are locked. If they were unlocked then surely the mouse up target will be the same as mouse down. But here since the positions are locked, the mouse up target should be wherever the mouse currently is. That I found was coming out as incorrect because of the drag transform being fired.

Can you review this draft and provide your suggestions ?

Thanks,
Kartik

[asturur]

My suggestion is that:

• i think the mouseup returning the same target on drag was an optimization to not research the target.
• we should remove the optiomization

Let me find it.

[asturur]

    /**
     * Cache common information needed during event processing
     * @private
     * @param {Event} e Event object fired on event
     */
    _cacheTransformEventData: function(e) {
      // reset in order to avoid stale caching
      this._resetTransformEventData();
      this._pointer = this.getPointer(e, true);
      this._absolutePointer = this.restorePointerVpt(this._pointer);
      this._target = this._currentTransform ? this._currentTransform.target : this.findTarget(e) || null;
    },

If there is a currentTransform, always return the target of the current transform.

[asturur]

We cannot actually skip executing _setupCurrentTransform or the event system could break.
The bug has been noticed for mouse up, is eventually valid for mouse move too during a drag.
I would suggest for now we just make an extra find target for mouseUp and we return an additional currentTarget together with target.

look for this function in that same file

    /**
     * @private
     * Handle event firing for target and subtargets
     * @param {Event} e event from mouse
     * @param {String} eventType event to fire (up, down or move)
     * @param {fabric.Object} targetObj receiving event
     * @param {Number} [button] button used in the event 1 = left, 2 = middle, 3 = right
     * @param {Boolean} isClick for left button only, indicates that the mouse up happened without move.
     */
    _handleEvent: function(e, eventType, button, isClick) {
      var target = this._target,
          targets = this.targets || [],
          options = {
            e: e,
            target: target,
            subTargets: targets,
            button: button || LEFT_CLICK,
            isClick: isClick || false,
            pointer: this._pointer,
            absolutePointer: this._absolutePointer,
            transform: this._currentTransform
          };
+     if (eventType === 'up) {
+       options.currentTarget = this.findTarget(e);
+     }
      this.fire('mouse:' + eventType, options);
      target && target.fire('mouse' + eventType, options);
      for (var i = 0; i < targets.length; i++) {
        targets[i].fire('mouse' + eventType, options);
      }
    },

Something like this should fix this case, and we can remove the optimization if someone finds out a valid use case for mouse move.

This change require a test, and also you need to submit the differences without changing the dist folder.

Before submitting the changes do:
git checkout master dist to get rid of the build process.

To add a test have a look in the file test/unit/canvas_events.js

[asturur]

PLease try to apply the fix in the comment

[kartikanand]

Hi @asturur, Yes you're correct we cannot simply remove the transform. I did some tests with my current fix and it doesn't work for all cases. Let me try the one you suggested and I'll update this pull request with both the change and the test.

Thanks,
Kartik

[asturur]

Seems good to me, as soon as is green we can merge. Thanks!

[kartikanand]

Thank you @asturur for your guidance!