fix(canvas export): regression caused by safegurading #7907

ShaMan123 · 2022-04-26T09:04:54Z

#7866 exposed a regression when using toCanvasElement
This commit fixes it by working around canvas.add and fixes another issue:
object.toCanvasElement() would fire an added event which is absolutely undesired

#7866 exposed a regression when using `toCanvasElement` This commit fixes it and fixes another issue: `object.toCanvasElement()` would fire an added event which is absolutely undesired

github-actions · 2022-04-26T09:10:07Z

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  |   82.92 |    75.36 |   86.25 |   82.63 |                                               
 fabric.js |   82.92 |    75.36 |   86.25 |   82.63 | ...,30453,30527,30538-30603,30726,30825,31061 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-04-26T09:20:46Z

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  |   82.92 |    75.36 |   86.25 |   82.63 |                                               
 fabric.js |   82.92 |    75.36 |   86.25 |   82.63 | ...,30453,30527,30538-30603,30726,30825,31061 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-04-26T09:37:26Z

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  |   82.97 |    75.39 |   86.25 |   82.68 |                                               
 fabric.js |   82.97 |    75.39 |   86.25 |   82.68 | ...,30456,30530,30541-30606,30729,30828,31064 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-04-26T09:49:34Z

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  |   82.92 |    75.36 |   86.25 |   82.63 |                                               
 fabric.js |   82.92 |    75.36 |   86.25 |   82.63 | ...,30456,30530,30541-30606,30729,30828,31064 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-04-26T10:10:28Z

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  |   82.92 |    75.36 |   86.25 |   82.63 |                                               
 fabric.js |   82.92 |    75.36 |   86.25 |   82.63 | ...,30456,30530,30541-30606,30729,30828,31064 
-----------|---------|----------|---------|---------|-----------------------------------------------

asturur · 2022-04-26T13:36:12Z

src/shapes/object.class.js

-      canvas.add(this);
-      var canvasEl = canvas.toCanvasElement(multiplier || 1, options);
-      this.shadow = originalShadow;
+      this.set('canvas', canvas);


switching from add to set('canvas') don't we miss a setCoords on the object that could be necessary for proper rendering?

i would add a setCoords here, just to be sure we change with something is equivalent

Also, what about we do:
canvas._objects = [this] since this is a trash-away canvas and we avoid adding an extra options to toDataUrl?

When adding this option here:
https://github.com/fabricjs/fabric.js/pull/7907/files#diff-6748525f6b3662ac6540af0ceab4cbcd5e5d44069b54ec0b1c3d9edab1e894ecR64

Is unclear that passing the objects in, those have to have a canvas reference properly set and setCoords done.
Is just an open door for misunderstandings. let's solve it inside the object.toDataUrl function and leave the rest as it is.

Ok this is good.
Regarding setCoords - why would we need it?
https://github.com/fabricjs/fabric.js/pull/7907/files#diff-de494ae6278212520cae6374e5a8eecaa8d08248ab53f6f30c167949186292caR1726

i think i would needed because if the object in question has offscreen coords, it will eventually not be drawn.
Since it needs to be drawn ( the whole reason of the dataurl export ) and coords can influence a drawing skip, and since we were implicitly setting them using add, i would continue setting them.
The coords are indeed reset at the end of the function. If we don't change them, there would be at least no reason to set them again.

de494ae6278212520cae6374e5a8eecaa8d08248ab53f6f30c167949186292caR1726

skipOffscreen: false, so it should not happen

asturur · 2022-04-26T13:42:26Z

What was the new issue?
I understand there was a past issue that was firing an extra added event.
is the new issue that now we are actually removing an object from the canvas, so going back to the original one is not working anymore?

ShaMan123 · 2022-04-26T14:13:44Z

The new issue was that the safeguarding logic removed the canvas ref from the StaticCanvas#add method and caused a bug when using with toCanvasElement

asturur · 2022-04-26T14:17:54Z

yes but which is the bug ? I think it is that it would missing in the object array of the original canvas, is it that?

github-actions · 2022-04-26T14:19:22Z

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  |   82.97 |    75.38 |   86.25 |   82.69 |                                               
 fabric.js |   82.97 |    75.38 |   86.25 |   82.69 | ...,30456,30530,30541-30606,30729,30828,31064 
-----------|---------|----------|---------|---------|-----------------------------------------------

ShaMan123 · 2022-04-26T14:20:28Z

It surfaced when I merged #7802 with #7859 I think. The drag image uses toCanvasElement - it was there.
Strange things happened and the selected/exported object disappeared

github-actions · 2022-04-26T14:24: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  |   82.92 |    75.36 |   86.25 |   82.63 |                                               
 fabric.js |   82.92 |    75.36 |   86.25 |   82.63 | ...,30456,30530,30541-30606,30729,30828,31064 
-----------|---------|----------|---------|---------|-----------------------------------------------

ShaMan123

ready

src/shapes/object.class.js

github-actions · 2022-04-29T03:11:04Z

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  |   82.93 |    75.35 |   86.25 |   82.64 |                                               
 fabric.js |   82.93 |    75.35 |   86.25 |   82.64 | ...,30458,30532,30543-30608,30731,30830,31066 
-----------|---------|----------|---------|---------|-----------------------------------------------

ShaMan123 added 2 commits April 26, 2022 11:59

fix(): mitigate canvas safegurad conflict

e2eba4c

#7866 exposed a regression when using `toCanvasElement` This commit fixes it and fixes another issue: `object.toCanvasElement()` would fire an added event which is absolutely undesired

Update object.js

e660879

ShaMan123 requested a review from asturur April 26, 2022 09:06

ShaMan123 mentioned this pull request Apr 26, 2022

fix(V6): nested selection 🔧 fix canvas exporting 🧩 #7893

Closed

3 tasks

revert(): must set canvas ref

c0e2e8a

ShaMan123 changed the title ~~fix(canvas export): safegurad conflict~~ fix(canvas export): regression caused by safegurad conflict Apr 26, 2022

ShaMan123 changed the title ~~fix(canvas export): regression caused by safegurad conflict~~ fix(canvas export): regression caused by safegurading Apr 26, 2022

ShaMan123 added the fix-next label Apr 26, 2022

ShaMan123 mentioned this pull request Apr 26, 2022

V6: 🎉 Group Rewrite #7670

Closed

48 tasks

asturur reviewed Apr 26, 2022

View reviewed changes

ShaMan123 added 2 commits April 26, 2022 17:15

revert

4f5af2a

refactor(): set object directly on export

044aece

remove redundant setCoords

5238fd2

ShaMan123 requested a review from asturur April 26, 2022 14:26

ShaMan123 commented Apr 26, 2022

View reviewed changes

asturur reviewed Apr 28, 2022

View reviewed changes

src/shapes/object.class.js Show resolved Hide resolved

restore setCoords

a2294f9

ShaMan123 requested a review from asturur April 29, 2022 03:09

asturur approved these changes Apr 30, 2022

View reviewed changes

asturur merged commit 06de045 into master Apr 30, 2022

ShaMan123 deleted the fix-canvas-export-safegurad-conflict branch September 11, 2022 23:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(canvas export): regression caused by safegurading #7907

fix(canvas export): regression caused by safegurading #7907

ShaMan123 commented Apr 26, 2022

github-actions bot commented Apr 26, 2022 •

edited

Loading

github-actions bot commented Apr 26, 2022 •

edited

Loading

github-actions bot commented Apr 26, 2022 •

edited

Loading

github-actions bot commented Apr 26, 2022 •

edited

Loading

github-actions bot commented Apr 26, 2022 •

edited

Loading

asturur Apr 26, 2022

asturur Apr 26, 2022

asturur Apr 26, 2022 •

edited

Loading

ShaMan123 Apr 26, 2022

asturur Apr 26, 2022

ShaMan123 Apr 26, 2022 •

edited

Loading

asturur commented Apr 26, 2022

ShaMan123 commented Apr 26, 2022

asturur commented Apr 26, 2022

github-actions bot commented Apr 26, 2022 •

edited

Loading

ShaMan123 commented Apr 26, 2022 •

edited

Loading

github-actions bot commented Apr 26, 2022 •

edited

Loading

ShaMan123 left a comment

github-actions bot commented Apr 29, 2022 •

edited

Loading

fix(canvas export): regression caused by safegurading #7907

fix(canvas export): regression caused by safegurading #7907

Conversation

ShaMan123 commented Apr 26, 2022

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

Code Coverage Summary

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

Code Coverage Summary

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

Code Coverage Summary

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

Code Coverage Summary

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

Code Coverage Summary

asturur Apr 26, 2022

Choose a reason for hiding this comment

asturur Apr 26, 2022

Choose a reason for hiding this comment

asturur Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

ShaMan123 Apr 26, 2022

Choose a reason for hiding this comment

asturur Apr 26, 2022

Choose a reason for hiding this comment

ShaMan123 Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

asturur commented Apr 26, 2022

ShaMan123 commented Apr 26, 2022

asturur commented Apr 26, 2022

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

Code Coverage Summary

ShaMan123 commented Apr 26, 2022 • edited Loading

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

Code Coverage Summary

ShaMan123 left a comment

Choose a reason for hiding this comment

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

Code Coverage Summary

github-actions bot commented Apr 26, 2022 •

edited

Loading

github-actions bot commented Apr 26, 2022 •

edited

Loading

github-actions bot commented Apr 26, 2022 •

edited

Loading

github-actions bot commented Apr 26, 2022 •

edited

Loading

github-actions bot commented Apr 26, 2022 •

edited

Loading

asturur Apr 26, 2022 •

edited

Loading

ShaMan123 Apr 26, 2022 •

edited

Loading

github-actions bot commented Apr 26, 2022 •

edited

Loading

ShaMan123 commented Apr 26, 2022 •

edited

Loading

github-actions bot commented Apr 26, 2022 •

edited

Loading

github-actions bot commented Apr 29, 2022 •

edited

Loading