From 47fb5ccacd2f5e5829543abc3e7f5ce1669ce8e1 Mon Sep 17 00:00:00 2001 From: Shachar <34343793+ShaMan123@users.noreply.github.com> Date: Thu, 12 Jan 2023 02:54:31 +0200 Subject: [PATCH] refactor(Text): canvas text editing manager (#8543) --- CHANGELOG.md | 1 + src/canvas/TextEditingManager.ts | 49 ++++++++++ src/canvas/canvas.class.ts | 9 -- src/canvas/canvas_events.ts | 25 +++++- src/mixins/itext_behavior.mixin.ts | 110 +++-------------------- src/mixins/itext_click_behavior.mixin.ts | 24 ++--- src/shapes/itext.class.ts | 13 ++- src/shapes/text.class.ts | 2 +- test/unit/canvas_events.js | 76 ++++++++++++++++ test/unit/itext.js | 52 ----------- 10 files changed, 183 insertions(+), 178 deletions(-) create mode 100644 src/canvas/TextEditingManager.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 1911f9f050b..e790789c314 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ - fix(): `_initRetinaScaling` initializaing the scaling regardless of settings in Canvas. [#8565](https://github.com/fabricjs/fabric.js/pull/8565) - fix(): regression of canvas migration with pointer and sendPointToPlane [#8563](https://github.com/fabricjs/fabric.js/pull/8563) \- chore(TS): Add declare in front of properties that are type definitions. [#8574](https://github.com/fabricjs/fabric.js/pull/8574) +- refactor(Canvas, IText): Handle cross instance text editing states to an EditingManager class [#8543](https://github.com/fabricjs/fabric.js/pull/8543) - chore(TS): move to export, babel, new rollup, change import statement for fabric. [#8585](https://github.com/fabricjs/fabric.js/pull/8585); - chore(TS): Add declare in front of properties that are type definitions. [#8574](https://github.com/fabricjs/fabric.js/pull/8574) - refactor(Animation): BREAKING: Animation api reduction and semplification (byValue is removed, '+=' syntax is removed, callbacks fired 100%) [#8547](https://github.com/fabricjs/fabric.js/pull/8547) diff --git a/src/canvas/TextEditingManager.ts b/src/canvas/TextEditingManager.ts new file mode 100644 index 00000000000..332c3a700c9 --- /dev/null +++ b/src/canvas/TextEditingManager.ts @@ -0,0 +1,49 @@ +import { TPointerEvent } from '../EventTypeDefs'; +import type { IText } from '../shapes/itext.class'; +import type { Textbox } from '../shapes/textbox.class'; +import { removeFromArray } from '../util/internals'; + +/** + * In charge of synchronizing all interactive text instances of a canvas + */ +export class TextEditingManager { + private targets: (IText | Textbox)[] = []; + private target?: IText | Textbox; + + exitTextEditing() { + this.target = undefined; + this.targets.forEach((target) => { + if (target.isEditing) { + target.exitEditing(); + } + }); + } + + add(target: IText | Textbox) { + this.targets.push(target); + } + + remove(target: IText | Textbox) { + this.unregister(target); + removeFromArray(this.targets, target); + } + + register(target: IText | Textbox) { + this.target = target; + } + + unregister(target: IText | Textbox) { + if (target === this.target) { + this.target = undefined; + } + } + + onMouseMove(e: TPointerEvent) { + this.target?.isEditing && this.target.updateSelectionOnMouseMove(e); + } + + dispose() { + this.targets = []; + this.target = undefined; + } +} diff --git a/src/canvas/canvas.class.ts b/src/canvas/canvas.class.ts index 25489ce7843..606fd77e544 100644 --- a/src/canvas/canvas.class.ts +++ b/src/canvas/canvas.class.ts @@ -519,8 +519,6 @@ export class SelectableCanvas< protected declare _isCurrentlyDrawing: boolean; declare freeDrawingBrush?: BaseBrush; declare _activeObject?: FabricObject; - declare _hasITextHandlers?: boolean; - declare _iTextInstances: (IText | Textbox)[]; /** * Constructor * @param {HTMLCanvasElement | String} el canvas element to initialize instance on @@ -1561,7 +1559,6 @@ export class SelectableCanvas< super.destroy(); wrapperEl.removeChild(upperCanvasEl); wrapperEl.removeChild(lowerCanvasEl); - this._iTextInstances = []; this.contextCache = null; this.contextTop = null; cleanUpJsdomNode(upperCanvasEl); @@ -1581,12 +1578,6 @@ export class SelectableCanvas< // this.discardActiveGroup(); this.discardActiveObject(); this.clearContext(this.contextTop); - if (this._hasITextHandlers) { - // @ts-ignore - this.off('mouse:up', this._mouseUpITextHandler); - this._iTextInstances = []; - this._hasITextHandlers = false; - } super.clear(); } diff --git a/src/canvas/canvas_events.ts b/src/canvas/canvas_events.ts index dc749e10c80..a39ffc333d2 100644 --- a/src/canvas/canvas_events.ts +++ b/src/canvas/canvas_events.ts @@ -22,6 +22,7 @@ import { isInteractiveTextObject, } from '../util/types'; import { SelectableCanvas } from './canvas.class'; +import { TextEditingManager } from './TextEditingManager'; const RIGHT_CLICK = 3, MIDDLE_CLICK = 2, @@ -123,6 +124,8 @@ export class Canvas extends SelectableCanvas { */ declare _previousPointer: Point; + textEditingManager = new TextEditingManager(); + /** * Adds mouse listeners to canvas * @private @@ -133,7 +136,6 @@ export class Canvas extends SelectableCanvas { // this is a workaround to having double listeners. this.removeListeners(); this._bindEvents(); - // @ts-ginore this.addOrRemove(addListener, 'add'); } @@ -1216,6 +1218,7 @@ export class Canvas extends SelectableCanvas { } else { this._transformObject(e); } + this.textEditingManager.onMouseMove(e); this._handleEvent(e, 'move'); this._resetTransformEventData(); } @@ -1631,6 +1634,26 @@ export class Canvas extends SelectableCanvas { this._groupSelector = null; } + exitTextEditing() { + this.textEditingManager.exitTextEditing(); + } + + /** + * @override clear {@link textEditingManager} + */ + clear() { + this.textEditingManager.dispose(); + super.clear(); + } + + /** + * @override clear {@link textEditingManager} + */ + destroy() { + super.destroy(); + this.textEditingManager.dispose(); + } + /** * Clones canvas instance without cloning existing data. * This essentially copies canvas dimensions since loadFromJSON does not affect canvas size. diff --git a/src/mixins/itext_behavior.mixin.ts b/src/mixins/itext_behavior.mixin.ts index 328f188eaba..642178d151d 100644 --- a/src/mixins/itext_behavior.mixin.ts +++ b/src/mixins/itext_behavior.mixin.ts @@ -1,15 +1,12 @@ // @ts-nocheck import { getEnv } from '../env'; -import { ObjectEvents, TEvent } from '../EventTypeDefs'; +import { ObjectEvents, TEvent, TPointerEvent } from '../EventTypeDefs'; import { Point } from '../point.class'; import { Text } from '../shapes/text.class'; -import { TPointerEvent } from '../typedefs'; import { setStyle } from '../util/dom_style'; -import { removeFromArray } from '../util/internals'; import { createCanvasElement } from '../util/misc/dom'; import { transformPoint } from '../util/misc/matrix'; -import type { Canvas } from '../canvas/canvas_events'; import { TextStyleDeclaration } from './text_style.mixin'; // extend this regex to support non english languages @@ -47,7 +44,6 @@ export abstract class ITextBehaviorMixin< }; protected declare _currentCursorOpacity: number; private declare _textBeforeEdit: string; - protected declare __isMousedown: boolean; protected declare __selectionStartOnMouseDown: number; private declare __dragImageDisposer: VoidFunction; private declare __dragStartFired: boolean; @@ -92,11 +88,10 @@ export abstract class ITextBehaviorMixin< * Initializes all the interactive behavior of IText */ initBehavior() { - this.initAddedHandler(); - this.initRemovedHandler(); this.initCursorSelectionHandlers(); this.initDoubleClickSimulation(); - this.mouseMoveHandler = this.mouseMoveHandler.bind(this); + this.updateSelectionOnMouseMove = + this.updateSelectionOnMouseMove.bind(this); this.dragEnterHandler = this.dragEnterHandler.bind(this); this.dragOverHandler = this.dragOverHandler.bind(this); this.dragLeaveHandler = this.dragLeaveHandler.bind(this); @@ -114,62 +109,6 @@ export abstract class ITextBehaviorMixin< this.selected = false; } - /** - * Initializes "added" event handler - */ - initAddedHandler() { - this.on('added', (opt) => { - // make sure we listen to the canvas added event - const canvas = opt.target; - if (canvas) { - if (!canvas._hasITextHandlers) { - canvas._hasITextHandlers = true; - this._initCanvasHandlers(canvas); - } - canvas._iTextInstances = canvas._iTextInstances || []; - canvas._iTextInstances.push(this); - } - }); - } - - initRemovedHandler() { - this.on('removed', (opt) => { - // make sure we listen to the canvas removed event - const canvas = opt.target; - if (canvas) { - canvas._iTextInstances = canvas._iTextInstances || []; - removeFromArray(canvas._iTextInstances, this); - if (canvas._iTextInstances.length === 0) { - canvas._hasITextHandlers = false; - this._removeCanvasHandlers(canvas); - } - } - }); - } - - /** - * register canvas event to manage exiting on other instances - * @private - */ - _initCanvasHandlers(canvas: Canvas) { - canvas._mouseUpITextHandler = function () { - if (canvas._iTextInstances) { - canvas._iTextInstances.forEach((tObj) => { - tObj.__isMousedown = false; - }); - } - }; - canvas.on('mouse:up', canvas._mouseUpITextHandler); - } - - /** - * remove canvas event to manage exiting on other instances - * @private - */ - _removeCanvasHandlers(canvas: Canvas) { - canvas.off('mouse:up', canvas._mouseUpITextHandler); - } - /** * @private */ @@ -193,7 +132,7 @@ export abstract class ITextBehaviorMixin< }, }; - obj.animate('_currentCursorOpacity', targetOpacity, { + obj._animate('_currentCursorOpacity', targetOpacity, { duration: duration, onComplete: function () { if (!tickState.isAborted) { @@ -448,7 +387,7 @@ export abstract class ITextBehaviorMixin< } if (this.canvas) { this.canvas.calcOffset(); - this.exitEditingOnOthers(this.canvas); + this.canvas.textEditingManager.exitTextEditing(); } this.isEditing = true; @@ -464,46 +403,21 @@ export abstract class ITextBehaviorMixin< this._tick(); this.fire('editing:entered'); this._fireSelectionChanged(); - if (!this.canvas) { - return this; - } - this.canvas.fire('text:editing:entered', { target: this }); - this.initMouseMoveHandler(); - this.canvas.requestRenderAll(); - return this; - } - - exitEditingOnOthers(canvas: Canvas) { - if (canvas._iTextInstances) { - canvas._iTextInstances.forEach((obj) => { - obj.selected = false; - if (obj.isEditing) { - obj.exitEditing(); - } - }); + if (this.canvas) { + this.canvas.fire('text:editing:entered', { target: this }); + this.canvas.requestRenderAll(); } } /** - * Initializes "mousemove" event handler - */ - initMouseMoveHandler() { - this.canvas.on('mouse:move', this.mouseMoveHandler); - } - - /** - * @private + * called by {@link canvas#textEditingManager} */ - mouseMoveHandler(options) { - if (!this.__isMousedown || !this.isEditing) { - return; - } - + updateSelectionOnMouseMove(e: TPointerEvent) { // regain focus getEnv().document.activeElement !== this.hiddenTextarea && this.hiddenTextarea.focus(); - const newSelectionStart = this.getSelectionStartFromPointer(options.e), + const newSelectionStart = this.getSelectionStartFromPointer(e), currentStart = this.selectionStart, currentEnd = this.selectionEnd; if ( @@ -524,7 +438,6 @@ export abstract class ITextBehaviorMixin< this.selectionStart !== currentStart || this.selectionEnd !== currentEnd ) { - this.restartCursorIfNeeded(); this._fireSelectionChanged(); this._updateTextarea(); this.renderCursorOrSelection(); @@ -1065,7 +978,6 @@ export abstract class ITextBehaviorMixin< this.fire('editing:exited'); isTextChanged && this.fire('modified'); if (this.canvas) { - this.canvas.off('mouse:move', this.mouseMoveHandler); this.canvas.fire('text:editing:exited', { target: this }); isTextChanged && this.canvas.fire('object:modified', { target: this }); } diff --git a/src/mixins/itext_click_behavior.mixin.ts b/src/mixins/itext_click_behavior.mixin.ts index c23ef4cae5c..0dfe22a4323 100644 --- a/src/mixins/itext_click_behavior.mixin.ts +++ b/src/mixins/itext_click_behavior.mixin.ts @@ -111,7 +111,7 @@ export abstract class ITextClickBehaviorMixin< return; } - this.__isMousedown = true; + this.canvas.textEditingManager.register(this); if (this.selected) { this.inCompositionMode = false; @@ -172,7 +172,17 @@ export abstract class ITextClickBehaviorMixin< * @private */ mouseUpHandler(options: TransformEvent) { - this.__isMousedown = false; + if (this.canvas) { + this.canvas.textEditingManager.unregister(this); + + const activeObject = this.canvas._activeObject; + if (activeObject && activeObject !== this) { + // avoid running this logic when there is an active object + // this because is possible with shift click and fast clicks, + // to rapidly deselect and reselect this object and trigger an enterEdit + return; + } + } if ( !this.editable || (this.group && !this.group.interactive) || @@ -182,16 +192,6 @@ export abstract class ITextClickBehaviorMixin< return; } - if (this.canvas) { - const currentActive = this.canvas._activeObject; - if (currentActive && currentActive !== this) { - // avoid running this logic when there is an active object - // this because is possible with shift click and fast clicks, - // to rapidly deselect and reselect this object and trigger an enterEdit - return; - } - } - if (this.__lastSelected && !this.__corner) { this.selected = false; this.__lastSelected = false; diff --git a/src/shapes/itext.class.ts b/src/shapes/itext.class.ts index 09bb0c2082e..261bdb46e3b 100644 --- a/src/shapes/itext.class.ts +++ b/src/shapes/itext.class.ts @@ -9,6 +9,7 @@ import { } from '../mixins/itext_key_const'; import { classRegistry } from '../util/class_registry'; import { TClassProperties, TFiller } from '../typedefs'; +import { Canvas } from '../canvas/canvas_events'; export type ITextEvents = ObjectEvents & { 'selection:changed': never; @@ -166,10 +167,14 @@ export class IText extends ITextClickBehaviorMixin { _set(key: string, value: any) { if (this.isEditing && this._savedProps && key in this._savedProps) { this._savedProps[key] = value; - } else { - super._set(key, value); + return this; + } + if (key === 'canvas') { + this.canvas instanceof Canvas && + this.canvas.textEditingManager.remove(this); + value instanceof Canvas && value.textEditingManager.add(this); } - return this; + return super._set(key, value); } /** @@ -429,7 +434,7 @@ export class IText extends ITextClickBehaviorMixin { ctx.fillStyle = this.cursorColor || this.getValueOfPropertyAt(lineIndex, charIndex, 'fill'); - ctx.globalAlpha = this.__isMousedown ? 1 : this._currentCursorOpacity; + ctx.globalAlpha = this._currentCursorOpacity; ctx.fillRect( boundaries.left + boundaries.leftOffset - cursorWidth / 2, topOffset + boundaries.top + dy, diff --git a/src/shapes/text.class.ts b/src/shapes/text.class.ts index 8533ef6f81d..2bc8e4a3ea2 100644 --- a/src/shapes/text.class.ts +++ b/src/shapes/text.class.ts @@ -95,7 +95,7 @@ export class Text< * @type Array * @private */ - declare _dimensionAffectingProps: (keyof this)[]; + declare _dimensionAffectingProps: string[]; /** * @private diff --git a/test/unit/canvas_events.js b/test/unit/canvas_events.js index 80ea59c972e..af1f94970e1 100644 --- a/test/unit/canvas_events.js +++ b/test/unit/canvas_events.js @@ -1258,4 +1258,80 @@ assert.equal(canvas.upperCanvasEl.style.cursor, expectedLockSkewingX[corner], `${corner} is ${expectedLockSkewingX[corner]} for lockSkewingX`); }); }); + + QUnit.test('text editing manager', async function (assert) { + const canvas = new fabric.Canvas(); + const manager = canvas.textEditingManager; + assert.ok(manager, 'should exist'); + assert.deepEqual(manager.targets, [], 'should be empty'); + + const a = new fabric.IText('test'); + const b = new fabric.IText('test'); + const e = { clientX: 30, clientY: 40, which: 1, target: canvas.upperCanvasEl }; + + canvas.add(a); + assert.deepEqual(manager.targets, [a]); + canvas.remove(a); + assert.deepEqual(manager.targets, []); + canvas.add(a); + assert.deepEqual(manager.targets, [a]); + canvas.add(b); + assert.deepEqual(manager.targets, [a, b]); + a.enterEditing(); + assert.ok(a.isEditing); + b.enterEditing(); + assert.ok(b.isEditing); + assert.ok(!a.isEditing, 'should have called exit editing'); + manager.register(b); + manager.exitTextEditing(); + assert.ok(!manager.target, 'should unregister b'); + assert.ok(!b.isEditing, 'b should exit editing'); + b.enterEditing(); + b.fire('mousedown', { e, pointer: new fabric.Point(30, 40) }); + assert.ok(manager.target === b, 'should register b'); + const called = []; + a.updateSelectionOnMouseMove = () => called.push(a); + b.updateSelectionOnMouseMove = () => called.push(b); + canvas.__onMouseMove(e); + assert.deepEqual(called, [b], 'manager is called from mouse move'); + manager.unregister(a); + assert.ok(manager.target === b, 'should not unregister b'); + a.fire('mouseup', { e, pointer: new fabric.Point(30, 40) }); + assert.ok(manager.target === b, 'should not unregister b'); + canvas.__onMouseUp(e); + assert.ok(!manager.target, 'should unregister b'); + manager.register(a); + assert.ok(manager.target === a, 'should register a'); + canvas.remove(a); + assert.ok(!manager.target, 'should unregister a'); + manager.dispose(); + assert.ok(!manager.target, 'should have disposed ref'); + assert.deepEqual(manager.targets, [], 'should have disposed refs'); + const g = new fabric.Group([a]); + canvas.add(g); + assert.deepEqual(manager.targets, [a], 'should register an existing nested instance'); + g.add(b); + assert.deepEqual(manager.targets, [a, b], 'should register an added nested instance'); + manager.register(b); + assert.ok(manager.target === b, 'should register b'); + g.remove(b); + assert.ok(!manager.target, 'should unregister b'); + assert.deepEqual(manager.targets, [a], 'should unregister a nested instance upon removal'); + manager.register(a); + assert.ok(manager.target === a, 'should register a'); + canvas.remove(g); + assert.ok(!manager.target, 'should unregister a'); + assert.deepEqual(manager.targets, [], 'should unregister nested instances upon group removal'); + canvas.add(b); + assert.deepEqual(manager.targets, [b], 'should register instance'); + canvas.clear(); + assert.deepEqual(manager.targets, [], 'clear should clear instances'); + canvas.add(b); + assert.deepEqual(manager.targets, [b], 'should register instance'); + manager.register(b); + assert.ok(manager.target === b, 'should register b'); + await canvas.dispose(); + assert.deepEqual(manager.targets, [], 'dispose should clear instances'); + assert.ok(!manager.target, 'should unregister b'); + }); })(); diff --git a/test/unit/itext.js b/test/unit/itext.js index 310c863d788..eb0bba67fa6 100644 --- a/test/unit/itext.js +++ b/test/unit/itext.js @@ -77,30 +77,6 @@ assert.deepEqual(iText.styles, { }); }); - QUnit.test('instances', function(assert) { - var iText = new fabric.IText('test'); - - // Not on a sketchpad; storing it in instances array already would leak it forever. - var instances = canvas._iTextInstances && canvas._iTextInstances; - var lastInstance = instances && instances[instances.length - 1]; - assert.equal(lastInstance, undefined); - - canvas.add(iText); - instances = canvas._iTextInstances && canvas._iTextInstances; - lastInstance = instances && instances[instances.length - 1]; - assert.equal(lastInstance, iText); - - canvas.remove(iText); - instances = canvas._iTextInstances && canvas._iTextInstances; - lastInstance = instances && instances[instances.length - 1]; - assert.equal(lastInstance, undefined); - - // Should survive being added again after removal. - canvas.add(iText); - lastInstance = canvas._iTextInstances && canvas._iTextInstances[canvas._iTextInstances.length - 1]; - assert.equal(lastInstance, iText); - }); - QUnit.test('fromObject', function(assert) { var done = assert.async(); assert.ok(typeof fabric.IText.fromObject === 'function'); @@ -636,34 +612,6 @@ assert.equal(iText.getCurrentCharFontSize(), 40); }); - QUnit.test('object removal from canvas', function(assert) { - canvas.clear(); - canvas._iTextInstances = null; - var text1 = new fabric.IText('Text Will be here'); - var text2 = new fabric.IText('Text Will be here'); - assert.ok(!canvas._iTextInstances, 'canvas has no iText instances'); - assert.ok(!canvas._hasITextHandlers, 'canvas has no handlers'); - - canvas.add(text1); - assert.deepEqual(canvas._iTextInstances, [text1], 'canvas has 1 text instance'); - assert.ok(canvas._hasITextHandlers, 'canvas has handlers'); - assert.equal(canvas._iTextInstances.length, 1, 'just one itext object should be on canvas'); - - canvas.add(text2); - assert.deepEqual(canvas._iTextInstances, [text1, text2], 'canvas has 2 text instance'); - assert.ok(canvas._hasITextHandlers, 'canvas has handlers'); - assert.equal(canvas._iTextInstances.length, 2, 'just two itext object should be on canvas'); - - canvas.remove(text1); - assert.deepEqual(canvas._iTextInstances, [text2], 'canvas has 1 text instance'); - assert.ok(canvas._hasITextHandlers, 'canvas has handlers'); - assert.equal(canvas._iTextInstances.length, 1, 'just two itext object should be on canvas'); - - canvas.remove(text2); - assert.deepEqual(canvas._iTextInstances, [], 'canvas has 0 text instance'); - assert.ok(!canvas._hasITextHandlers, 'canvas has no handlers'); - }); - QUnit.test('getCurrentCharColor', function(assert) { var iText = new fabric.IText('test foo bar-baz\nqux', { styles: {