Fabric.js: object:modified does not fire when an object is finished moving
Version
1.7.0
Test Case
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
All 27 comments
It would also be ideal if object:modified was fired after a scale operation is complete.
dennisrjohn
on 1 Dec 2016
it actually should do.
do you have a fiddle where it fails?
asturur
on 1 Dec 2016
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.
dennisrjohn
on 1 Dec 2016
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
on 1 Dec 2016
this gif has been created with page freshly loaded. first try.
asturur
on 1 Dec 2016
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.
dennisrjohn
on 1 Dec 2016
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?
asturur
on 1 Dec 2016
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.
dennisrjohn
on 1 Dec 2016
that is weird.
What could it be? chrome version with problems? macos version?
asturur
on 2 Dec 2016
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.
dennisrjohn
on 2 Dec 2016
gonna do some diff work.
I have el Capitan.
asturur
on 2 Dec 2016
Just as another data point, I am likewise able to reproduce this issue in Mac OS X Sierra. Here pictured in Safari:
ghost
on 3 Dec 2016
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.
asturur
on 3 Dec 2016
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.
dennisrjohn
on 3 Dec 2016
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.
asturur
on 3 Dec 2016
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:
https://github.com/kangax/fabric.js/pull/3510
dennisrjohn
on 8 Dec 2016
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
asturur
on 8 Dec 2016
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.
dennisrjohn
on 8 Dec 2016
Add the stateful property to true in my Canvas declaration solved my problem.
https://github.com/kangax/fabric.js/commit/37b20991c74b4a0d182a4680d8282cbc47d2f9db
ghoullier
on 12 Dec 2016
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.
dennisrjohn
on 12 Dec 2016
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.
asturur
on 12 Dec 2016
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?
dennisrjohn
on 12 Dec 2016
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.
khmyznikov
on 13 Dec 2016
are you on sierra macos?
can i ask you if it happens both with mouse and trackpad?
asturur
on 13 Dec 2016
@asturur Yes sierra. Both.
khmyznikov
on 13 Dec 2016
i have sierra and i can replicate now.
asturur
on 14 Dec 2016
Ok the problem is fixed now.
Effectively the root cause is that of too much firing.
asturur
on 17 Dec 2016
Related issues
keanass
·
5Comments
semiadam
·
3Comments
lyzs90
·
3Comments
medialwerk
·
5Comments
vleandersson
·
4Comments