Skip to content

fix(Canvas): toDataURL and toCanvasElement do not draw controls by default (skipControlsDrawing) #9896

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

Merged
merged 5 commits into from
Jun 3, 2024
Merged

fix(Canvas): toDataURL and toCanvasElement do not draw controls by default (skipControlsDrawing) #9896

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4a2b998
patch-fix
asturur May 30, 2024
2543b4a
update CHANGELOG.md
github-actions[bot] May 30, 2024
2c49663
usea boolean
asturur Jun 3, 2024
4440e1c
Merge branch 'toCanvasElement-controls' of github.com:fabricjs/fabric…
asturur Jun 3, 2024
ce37b43
simple test
asturur Jun 3, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## [next]

- patch-fix [#9896](https://github.com/fabricjs/fabric.js/pull/9896)
- perf(): Rework constructors to avoid the extra perf cost of current setup [#9891](https://github.com/fabricjs/fabric.js/pull/9891)
- perf(): Remove redundant matrix multiplication in multiplayTransformMatrixArray [#9893](https://github.com/fabricjs/fabric.js/pull/9893)
- test(): Convert Animation tests to jest [#9892](https://github.com/fabricjs/fabric.js/pull/9892)
Expand Down
15 changes: 15 additions & 0 deletions src/canvas/SelectableCanvas.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import type {
TOriginY,
TSize,
TSVGReviver,
TToCanvasElementOptions,
} from '../typedefs';
import { degreesToRadians } from '../util/misc/radiansDegreesConversion';
import { getPointer, isTouchEvent } from '../util/dom_event';
Expand Down Expand Up @@ -467,6 +468,20 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
}
}

toCanvasElement(
multiplier = 1,
options = {} as TToCanvasElementOptions
): HTMLCanvasElement {
const { keepControls } = options;
const originalActive = this._activeObject;
Copy link
Contributor

@jiayihu jiayihu May 31, 2024

Choose a reason for hiding this comment

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

This assumes that only the activeObj has active controls, which is true for vanilla fabric but easily not-so in apps. And it feels hacky anyway. The way instead we did was to use the now-removed interactive property when exporting the canvas and skip control rendering if true. For instance skipping canvas.drawControls().

Copy link
Member Author

@asturur asturur May 31, 2024

Choose a reason for hiding this comment

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

but this.interactive does not exist anymore.
The issue is in the renderCanvas method that because render the controls in 2 moments of the function is not easily divisible in the static vs non static part.

I could also just add interactive back, it would be as hacky has this but less code probably

Copy link
Contributor

@jiayihu jiayihu May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think it's hacky as ultimately you need a condition to tell if the rendering is happening because of export and differentiate based on that. It can be called interactive but also exporting/static whatever

Copy link
Member Author

@asturur asturur May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

or could be called protected shouldRenderControls since is being built just for that

Copy link
Member Author

@asturur asturur May 31, 2024

Choose a reason for hiding this comment

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

It is a workaround as the other because the real issue is that the StaticCanvas shouldn't have an empty drawControls method to call because renderCanvas isn't correctly split between the 2 classes. If it was i could just call the part that does not render the controls

We do not need a boolean to render controls if not for this issue here. In that sense are both workarounds

Copy link
Contributor

@jiayihu jiayihu May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We can also say that it should be left to the app deciding what to do. If you're using SelectableCanvas and do not want to render controls, then just discard the activeObj before toCanvasElement().

I think determining when to render controls in general is highly app-specific.

Copy link
Member Author

@asturur asturur Jun 3, 2024

Choose a reason for hiding this comment

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

Yes all good points, but when i have a sort of regression, i patch the issue, i take notes for improvement, but i don't like to do both at once. In this case the boolean will take care of both, but is a new feature introduced for a fix, and that is smells of problems to me, always.

if (!keepControls) {
this._activeObject = undefined;
}
const el = super.toCanvasElement(multiplier, options);
this._activeObject = originalActive;
return el;
}

/**
* @private
* @param {TPointerEvent} e Event object
Expand Down
5 changes: 3 additions & 2 deletions src/canvas/StaticCanvas.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1414,9 +1414,10 @@ export class StaticCanvas<
*/
toCanvasElement(
multiplier = 1,
{ width, height, left, top, filter } = {} as TToCanvasElementOptions
options = {} as TToCanvasElementOptions
): HTMLCanvasElement {
const scaledWidth = (width || this.width) * multiplier,
const { width, height, left, top, filter } = options,
scaledWidth = (width || this.width) * multiplier,
scaledHeight = (height || this.height) * multiplier,
zoom = this.getZoom(),
originalWidth = this.width,
Expand Down
1 change: 1 addition & 0 deletions src/typedefs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ export type TToCanvasElementOptions<
width?: number;
height?: number;
filter?: (object: T) => boolean;
keepControls?: boolean;
};

export type TDataUrlOptions<T extends BaseFabricObject = BaseFabricObject> =
Expand Down