object:modified does not fire when an object is finished moving

[dennisrjohn]

Version

1.7.0

Test Case

http://fabricjs.com/events

Steps to reproduce

Move any object around the canvas.

Expected Behavior

When the move operation is complete, the object modified event will fire.

The docs state that the object modified event fires after an object is moved (from https://github.com/kangax/fabric.js/wiki/Working-with-events):
object:modified — fired after object is modified (moved, scaled, rotated)

We need to know when the move operation is complete so we can save the location of the moved object to the server.

Actual Behavior

object:modified is not fired when the move operation is complete.

[dennisrjohn]

It would also be ideal if object:modified was fired after a scale operation is complete.

[asturur]

it actually should do.
do you have a fiddle where it fails?

[dennisrjohn]

Sure.

https://jsfiddle.net/6vg5d3oz/

Notice when you move or scale the object, the alert doesn't fire, when you rotate the object, it does.

This seems to be a recent issue, as if I reference Fabric 1.5.0 the event fires in both scenarios.

[asturur]

i went to check fiddle. I have to say first scale did not fired.
Then i tried again and fired.
The reset and fire always.
Can you double check?

[asturur]

this gif has been created with page freshly loaded. first try.

[dennisrjohn]

It's very inconsistent. Most of my move and scale actions don't fire the event, unless the object is rotated, then most of the scale events fire, but not the move events

I'm on Chrome on Mac.

[asturur]

at end of transform the event is fired if the old value is different from the new value. I m going to give a check if some code path is not covered.
Is it inconsisten in all browser?

if you hook the event in the execute tab on kitchensink, do you get different results? i m wondering if jsfiddle can interfer in some way with the alert. a console log is failing in same way?

[dennisrjohn]

On my Mac I tested Chrome, Firefox and Safari. All produce inconsistent results.

On a co-worker's Mac using Chrome, it works consistently.

Chrome and Firefox on Windows work consistently.

[asturur]

that is weird.
What could it be? chrome version with problems? macos version?

[dennisrjohn]

Well, I've tested it on 4 different Macs.

3 of them have OSX Sierra and produce inconsistent results on all browsers. One has OSX El Capitan and consistently works.

However, I did confirm that if I reference fabric 1.6.7 in the jsfiddle, all browsers consistently work, so it is some change made between 1.6.7 and 1.7.0.

[asturur]

gonna do some diff work.
I have el Capitan.

[ghost]

Just as another data point, I am likewise able to reproduce this issue in Mac OS X Sierra. Here pictured in Safari:

[asturur]

how likely this is a fabric bug? so strange. @dennisrjohn what about removing the alert and trying a simple console log? is that the same? is just on macos sierra for now.

[dennisrjohn]

console.log produces the same results.

I would personally classify this as a fabric bug, because 1.6.7 works fine on Sierra. Did you have any luck with your diffs? If you can point me to the area that where that functionality is located, I have the repository cloned and building so I can investigate it early next week.

Sierra is at 34% adoption rate right now, so this will affect 1 in 3 mac users.

I agree, it's definitely very strange.

[asturur]

did not start yet. i m working on the custom build from website. i ll check also this.

as soon as i start i ll post here the file where to look at.

[dennisrjohn]

I found where the issue is. I'm not sure why it's only happening on sierra, but the mouse move events fire A LOT more often than they should.

in the following method in canvas.class.js (line 650):
_translateObject: function (x, y) {

it determines if an object has moved by comparing it to it's last known X and Y coordinates:
moveX = !target.get('lockMovementX') && target.left !== newLeft,
moveY = !target.get('lockMovementY') && target.top !== newTop;

Unfortunately, on Sierra, multiple mouse events can fire per pixel. So when I move the mouse to location 100, 100, the mouse event may (and often does) fire more than once. The first event will return that the X and Y have changed, the subsequent events will not.

To fix this, I changed moveX to compare against transform.offsetX and offsetY (which should remain as the initial value before any transforms have been applied)

moveX = !target.get('lockMovementX') && transform.offsetX !== newLeft,
moveY = !target.get('lockMovementY') && transform.offsetY !== newTop;

There is a similar issue with scaling from the top and left handles. Again, it needs to compare the top and left against the original values instead of the last value. I fixed that by adding the following to scaleObject in canvas.class.js line 788:

scaled = t.top != target.top || t.left != target.left;
scaled = this._setObjectScale(localMouse, t, lockScalingX, lockScalingY, by, lockScalingFlip, dim) || scaled;

I created a pull request with these fixes:
#3510

[asturur]

ok the debug information is good. this problem of too much firing may affect performance issues.
i guess we should compare with original object from the initial transform but we should not mix left and top during scale operation. i ll review better the changes, thanks for looking into this

[dennisrjohn]

Out of curiosity this morning I put this fiddle together:
https://jsfiddle.net/doa0facu/

It simply outputs the x and y coordinates on move. I am only seeing a single mouse move event per pixel using this simple example.

Maybe there are multiple mouse move events being attached in fabric and only one is being fired on operating systems other than Sierra? I did see some code in fabric dealing with retina screens, maybe something to do with that?

The submitted fix will still work, but it's not addressing the root cause.

[ghoullier]

Add the stateful property to true in my Canvas declaration solved my problem.

37b2099

[dennisrjohn]

That definitely looks like the commit that caused the issue. Unfortunately, I can't change stateful to true because I need to be able to draw outside the bounds of a fabric group.

[asturur]

statefull true and group bounds should be unrelated. stateful means that at every transformation every property get checked for a change. if it something changed an event modified is fired. we are able to detect the modify on scale move etc based on other props. mac os sierra is an exception.

[dennisrjohn]

Sorry, you are correct, I had confused stateful with Object Caching.

My understanding is that stateful can affect performance when you have a lot of items on a canvas. We have canvases with sometimes 100 images and textboxes. I guess if we see performance issues, we can disable stateful when an object is selected and re-enable it when it's deselected. Would that work? Or is stateful not able to be turned on and off at will?

[khmyznikov]

I have this issue with "scale" action. One "scale" action from five does not firing "modified" event, transform.actionPerformed is false.

I hacked this by manual set canvas.stateful = true on 'scaling' event and turn it into false on 'modified' event.

[asturur]

are you on sierra macos?
can i ask you if it happens both with mouse and trackpad?

[khmyznikov]

@asturur Yes sierra. Both.

[asturur]

i have sierra and i can replicate now.

[asturur]

Ok the problem is fixed now.
Effectively the root cause is that of too much firing.