From 4a633825313580d6b399bd4d31f5f33c582b92b0 Mon Sep 17 00:00:00 2001 From: tbolis Date: Mon, 2 Nov 2026 12:52:54 +0200 Subject: [PATCH] fix: the rectangle label listens to the canvas once for all its labels Each label subscribed to object:scaling and object:moving on the canvas when it was drawn, and nothing unsubscribed. The listeners stayed after a label was deleted or the canvas was cleared, so a canvas drawn on for long kept more and more of them. The canvas has the two listeners once, and they look the label up by the target of the event. They are not removed with a label, as undo and redo take its rectangle off the canvas and put it back, and it follows again. --- CHANGELOG.md | 4 ++ .../RectangleLabel/RectangleLabelObject.ts | 30 +++++++-- .../SketchField/tools/tools.spec.ts | 62 +++++++++++++++++++ 3 files changed, 91 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index df4a586..ae5d7c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -92,6 +92,10 @@ The eraser, laser pointer and polygon tools, pinch zoom, and features that forks a text from `addText()`, a pasted object, and one which undo brings back. They were selectable, so a press on an image drew nothing and moved the image instead, with the arrow or any other of these tools. They are selectable again when the select tool takes over. +- The rectangle label tool: the canvas has two listeners for all the labels, not two more for each label + that is drawn. They were never removed, so they stayed after a label was deleted or the canvas was + cleared, and a canvas that is drawn on for long kept more of them. A label still follows its rectangle + when undo and redo take it off the canvas and put it back. ### Packaging and tooling diff --git a/packages/react-sketch/src/components/SketchField/tools/RectangleLabel/RectangleLabelObject.ts b/packages/react-sketch/src/components/SketchField/tools/RectangleLabel/RectangleLabelObject.ts index a1db611..162f1b5 100644 --- a/packages/react-sketch/src/components/SketchField/tools/RectangleLabel/RectangleLabelObject.ts +++ b/packages/react-sketch/src/components/SketchField/tools/RectangleLabel/RectangleLabelObject.ts @@ -1,6 +1,27 @@ import * as fabric from "fabric"; import { CanvasEvent } from "../../events"; +/** The label of each rectangle, so that the listeners of a canvas serve all of its labels */ +const labels = new WeakMap(); + +/** The canvases which already have those listeners */ +const following = new WeakSet(); + +/** + * Make the labels of the canvas follow their rectangles. A label stays with its + * rectangle when it is taken off the canvas and put back (undo, redo), so the + * listeners are not removed with it: there are two for each canvas, not for each label. + */ +const follow = (canvas: fabric.Canvas): void => { + if (following.has(canvas)) return; + following.add(canvas); + const update = ({ target }: CanvasEvent): void => { + if (target) labels.get(target)?.update(); + }; + canvas.on("object:scaling", update); + canvas.on("object:moving", update); +}; + /** * A rectangle with a text box on top of it. The text box follows the * rectangle when it is moved or scaled. @@ -17,12 +38,11 @@ export class RectangleLabelObject { ) { this.rect = new fabric.Rect(rectProps); this.textbox = new fabric.Textbox(text, textProps); - canvas.on("object:scaling", this.update); - canvas.on("object:moving", this.update); + labels.set(this.rect, this); + follow(canvas); } - update = (event: CanvasEvent): void => { - if (event.target !== this.rect) return; + update(): void { this.textbox.set({ width: this.rect.getScaledWidth(), scaleX: 1, @@ -30,7 +50,7 @@ export class RectangleLabelObject { top: (this.rect.top ?? 0) - this.textbox.getScaledHeight(), left: this.rect.left, }); - }; + } setText(text: string): void { this.textbox.set({ text }); diff --git a/packages/react-sketch/src/components/SketchField/tools/tools.spec.ts b/packages/react-sketch/src/components/SketchField/tools/tools.spec.ts index d2d7654..6ed6d32 100644 --- a/packages/react-sketch/src/components/SketchField/tools/tools.spec.ts +++ b/packages/react-sketch/src/components/SketchField/tools/tools.spec.ts @@ -215,6 +215,68 @@ describe("Tools", () => { expect(text.left).toEqual(200); expect(text.top).toEqual(150 - text.getScaledHeight()); }); + + it("listens to the canvas once, whatever the number of labels", () => { + const on = jest.spyOn(canvas, "on"); + const tool = create_tool(canvas, { tool: Tool.RECTANGLE_LABEL }); + [100, 200, 300].forEach((y) => drag(tool, [100, y], [180, y + 60])); + + const listened = (name: string) => + on.mock.calls.filter(([event]) => event === name); + expect(canvas.getObjects()).toHaveLength(6); + expect(listened("object:moving")).toHaveLength(1); + expect(listened("object:scaling")).toHaveLength(1); + }); + + it("moves each label along with its own rectangle", () => { + const tool = create_tool(canvas, { tool: Tool.RECTANGLE_LABEL }); + drag(tool, [100, 100], [180, 160]); + drag(tool, [100, 300], [180, 360]); + + const [, first, second, other] = canvas.getObjects() as fabric.FabricObject[]; + const firstTop = first.top; + second.set({ left: 250, top: 320 }); + fire(canvas, "object:moving", { target: second }); + + expect(other.left).toEqual(250); + expect(other.top).toEqual(320 - other.getScaledHeight()); + expect(first.top).toEqual(firstTop); + }); + + it("moves the label of a canvas which is not the first one", () => { + const another = new fabric.Canvas(document.createElement("canvas")); + try { + drag( + create_tool(canvas, { tool: Tool.RECTANGLE_LABEL }), + [10, 10], + [50, 50] + ); + const tool = create_tool(another, { tool: Tool.RECTANGLE_LABEL }); + drag(tool, [100, 100], [180, 160]); + + const [rect, text] = another.getObjects() as fabric.FabricObject[]; + rect.set({ left: 200, top: 150 }); + fire(another, "object:moving", { target: rect }); + + expect(text.left).toEqual(200); + expect(text.top).toEqual(150 - text.getScaledHeight()); + } finally { + another.dispose(); + } + }); + + it("moves the label of a rectangle which undo and redo took off and put back", () => { + const tool = create_tool(canvas, { tool: Tool.RECTANGLE_LABEL }); + drag(tool, [100, 100], [180, 160]); + + const [rect, text] = canvas.getObjects() as fabric.FabricObject[]; + canvas.remove(rect); + canvas.add(rect); + rect.set({ left: 200, top: 150 }); + fire(canvas, "object:moving", { target: rect }); + + expect(text.left).toEqual(200); + }); }); describe("Pan", () => {