fix(Canvas): safeguard dispose #7775

ShaMan123 · 2022-03-08T08:11:55Z

github-actions · 2022-03-08T08:13:55Z

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.58 |   86.32 |   82.92 |                                               
 fabric.js |    83.2 |    76.58 |   86.32 |   82.92 | ...,29862,29987,30067-30132,30255,30354,30571 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-03-08T09:14: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.15 |    76.55 |   86.32 |   82.87 |                                               
 fabric.js |   83.15 |    76.55 |   86.32 |   82.87 | ...,29862,29987,30067-30132,30255,30354,30571 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-03-08T10:44:37Z

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.15 |    76.55 |   86.32 |   82.87 |                                               
 fabric.js |   83.15 |    76.55 |   86.32 |   82.87 | ...,29862,29987,30067-30132,30255,30354,30571 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-03-08T11:25: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.2 |    76.58 |   86.32 |   82.92 |                                               
 fabric.js |    83.2 |    76.58 |   86.32 |   82.92 | ...,29862,29987,30067-30132,30255,30354,30571 
-----------|---------|----------|---------|---------|-----------------------------------------------

This reverts commit 377a7d5.

github-actions · 2022-03-08T11:30:18Z

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.19 |    76.55 |   86.32 |   82.91 |                                               
 fabric.js |   83.19 |    76.55 |   86.32 |   82.91 | ...,29862,29987,30067-30132,30255,30354,30571 
-----------|---------|----------|---------|---------|-----------------------------------------------

stale · 2022-03-27T20:23:38Z

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.

asturur

Can you check which of the 2 changes it the real fix?

github-actions · 2022-03-29T05:50:14Z

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.15 |    76.54 |   86.23 |   82.87 |                                               
 fabric.js |   83.15 |    76.54 |   86.23 |   82.87 | ...,29909,30034,30114-30179,30302,30401,30637 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-03-29T06:36:31Z

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.14 |     76.5 |   86.23 |   82.86 |                                               
 fabric.js |   83.14 |     76.5 |   86.23 |   82.86 | ...,29909,30034,30114-30179,30302,30401,30637 
-----------|---------|----------|---------|---------|-----------------------------------------------

ShaMan123 · 2022-03-29T06:38:42Z

Can you check which of the 2 changes it the real fix?

Both are the real fix.
Added another test for setting dimensions and disposing.
Made me encounter a race condition between dispose and render.
If you look at the test you can see I had to wrap the disposing in an event to wait for after rendering.
I think this should be standard behavior handled internally by canvas, returning a promise once disposing has completed.
Thoughts?
But as you often suggest, let's limit the scope of this PR.
This on my end it's ready.

This reverts commit f1ca115.

github-actions · 2022-03-30T03:36:12Z

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.14 |     76.5 |   86.23 |   82.86 |                                               
 fabric.js |   83.14 |     76.5 |   86.23 |   82.86 | ...,29909,30034,30114-30179,30302,30401,30637 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-03-30T08:58: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.15 |    76.54 |   86.23 |   82.87 |                                               
 fabric.js |   83.15 |    76.54 |   86.23 |   82.87 | ...,29909,30034,30114-30179,30302,30401,30637 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-03-30T18:23: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  |   83.18 |    76.55 |   86.29 |    82.9 |                                               
 fabric.js |   83.18 |    76.55 |   86.29 |    82.9 | ...,29913,30038,30118-30183,30306,30405,30641 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-03-30T18:23: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.29 |   82.85 |                                               
 fabric.js |   83.13 |    76.52 |   86.29 |   82.85 | ...,29913,30038,30118-30183,30306,30405,30641 
-----------|---------|----------|---------|---------|-----------------------------------------------

ShaMan123 · 2022-03-30T18:24:16Z

I'll explain.
Using cssText solves the problem of trying to set read only props (and is better for perf I think)
Only an interactive canvas changes lowerCanvasEl style so that's why I wrapped the style restoration as well

asturur · 2022-04-01T08:59:53Z

weird i would imagine this
https://github.com/fabricjs/fabric.js/pull/7775/files#diff-fc54cc24e3c94d9201b66567331262bc363a509f49283a00e715aa3bf08dce35R1559-R1562
would take care of interaction between disposing and rendering

ShaMan123 · 2022-04-01T22:19:29Z

weird i would imagine this https://github.com/fabricjs/fabric.js/pull/7775/files#diff-fc54cc24e3c94d9201b66567331262bc363a509f49283a00e715aa3bf08dce35R1559-R1562 would take care of interaction between disposing and rendering

The link doesn't open properly

ShaMan123 added 2 commits March 8, 2022 09:45

Update static_canvas.class.js

415b556

safeguard from style object

f6139c5

ShaMan123 mentioned this pull request Mar 8, 2022

Add context check for __initRetinaScaling #7706

Closed

tests

208b666

Update canvas.js

194b029

ShaMan123 requested a review from asturur March 8, 2022 11:24

ShaMan123 added 2 commits March 8, 2022 13:26

Update canvas.js

377a7d5

Revert "Update canvas.js"

7314cfa

This reverts commit 377a7d5.

ShaMan123 requested a review from melchiar March 9, 2022 07:27

ShaMan123 mentioned this pull request Mar 18, 2022

IMPORTANT: Migrating to modern javascript/typescript #7596

Closed

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

ShaMan123 added fix-next and removed stale Issue marked as stale by the stale bot labels Mar 28, 2022

asturur self-assigned this Mar 28, 2022

asturur reviewed Mar 28, 2022

View reviewed changes

Merge branch 'master' into fix-canvas-dispose

0ffac34

ShaMan123 added 2 commits March 29, 2022 09:30

Update canvas.class.js

f1ca115

Update canvas.js

32039f7

Revert "Update canvas.class.js"

ea3f8b6

This reverts commit f1ca115.

ShaMan123 added 2 commits March 30, 2022 21:19

Merge branch 'master' into fix-canvas-dispose

333d8c6

Update static_canvas.class.js

f2366cc

asturur approved these changes Apr 1, 2022

View reviewed changes

ShaMan123 merged commit 6b64132 into master Apr 1, 2022

ShaMan123 deleted the fix-canvas-dispose branch April 1, 2022 22:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(Canvas): safeguard dispose #7775

fix(Canvas): safeguard dispose #7775

ShaMan123 commented Mar 8, 2022

github-actions bot commented Mar 8, 2022 •

edited

github-actions bot commented Mar 8, 2022 •

edited

github-actions bot commented Mar 8, 2022 •

edited

github-actions bot commented Mar 8, 2022 •

edited

github-actions bot commented Mar 8, 2022 •

edited

stale bot commented Mar 27, 2022

asturur left a comment

github-actions bot commented Mar 29, 2022 •

edited

github-actions bot commented Mar 29, 2022 •

edited

ShaMan123 commented Mar 29, 2022 •

edited

github-actions bot commented Mar 30, 2022 •

edited

github-actions bot commented Mar 30, 2022 •

edited

github-actions bot commented Mar 30, 2022 •

edited

github-actions bot commented Mar 30, 2022 •

edited

ShaMan123 commented Mar 30, 2022

asturur commented Apr 1, 2022

ShaMan123 commented Apr 1, 2022

fix(Canvas): safeguard dispose #7775

fix(Canvas): safeguard dispose #7775

Conversation

ShaMan123 commented Mar 8, 2022

github-actions bot commented Mar 8, 2022 • edited

Code Coverage Summary

github-actions bot commented Mar 8, 2022 • edited

Code Coverage Summary

github-actions bot commented Mar 8, 2022 • edited

Code Coverage Summary

github-actions bot commented Mar 8, 2022 • edited

Code Coverage Summary

github-actions bot commented Mar 8, 2022 • edited

Code Coverage Summary

stale bot commented Mar 27, 2022

asturur left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 29, 2022 • edited

Code Coverage Summary

github-actions bot commented Mar 29, 2022 • edited

Code Coverage Summary

ShaMan123 commented Mar 29, 2022 • edited

github-actions bot commented Mar 30, 2022 • edited

Code Coverage Summary

github-actions bot commented Mar 30, 2022 • edited

Code Coverage Summary

github-actions bot commented Mar 30, 2022 • edited

Code Coverage Summary

github-actions bot commented Mar 30, 2022 • edited

Code Coverage Summary

ShaMan123 commented Mar 30, 2022

asturur commented Apr 1, 2022

ShaMan123 commented Apr 1, 2022

github-actions bot commented Mar 8, 2022 •

edited

github-actions bot commented Mar 8, 2022 •

edited

github-actions bot commented Mar 8, 2022 •

edited

github-actions bot commented Mar 8, 2022 •

edited

github-actions bot commented Mar 8, 2022 •

edited

github-actions bot commented Mar 29, 2022 •

edited

github-actions bot commented Mar 29, 2022 •

edited

ShaMan123 commented Mar 29, 2022 •

edited

github-actions bot commented Mar 30, 2022 •

edited

github-actions bot commented Mar 30, 2022 •

edited

github-actions bot commented Mar 30, 2022 •

edited

github-actions bot commented Mar 30, 2022 •

edited