fix(Canvas): selection + `setViewportTransform` edge case #7713

ShaMan123 · 2022-02-19T23:07:39Z

This PR handle the following edge case:
Say an object is being actively transformed by the user and at some point a call to setViewportTransform changes canvas vpt.
What happens prior to this PR is that the object jumps from the mouse because of the new vpt and then jumps back once the next event fires.
This is bad.
I think that setViewportTransform should not affect an active object that is being transformed.
It makes no sense.

https://gkpiim.csb.app/

Repro

http://jsfiddle.net/4zexwjo6/

Select and hold an object OR transform it OR edit the text.
After a few seconds canvas vpt changes and the object will move to another place
You will experience an unwanted jump in transformation

To fix the fiddle change the cdn.

Before

Expected Behavior

selected object should not be transformed by canvas vpt

After

github-actions · 2022-02-19T23:09:53Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.34 |    76.75 |   86.53 |   83.07 |                                               
 fabric.js |   83.34 |    76.75 |   86.53 |   83.07 | ...,29680,29805,29885-29950,30073,30172,30389 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-02-19T23:13:39Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |    83.3 |    76.71 |   86.53 |   83.03 |                                               
 fabric.js |    83.3 |    76.71 |   86.53 |   83.03 | ...,29689,29814,29894-29959,30082,30181,30398 
-----------|---------|----------|---------|---------|-----------------------------------------------

asturur · 2022-02-19T23:18:35Z

I m not sure what is going on, the explanations are scarce.
The vieportTransform shouldn't transform anything, is just a camera view over the canvas.
What is happening that is out of ordinary?

ShaMan123 · 2022-02-19T23:34:08Z

Take a look at the demo.
I hit an edge case a while back.
If you are transforming an object (say scaling or moving) and at the same moment the canvas vpt changes it affects the active object. Resulting in a jump once the next event is triggered.

ShaMan123 · 2022-02-19T23:42:49Z

~~I am not sure about c1279b4~~ I have reverted it (say the canvas vpt is transformed while an active selection exists but is not actively transforming, the object becomes off screen and then the vpt is reversed and it's returned to screen)
What do you think?
~~Anyways if we decide it's in then we need to merge #7698 first (it calls isShadowOnScreen)~~

github-actions · 2022-02-19T23:49:47Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.28 |    76.65 |   86.53 |      83 |                                               
 fabric.js |   83.28 |    76.65 |   86.53 |      83 | ...,29708,29833,29913-29978,30101,30200,30417 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-02-20T06:50:24Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.36 |    76.75 |   86.54 |   83.09 |                                               
 fabric.js |   83.36 |    76.75 |   86.54 |   83.09 | ...,29705,29830,29910-29975,30098,30197,30414 
-----------|---------|----------|---------|---------|-----------------------------------------------

ShaMan123 · 2022-02-20T09:44:05Z

ready

asturur · 2022-02-20T14:24:45Z

Most of your demo get stuck hours like this.
I'm not sure why.

ShaMan123 · 2022-02-20T15:09:52Z

This is a repro http://jsfiddle.net/exabpkto/

ShaMan123 · 2022-02-20T15:10:30Z

Most of your demo get stuck hours like this. I'm not sure why.

Probably because it's very heavy. fabric dist is copied to there. I plan to change that

This reverts commit c1279b4.

This reverts commit dc335c1.

github-actions · 2022-03-02T08:18:40Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.21 |    76.58 |   86.39 |   82.93 |                                               
 fabric.js |   83.21 |    76.58 |   86.39 |   82.93 | ...,29887,30012,30092-30157,30280,30379,30596 
-----------|---------|----------|---------|---------|-----------------------------------------------

This reverts commit 5e3226b.

github-actions · 2022-03-02T08:21:25Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.31 |    76.72 |   86.53 |   83.04 |                                               
 fabric.js |   83.31 |    76.72 |   86.53 |   83.04 | ...,29701,29826,29906-29971,30094,30193,30410 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-03-02T08:22:36Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |    83.2 |    76.55 |   86.39 |   82.92 |                                               
 fabric.js |    83.2 |    76.55 |   86.39 |   82.92 | ...,29887,30012,30092-30157,30280,30379,30596 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-03-02T08:24:39Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.25 |    76.58 |   86.39 |   82.97 |                                               
 fabric.js |   83.25 |    76.58 |   86.39 |   82.97 | ...,29887,30012,30092-30157,30280,30379,30596 
-----------|---------|----------|---------|---------|-----------------------------------------------

stale · 2022-03-18T00:54:08Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions · 2022-04-02T07:05:49Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |    55.3 |    46.46 |   53.11 |   55.06 |                                               
 fabric.js |    55.3 |    46.46 |   53.11 |   55.06 | ...-30545,30627,30689,30700-30703,30726-30750 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-04-02T07:43:21Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.12 |    76.49 |   86.25 |   82.84 |                                               
 fabric.js |   83.12 |    76.49 |   86.25 |   82.84 | ...,29999,30124,30204-30269,30392,30491,30727 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-04-02T07:50:24Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.18 |    76.55 |   86.25 |   82.91 |                                               
 fabric.js |   83.18 |    76.55 |   86.25 |   82.91 | ...,29999,30124,30204-30269,30392,30491,30727 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-04-02T07:56:43Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.13 |    76.52 |   86.25 |   82.85 |                                               
 fabric.js |   83.13 |    76.52 |   86.25 |   82.85 | ...,29999,30124,30204-30269,30392,30491,30727 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-04-02T08:00:41Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.13 |    76.52 |   86.25 |   82.85 |                                               
 fabric.js |   83.13 |    76.52 |   86.25 |   82.85 | ...,29999,30124,30204-30269,30392,30491,30727 
-----------|---------|----------|---------|---------|-----------------------------------------------

ShaMan123

Fixed the main issue and found and fixed a side effect that was causing tests to fail (stale _currentTransform ref on canvas)

ShaMan123 · 2022-04-02T07:49:11Z

src/canvas.class.js

@@ -1244,6 +1248,7 @@
          return false;
        }
        this._activeObject = null;
+        this._currentTransform = null;


crucial cleanup in case active object is during a transform

ShaMan123 · 2022-04-02T07:50:13Z

src/canvas.class.js

      }
-      fabric.StaticCanvas.prototype.setViewportTransform.call(this, vpt);
+      this._setViewportTransform(vpt);
+      activeObject && activeObject.setCoords();


I think the following is sufficient because StaticCanvas sets coords on all objects

Suggested change

activeObject && activeObject.setCoords();

activeObject && activeObject.type === 'activeSelection' && activeObject.setCoords();

ShaMan123 · 2022-04-02T08:47:14Z

Updated description

github-actions · 2022-05-01T17:06:54Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.04 |    75.93 |   85.71 |   82.76 |                                               
 fabric.js |   83.04 |    75.93 |   85.71 |   82.76 | ...,30874,30948,30959-31024,31147,31246,31482 
-----------|---------|----------|---------|---------|-----------------------------------------------

ShaMan123 requested a review from asturur February 20, 2022 00:29

ShaMan123 added 9 commits March 2, 2022 10:18

Update static_canvas.class.js

2e6ed2e

fix

5e08379

move logic to canvas

856bd4d

finalize

5790a91

discard active object if offscreen

7a96b17

finalize

f97f00c

Revert "discard active object if offscreen"

fcdef73

This reverts commit c1279b4.

Update canvas.class.js

091d841

Revert "Squashed commit of the following:"

5e3226b

This reverts commit dc335c1.

ShaMan123 force-pushed the selection-vpt-change-edge-case branch from dc335c1 to 5e3226b Compare March 2, 2022 08:18

Revert "Revert "Squashed commit of the following:""

874bbb6

This reverts commit 5e3226b.

Update canvas.class.js

ff6124f

stale bot added the stale Issue marked as stale by the stale bot label Mar 18, 2022

ShaMan123 added 8 commits April 2, 2022 09:10

Update canvas.class.js

51560b9

Update canvas.js

e1c88fd

Update canvas.js

28d5b12

Update canvas.js

fd55c7a

account for text editing

1fcfd09

Update canvas.class.js

9c8dee4

Update canvas.js

5c4c6c6

Update canvas.js

7d9b9b4

fix(canvas#discardActiveObject): clear currentTransform

9f558e1

Update static_canvas.class.js

b1cad21

Update canvas.js

c30d82b

Update canvas.js

2acf9ca

Update canvas.js

ecd9ef5

ShaMan123 commented Apr 2, 2022

View reviewed changes

ShaMan123 requested a review from melchiar April 2, 2022 08:07

fabricjs deleted a comment from github-actions bot Apr 2, 2022

This was referenced Apr 26, 2022

custom object drawing #7905

Closed

V6: 🎉 Group Rewrite #7670

Closed

Merge branch 'master' into selection-vpt-change-edge-case

8d163d6

asturur added awaiting evaluation feature and removed fix-next labels Jan 14, 2024

ShaMan123 added the stale Issue marked as stale by the stale bot label May 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(Canvas): selection + `setViewportTransform` edge case #7713

fix(Canvas): selection + `setViewportTransform` edge case #7713

ShaMan123 commented Feb 19, 2022 •

edited

Loading

github-actions bot commented Feb 19, 2022 •

edited

Loading

github-actions bot commented Feb 19, 2022 •

edited

Loading

asturur commented Feb 19, 2022

ShaMan123 commented Feb 19, 2022

ShaMan123 commented Feb 19, 2022 •

edited

Loading

github-actions bot commented Feb 19, 2022 •

edited

Loading

github-actions bot commented Feb 20, 2022 •

edited

Loading

ShaMan123 commented Feb 20, 2022

asturur commented Feb 20, 2022

ShaMan123 commented Feb 20, 2022

ShaMan123 commented Feb 20, 2022 •

edited

Loading

github-actions bot commented Mar 2, 2022 •

edited

Loading

github-actions bot commented Mar 2, 2022 •

edited

Loading

github-actions bot commented Mar 2, 2022 •

edited

Loading

github-actions bot commented Mar 2, 2022 •

edited

Loading

stale bot commented Mar 18, 2022

github-actions bot commented Apr 2, 2022 •

edited

Loading

github-actions bot commented Apr 2, 2022 •

edited

Loading

github-actions bot commented Apr 2, 2022 •

edited

Loading

github-actions bot commented Apr 2, 2022 •

edited

Loading

github-actions bot commented Apr 2, 2022 •

edited

Loading

ShaMan123 left a comment

ShaMan123 Apr 2, 2022

ShaMan123 Apr 2, 2022

ShaMan123 commented Apr 2, 2022

github-actions bot commented May 1, 2022 •

edited

Loading

	activeObject && activeObject.setCoords();
	activeObject && activeObject.type === 'activeSelection' && activeObject.setCoords();

fix(Canvas): selection + setViewportTransform edge case #7713

Are you sure you want to change the base?

fix(Canvas): selection + setViewportTransform edge case #7713

Conversation

ShaMan123 commented Feb 19, 2022 • edited Loading

Repro

Before

Expected Behavior

After

github-actions bot commented Feb 19, 2022 • edited Loading

Code Coverage Summary

github-actions bot commented Feb 19, 2022 • edited Loading

Code Coverage Summary

asturur commented Feb 19, 2022

ShaMan123 commented Feb 19, 2022

ShaMan123 commented Feb 19, 2022 • edited Loading

github-actions bot commented Feb 19, 2022 • edited Loading

Code Coverage Summary

github-actions bot commented Feb 20, 2022 • edited Loading

Code Coverage Summary

ShaMan123 commented Feb 20, 2022

asturur commented Feb 20, 2022

ShaMan123 commented Feb 20, 2022

ShaMan123 commented Feb 20, 2022 • edited Loading

github-actions bot commented Mar 2, 2022 • edited Loading

Code Coverage Summary

github-actions bot commented Mar 2, 2022 • edited Loading

Code Coverage Summary

github-actions bot commented Mar 2, 2022 • edited Loading

Code Coverage Summary

github-actions bot commented Mar 2, 2022 • edited Loading

Code Coverage Summary

stale bot commented Mar 18, 2022

github-actions bot commented Apr 2, 2022 • edited Loading

Code Coverage Summary

github-actions bot commented Apr 2, 2022 • edited Loading

Code Coverage Summary

github-actions bot commented Apr 2, 2022 • edited Loading

Code Coverage Summary

github-actions bot commented Apr 2, 2022 • edited Loading

Code Coverage Summary

github-actions bot commented Apr 2, 2022 • edited Loading

Code Coverage Summary

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 Apr 2, 2022

Choose a reason for hiding this comment

ShaMan123 Apr 2, 2022

Choose a reason for hiding this comment

ShaMan123 commented Apr 2, 2022

github-actions bot commented May 1, 2022 • edited Loading

Code Coverage Summary

fix(Canvas): selection + `setViewportTransform` edge case #7713

fix(Canvas): selection + `setViewportTransform` edge case #7713

ShaMan123 commented Feb 19, 2022 •

edited

Loading

github-actions bot commented Feb 19, 2022 •

edited

Loading

github-actions bot commented Feb 19, 2022 •

edited

Loading

ShaMan123 commented Feb 19, 2022 •

edited

Loading

github-actions bot commented Feb 19, 2022 •

edited

Loading

github-actions bot commented Feb 20, 2022 •

edited

Loading

ShaMan123 commented Feb 20, 2022 •

edited

Loading

github-actions bot commented Mar 2, 2022 •

edited

Loading

github-actions bot commented Mar 2, 2022 •

edited

Loading

github-actions bot commented Mar 2, 2022 •

edited

Loading

github-actions bot commented Mar 2, 2022 •

edited

Loading

github-actions bot commented Apr 2, 2022 •

edited

Loading

github-actions bot commented Apr 2, 2022 •

edited

Loading

github-actions bot commented Apr 2, 2022 •

edited

Loading

github-actions bot commented Apr 2, 2022 •

edited

Loading

github-actions bot commented Apr 2, 2022 •

edited

Loading

github-actions bot commented May 1, 2022 •

edited

Loading