fix(Object): support specyfing toCanvasElement canvas

[jiayihu]

It's fundamentally wrong to assume StaticCanvas is enough for rendering any generic object to a new canvas toCanvasElement. The object typically assumes the canvas reference is the usual canvas, e.g. fabric.Canvas, so it's unsafe to replace it with a StaticCanvas. I could even just do this in the object:

_render() {
  if (canvas.getActiveObject() === this) {
    // Draw some border
  }
}

This is just a quick fix so that one can use a different canvas if needed. It's difficult for fabric to know exactly what canvas class should be used. Something like new this.canvas.constructor(el) would not work.

[codesandbox]

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

[ShaMan123]

Thanks

[ShaMan123]

can we rename the arg?
maybe to createCanvas? I don't know.
I dislike the way this method is impl.
A lot of mess, especially the fact that we are mutating the ref itself instead of cloning and returning a promise.
Maybe that is a better impl.

1. clone
2. send to canvas plane so that tl is positioned at 0,0
3. create canvas
4. render
5. return both canvas and clone

Thoughts?
For now this is a good enough patch

[ShaMan123]

Also the setCoords calls seem to me quite redundant unless the object uses them for the exported image

[ShaMan123]

and if that is the case I would argue the dev should subclass and call setCoords themselves

[asturur]

setCords is there to avoid offScreen check issues.
i THINK it can be removed by specifying for the new canvas that it should not use the offscreen check.
I do not think there is any other reason

But let's do it in a different PR

[asturur]

any reason for this function to not be another option?
Also as pointed out the name of the function could be improved, some suggestions

• canvasCreator
• canvasProvider

[jiayihu]

Good points, done

[asturur]

I understand the point of the pr, but i m still curious about the actual hiccup you found.

I would argue that any subclass should be safe to render on a static canvas, maybe it won't give the exact same pixels but should be at least safe.
The example you pointed out about the getActiveObject during render would be bounced away by me by telling that that part is for the renderControls or drawControls part of an object, not for the render method that should be neutral about the canvas type it is in.

So what is the real issue you found?

[jiayihu]

The use case was implementing the drag preview of an objectfor drag-and-drop, thus using obj.toCanvasElement() as DraggableTextDelegate.ts does: https://github.com/fabricjs/fabric.js/blob/master/src/shapes/IText/DraggableTextDelegate.ts#L142

I would argue that any subclass should be safe to render on a static canvas

True, but that static canvas would be some custom class anyway, not StaticCanvas. I could be using any application-level state for the rendering.

[asturur]

The use case was implementing the drag preview of an objectfor drag-and-drop, thus using obj.toCanvasElement() as DraggableTextDelegate.ts does: https://github.com/fabricjs/fabric.js/blob/master/src/shapes/IText/DraggableTextDelegate.ts#L142

I would argue that any subclass should be safe to render on a static canvas

True, but that static canvas would be some custom class anyway, not StaticCanvas. I could be using any application-level state for the rendering.

And what did you find not working when doing so? What happened?

[jiayihu]

And what did you find not working when doing so? What happened?

The dragged object was using canvas.getActiveObject() to render differently based on the fact it was selected or not. If not selected, it doesn't draw text to improve performance. So the method call throws when the canvas is a StaticCanvas.

I don't think objects should ensure they can be rendered correctly on a StaticCanvas, it's not something you think of when developing an object. The DnD logic should be responsible of correctly generating a drag preview for any existing object, without having the latter adapt to StaticCanvas.

[asturur]

Sorry had full days a work forgot about this

[asturur]

I don't think objects should ensure they can be rendered correctly on a StaticCanvas, it's not something you think of when developing an object. The DnD logic should be responsible of correctly generating a drag preview for any existing object, without having the latter adapt to StaticCanvas.

Independently from this PR that is ok, Objects have to safely render on the staticCanvas, without crashing.
Is a question of separation of concerns that maybe we didn't reach yet or we broke adding new features.
FabricJS remains a rendering library with an interactive layer on top, so if an object has a render method that fails on StaticCanvas ( fail as typeerror /crash the context ) is a bad object.

[asturur]

the reason why this would not work 'new this.canvas.constructor(el)` is because an object doesn't have to have a canvas to be exported, or there is more i don't understand?

[jiayihu]

the reason why this would not work 'new this.canvas.constructor(el)` is because an object doesn't have to have a canvas to be exported, or there is more i don't understand?

It's because a derived Canvas class can take a different set of parameters. It just needs to do super(el) to instantiate the base Canvas class.

[ShaMan123]

Couldn't we have used this.canvas.cloneWithoutData?
nvm

[ShaMan123]

better as is