Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Apr 30, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e2eba4c
fix(): mitigate canvas safegurad conflict
ShaMan123 Apr 26, 2022
e660879
Update object.js
ShaMan123 Apr 26, 2022
c0e2e8a
revert(): must set canvas ref
ShaMan123 Apr 26, 2022
4f5af2a
revert
ShaMan123 Apr 26, 2022
044aece
refactor(): set object directly on export
ShaMan123 Apr 26, 2022
5238fd2
remove redundant `setCoords`
ShaMan123 Apr 26, 2022
a2294f9
restore `setCoords`
ShaMan123 Apr 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions src/shapes/object.class.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1729,16 +1729,18 @@
canvas.backgroundColor = '#fff';
}
this.setPositionByOrigin(new fabric.Point(canvas.width / 2, canvas.height / 2), 'center', 'center');

var originalCanvas = this.canvas;
canvas.add(this);
canvas._objects = [this];
this.set('canvas', canvas);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@asturur asturur Apr 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ShaMan123 ShaMan123 Apr 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

de494ae6278212520cae6374e5a8eecaa8d08248ab53f6f30c167949186292caR1726

skipOffscreen: false, so it should not happen

this.setCoords();
var canvasEl = canvas.toCanvasElement(multiplier || 1, options);
this.shadow = originalShadow;
this.set('canvas', originalCanvas);
this.shadow = originalShadow;
if (originalGroup) {
this.group = originalGroup;
}
this.set(origParams).setCoords();
ShaMan123 marked this conversation as resolved.
Show resolved Hide resolved
this.set(origParams);
this.setCoords();
// canvas.dispose will call image.dispose that will nullify the elements
// since this canvas is a simple element for the process, we remove references
// to objects in this way in order to avoid object trashing.
Expand Down
3 changes: 2 additions & 1 deletion test/unit/object.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,14 +396,15 @@

QUnit.test('toCanvasElement', function(assert) {
var cObj = new fabric.Rect({
width: 100, height: 100, fill: 'red', strokeWidth: 0
width: 100, height: 100, fill: 'red', strokeWidth: 0, canvas: canvas
});

assert.ok(typeof cObj.toCanvasElement === 'function');

var canvasEl = cObj.toCanvasElement();

assert.ok(typeof canvasEl.getContext === 'function', 'the element returned is a canvas');
assert.ok(cObj.canvas === canvas, 'canvas ref should remain unchanged');
});

QUnit.test('toCanvasElement activeSelection', function(assert) {
Expand Down