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.

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(IText): **NO** clear context top during rendering #8560

Merged
merged 20 commits into from Jan 6, 2023
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -2,6 +2,7 @@

## [next]

- fix(IText): **NO** clear context top during rendering #8560
ShaMan123 marked this conversation as resolved.
Show resolved Hide resolved
- fix(): regression of canvas migration with pointer and sendPointToPlane [#8563](https://github.com/fabricjs/fabric.js/pull/8563)
- chore(TS): Use exports from files to build fabricJS, get rid of HEADER.js [#8549](https://github.com/fabricjs/fabric.js/pull/8549)
- chore(): rm `fabric.filterBackend` => `getFilterBackend` [#8487](https://github.com/fabricjs/fabric.js/pull/8487)
Expand Down
14 changes: 1 addition & 13 deletions src/canvas/canvas.class.ts
Expand Up @@ -19,11 +19,10 @@ import {
isActiveSelection,
isCollection,
isFabricObjectCached,
isInteractiveTextObject,
} from '../util/types';
import { invertTransform, transformPoint } from '../util/misc/matrix';
import { isTransparent } from '../util/misc/isTransparent';
import { TMat2D, TOriginX, TOriginY, TSize } from '../typedefs';
import { TOriginX, TOriginY, TSize } from '../typedefs';
import { degreesToRadians } from '../util/misc/radiansDegreesConversion';
import { getPointer, isTouchEvent } from '../util/dom_event';
import type { IText } from '../shapes/itext.class';
Expand Down Expand Up @@ -1631,17 +1630,6 @@ export class SelectableCanvas<
super._setSVGObject(markup, instance, reviver);
instance.set(originalProperties);
}

setViewportTransform(vpt: TMat2D) {
if (
this.renderOnAddRemove &&
isInteractiveTextObject(this._activeObject) &&
this._activeObject.isEditing
) {
this._activeObject.clearContextTop();
ShaMan123 marked this conversation as resolved.
Show resolved Hide resolved
}
super.setViewportTransform(vpt);
}
}

Object.assign(SelectableCanvas.prototype, {
Expand Down
4 changes: 4 additions & 0 deletions src/canvas/canvas_events.ts
Expand Up @@ -342,10 +342,12 @@ export class Canvas extends SelectableCanvas {
source?: FabricObject,
target?: FabricObject
) {
let dirty = false;
const ctx = this.contextTop;
if (source) {
source.clearContextTop(true);
source.renderDragSourceEffect(e);
dirty = true;
}
if (target) {
if (target !== source) {
Expand All @@ -354,8 +356,10 @@ export class Canvas extends SelectableCanvas {
target.clearContextTop(true);
}
target.renderDropTargetEffect(e);
dirty = true;
}
ctx.restore();
dirty && (this.contextTopDirty = true);
}

/**
Expand Down
4 changes: 1 addition & 3 deletions src/mixins/itext_behavior.mixin.ts
Expand Up @@ -258,9 +258,7 @@ export abstract class ITextBehaviorMixin<
this._currentCursorOpacity = 1;

// make sure we clear context even if instance is not editing
if (shouldClear) {
this.clearContextTop();
}
shouldClear && this.clearContextTop();
Copy link
Member

Choose a reason for hiding this comment

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

we should investigate if the minifier does this for us and in case stop doing it.

Copy link
Member Author

Choose a reason for hiding this comment

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

in case it doesn't, u mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

how? everything is unreadable

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 the minifier swap if (condition) { single line } for condition && singleline automatically

Copy link
Member

Choose a reason for hiding this comment

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

not an issue of this pr but i don't think it make sense to be terse manualy now

}

restartCursorIfNeeded() {
Expand Down
2 changes: 1 addition & 1 deletion src/shapes/Object/InteractiveObject.ts
Expand Up @@ -512,7 +512,7 @@ export class InteractiveFabricObject<
* Clears the canvas.contextTop in a specific area that corresponds to the object's bounding box
* that is in the canvas.contextContainer.
* This function is used to clear pieces of contextTop where we render ephemeral effects on top of the object.
* Example: blinking cursror text selection, drag effects.
* Example: blinking cursor text selection, drag effects.
* @todo discuss swapping restoreManually with a renderCallback, but think of async issues
* @param {Boolean} [restoreManually] When true won't restore the context after clear, in order to draw something else.
* @return {CanvasRenderingContext2D|undefined} canvas.contextTop that is either still transformed
Expand Down
37 changes: 12 additions & 25 deletions src/shapes/itext.class.ts
Expand Up @@ -221,7 +221,6 @@ export class IText extends ITextClickBehaviorMixin<ITextEvents> {
*/
initDimensions() {
this.isEditing && this.initDelayedCursor();
this.clearContextTop();
super.initDimensions();
}

Expand Down Expand Up @@ -272,22 +271,13 @@ export class IText extends ITextClickBehaviorMixin<ITextEvents> {
* @param {CanvasRenderingContext2D} ctx Context to render on
*/
render(ctx: CanvasRenderingContext2D) {
this.clearContextTop();
super.render(ctx);
// clear the cursorOffsetCache, so we ensure to calculate once per renderCursor
// the correct position but not at every cursor animation.
this.cursorOffsetCache = {};
this.renderCursorOrSelection();
}

/**
* @private
* @param {CanvasRenderingContext2D} ctx Context to render on
*/
_render(ctx: CanvasRenderingContext2D) {
super._render(ctx);
}
ShaMan123 marked this conversation as resolved.
Show resolved Hide resolved

/**
* Renders cursor or selection (depending on what exists)
* it does on the contextTop. If contextTop is not available, do nothing.
Expand All @@ -306,19 +296,10 @@ export class IText extends ITextClickBehaviorMixin<ITextEvents> {
} else {
this.renderSelection(ctx, boundaries);
}
this.canvas!.contextTopDirty = true;
Copy link
Member

@asturur asturur Jan 3, 2023

Choose a reason for hiding this comment

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

Ok the difference here is that swapping this with the one from our render function here https://github.com/fabricjs/fabric.js/pull/8560/files#diff-876b5f345ecc11628b120c60dfe0b3fde14630cc3940551bcb4ed23f41c580bcL275 we are swapping a partial contextTop clear with a full contextTop clear.

That is fine if it is needed and solves bugs.
Is that the case?

Copy link
Member Author

Choose a reason for hiding this comment

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

indeed
re read my explanation. it is now clearer I hope. redone it

Copy link
Member Author

Choose a reason for hiding this comment

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

The render method calls renderCursorOrSelection that runs only if in editing mode.
If the entire canvas is rendered it is 100% proof since the top context will be cleared by the canvas and then selection will be drawn by the instance that is being edited.
If we mess with the context outside w/o rendering canvas as we do with key/input events then we must clear context top as part of logic and we must take into account any changes to size of instance

Copy link
Member Author

Choose a reason for hiding this comment

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

I will add this to the decription

ctx.restore();
}

/**
* Renders cursor on context Top, outside the animation cycle, on request
* Used for the drag/drop effect.
* If contextTop is not available, do nothing.
*/
renderCursorAt(selectionStart) {
const boundaries = this._getCursorBoundaries(selectionStart, true);
this._renderCursor(this.canvas.contextTop, boundaries, selectionStart);
}

/**
* Returns cursor boundaries (left, top, leftOffset, topOffset)
* left/top are left/top of entire text box
Expand Down Expand Up @@ -406,6 +387,16 @@ export class IText extends ITextClickBehaviorMixin<ITextEvents> {
return boundaries;
}

/**
* Renders cursor on context Top, outside the animation cycle, on request
* Used for the drag/drop effect.
* If contextTop is not available, do nothing.
*/
renderCursorAt(selectionStart: number) {
ShaMan123 marked this conversation as resolved.
Show resolved Hide resolved
const boundaries = this._getCursorBoundaries(selectionStart, true);
this._renderCursor(this.canvas.contextTop, boundaries, selectionStart);
}

/**
* Renders cursor
* @param {Object} boundaries
Expand Down Expand Up @@ -468,11 +459,7 @@ export class IText extends ITextClickBehaviorMixin<ITextEvents> {
* Renders drag start text selection
*/
renderDragSourceEffect() {
if (
this.__isDragging &&
this.__dragStartSelection &&
this.__dragStartSelection
) {
if (this.__isDragging && this.__dragStartSelection) {
this._renderSelection(
this.canvas.contextTop,
this.__dragStartSelection,
Expand Down
1 change: 0 additions & 1 deletion src/shapes/textbox.class.ts
Expand Up @@ -52,7 +52,6 @@ export class Textbox extends IText {
return;
}
this.isEditing && this.initDelayedCursor();
this.clearContextTop();
this._clearCache();
// clear dynamicMinWidth as it will be different after we re-wrap line
this.dynamicMinWidth = 0;
Expand Down
2 changes: 1 addition & 1 deletion test/lib/visualTestLoop.js
Expand Up @@ -188,7 +188,7 @@
}
await fabricCanvas.dispose();
done();
});
}, assert);
});
}
}
Expand Down
28 changes: 27 additions & 1 deletion test/visual/freedraw.js
Expand Up @@ -3,13 +3,14 @@
canvas.freeDrawingBrush = brush;
}
var options = { e: { pointerId: 1 } };
function pointDrawer(points, brush, fireUp = false) {
function pointDrawer(points, brush, fireUp = false, onMove = undefined) {
setBrush(brush.canvas, brush);
brush.onMouseDown(points[0], options);
for (var i = 1; i < points.length; i++) {
points[i].x = parseFloat(points[i].x);
points[i].y = parseFloat(points[i].y);
brush.onMouseMove(points[i], options);
onMove && onMove(points[i], i, points);
}
if (fireUp) {
brush.onMouseUp(options);
Expand Down Expand Up @@ -2255,6 +2256,31 @@ QUnit.module('Free Drawing', hooks => {
}
});

function withText(canvas) {
canvas.add(new fabric.IText('This textbox should NOT\nclear the brush during rendering'));
const brush = new fabric.PencilBrush(canvas);
brush.color = 'red';
brush.width = 25;
pointDrawer(points, brush, false, (point, index, points) => index === points.length - 1 && canvas.renderAll());
}

tests.push({
test: 'textbox should not clear brush',
build: withText,
golden: 'withText.png',
percentage: 0.02,
width: 200,
height: 250,
fabricClass: 'Canvas',
targets: {
top: true,
main: false,
mesh: true,
result: false,
compare: false
}
});

tests.forEach(function (test) {
var options = Object.assign({}, freeDrawingTestDefaults, test.targets);
if (options.top) {
Expand Down
Binary file added test/visual/golden/mesh_withText.png
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added test/visual/golden/textSelectionClearing.png
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added test/visual/golden/textSelectionClearing2.png
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added test/visual/golden/textSelectionClearing3.png
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added test/visual/golden/top_ctx_withText.png
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
96 changes: 96 additions & 0 deletions test/visual/text.js
Expand Up @@ -491,5 +491,101 @@
percentage: 0.092,
});


// sinon could have spied this w/o effort and in one line
class TestTextbox extends fabric.Textbox {
__calledInitDimensions = 0;
initDimensions() {
super.initDimensions();
this.initialized && this.__calledInitDimensions++;
}
}

function selectionClearingEdgeCases(canvas, callback, assert) {
const text = new TestTextbox('lorem ipsum dolor sit Amet consectgetur', {
width: 200,
centeredRotation: true
});
canvas.add(text);
canvas.setActiveObject(text);
text.enterEditing();
text.selectAll();
canvas.renderAll();
text.rotate(90);
text.scale(0.8);
canvas.centerObject(text);
canvas.renderAll();
assert.equal(text.__calledInitDimensions, 0, 'initDimensions was not called');
canvas.getContext().drawImage(canvas.upperCanvasEl, 0, 0);
callback(canvas.lowerCanvasEl);
}

tests.push({
test: 'Text selection clearing edge cases: transform',
code: selectionClearingEdgeCases,
width: 200,
height: 200,
disabled: fabric.getEnv().isLikelyNode,
golden: 'textSelectionClearing.png',
percentage: 0.02,
fabricClass: 'Canvas'
});

function selectionClearingEdgeCases2(canvas, callback, assert) {
const text = new TestTextbox('lorem ipsum dolor sit Amet sit Amet', {
width: 200,
});
canvas.add(text);
canvas.setActiveObject(text);
text.enterEditing();
text.selectAll();
assert.ok(canvas.contextTopDirty, 'flagged as dirty');
canvas.renderAll();
text.width = 150;
text._forceClearCache = true;
canvas.renderAll();
assert.equal(text.__calledInitDimensions, 1, 'initDimensions was called');
canvas.getContext().drawImage(canvas.upperCanvasEl, 0, 0);
callback(canvas.lowerCanvasEl);
}

tests.push({
test: 'Text selection clearing edge cases: changing width, `initDimensions`',
code: selectionClearingEdgeCases2,
width: 200,
height: 200,
disabled: fabric.getEnv().isLikelyNode,
golden: 'textSelectionClearing2.png',
percentage: 0.02,
fabricClass: 'Canvas'
});

function selectionClearingEdgeCases3(canvas, callback, assert) {
const text = new TestTextbox('lorem ipsum dolor sit Amet consectgetur', {
width: 200
});
canvas.add(text);
canvas.setActiveObject(text);
text.enterEditing();
text.selectAll();
canvas.renderAll();
canvas.setViewportTransform([0.8, 0, 0, 1, 0, 0]);
canvas.renderAll();
assert.equal(text.__calledInitDimensions, 0, 'initDimensions was not called');
canvas.getContext().drawImage(canvas.upperCanvasEl, 0, 0);
callback(canvas.lowerCanvasEl);
}

tests.push({
test: 'Text selection clearing edge cases: vpt',
code: selectionClearingEdgeCases3,
width: 200,
height: 200,
disabled: fabric.getEnv().isLikelyNode,
golden: 'textSelectionClearing3.png',
percentage: 0.02,
fabricClass: 'Canvas'
});

tests.forEach(visualTestLoop(QUnit));
})();