From 5368ddef74570abd3889e7db3133038b04b99cc7 Mon Sep 17 00:00:00 2001 From: Aakansha Doshi Date: Wed, 22 Feb 2023 16:28:12 +0530 Subject: [PATCH] fix: improve text wrapping inside rhombus and more fixes (#6265) * fix: improve text wrapping inside rhombus * Add comments * specs * fix: shift resize and multiple element regression for ellipse and rhombus * use container width for scaling font size * fix * fix multiple resize * lint * redraw on submit * redraw only newly pasted elements * no padding when center * fix tests * fix * dont add padding in rhombus when aligning * refactor * fix * move getMaxContainerHeight and getMaxContainerWidth to textElement.ts * Add specs --- src/components/App.tsx | 9 +-- src/element/newElement.ts | 46 +----------- src/element/resizeElements.ts | 26 ++++--- src/element/textElement.test.ts | 81 +++++++++++++++++++-- src/element/textElement.ts | 99 ++++++++++++++++++++------ src/element/textWysiwyg.test.tsx | 12 ++-- src/element/textWysiwyg.tsx | 13 ++-- src/tests/linearElementEditor.test.tsx | 7 +- 8 files changed, 192 insertions(+), 101 deletions(-) diff --git a/src/components/App.tsx b/src/components/App.tsx index effa604b..3c0ac9dc 100644 --- a/src/components/App.tsx +++ b/src/components/App.tsx @@ -1627,6 +1627,7 @@ class App extends React.Component { oldIdToDuplicatedId.set(element.id, newElement.id); return newElement; }); + bindTextToShapeAfterDuplication(newElements, elements, oldIdToDuplicatedId); const nextElements = [ ...this.scene.getElementsIncludingDeleted(), @@ -1640,10 +1641,10 @@ class App extends React.Component { this.scene.replaceAllElements(nextElements); - nextElements.forEach((nextElement) => { - if (isTextElement(nextElement) && isBoundToContainer(nextElement)) { - const container = getContainerElement(nextElement); - redrawTextBoundingBox(nextElement, container); + newElements.forEach((newElement) => { + if (isTextElement(newElement) && isBoundToContainer(newElement)) { + const container = getContainerElement(newElement); + redrawTextBoundingBox(newElement, container); } }); diff --git a/src/element/newElement.ts b/src/element/newElement.ts index c7f33a46..6024765e 100644 --- a/src/element/newElement.ts +++ b/src/element/newElement.ts @@ -22,15 +22,15 @@ import { getElementAbsoluteCoords } from "."; import { adjustXYWithRotation } from "../math"; import { getResizedElementAbsoluteCoords } from "./bounds"; import { - getBoundTextElement, getBoundTextElementOffset, getContainerDims, getContainerElement, measureText, normalizeText, wrapText, + getMaxContainerWidth, } from "./textElement"; -import { BOUND_TEXT_PADDING, VERTICAL_ALIGN } from "../constants"; +import { VERTICAL_ALIGN } from "../constants"; import { isArrowElement } from "./typeChecks"; type ElementConstructorOpts = MarkOptional< @@ -278,48 +278,6 @@ export const refreshTextDimensions = ( return { text, ...dimensions }; }; -export const getMaxContainerWidth = (container: ExcalidrawElement) => { - const width = getContainerDims(container).width; - if (isArrowElement(container)) { - const containerWidth = width - BOUND_TEXT_PADDING * 8 * 2; - if (containerWidth <= 0) { - const boundText = getBoundTextElement(container); - if (boundText) { - return boundText.width; - } - return BOUND_TEXT_PADDING * 8 * 2; - } - return containerWidth; - } else if (container.type === "ellipse") { - // The width of the largest rectangle inscribed inside an ellipse is - // Math.round((ellipse.width / 2) * Math.sqrt(2)) which is derived from - // equation of an ellipse -https://github.com/excalidraw/excalidraw/pull/6172 - return Math.round((width / 2) * Math.sqrt(2)) - BOUND_TEXT_PADDING * 2; - } - return width - BOUND_TEXT_PADDING * 2; -}; - -export const getMaxContainerHeight = (container: ExcalidrawElement) => { - const height = getContainerDims(container).height; - if (isArrowElement(container)) { - const containerHeight = height - BOUND_TEXT_PADDING * 8 * 2; - if (containerHeight <= 0) { - const boundText = getBoundTextElement(container); - if (boundText) { - return boundText.height; - } - return BOUND_TEXT_PADDING * 8 * 2; - } - return height; - } else if (container.type === "ellipse") { - // The height of the largest rectangle inscribed inside an ellipse is - // Math.round((ellipse.height / 2) * Math.sqrt(2)) which is derived from - // equation of an ellipse - https://github.com/excalidraw/excalidraw/pull/6172 - return Math.round((height / 2) * Math.sqrt(2)) - BOUND_TEXT_PADDING * 2; - } - return height - BOUND_TEXT_PADDING * 2; -}; - export const updateTextElement = ( textElement: ExcalidrawTextElement, { diff --git a/src/element/resizeElements.ts b/src/element/resizeElements.ts index 605ab0c2..fe4f8e48 100644 --- a/src/element/resizeElements.ts +++ b/src/element/resizeElements.ts @@ -43,12 +43,12 @@ import { getApproxMinLineWidth, getBoundTextElement, getBoundTextElementId, - getBoundTextElementOffset, getContainerElement, handleBindTextResize, measureText, + getMaxContainerHeight, + getMaxContainerWidth, } from "./textElement"; -import { getMaxContainerWidth } from "./newElement"; export const normalizeAngle = (angle: number): number => { if (angle >= 2 * Math.PI) { @@ -427,12 +427,16 @@ export const resizeSingleElement = ( }; } if (shouldMaintainAspectRatio) { - const boundTextElementPadding = - getBoundTextElementOffset(boundTextElement); + const updatedElement = { + ...element, + width: eleNewWidth, + height: eleNewHeight, + }; + const nextFont = measureFontSizeFromWH( boundTextElement, - eleNewWidth - boundTextElementPadding * 2, - eleNewHeight - boundTextElementPadding * 2, + getMaxContainerWidth(updatedElement), + getMaxContainerHeight(updatedElement), ); if (nextFont === null) { return; @@ -697,11 +701,15 @@ const resizeMultipleElements = ( const boundTextElement = getBoundTextElement(element.latest); if (boundTextElement || isTextElement(element.orig)) { - const optionalPadding = getBoundTextElementOffset(boundTextElement) * 2; + const updatedElement = { + ...element.latest, + width, + height, + }; const textMeasurements = measureFontSizeFromWH( boundTextElement ?? (element.orig as ExcalidrawTextElement), - width - optionalPadding, - height - optionalPadding, + getMaxContainerWidth(updatedElement), + getMaxContainerHeight(updatedElement), ); if (!textMeasurements) { diff --git a/src/element/textElement.test.ts b/src/element/textElement.test.ts index 91974aca..22086095 100644 --- a/src/element/textElement.test.ts +++ b/src/element/textElement.test.ts @@ -3,6 +3,8 @@ import { API } from "../tests/helpers/api"; import { computeContainerHeightForBoundText, getContainerCoords, + getMaxContainerWidth, + getMaxContainerHeight, measureText, wrapText, } from "./textElement"; @@ -202,24 +204,37 @@ describe("Test measureText", () => { describe("Test getContainerCoords", () => { const params = { width: 200, height: 100, x: 10, y: 20 }; + it("should compute coords correctly when ellipse", () => { - const ellipse = API.createElement({ + const element = API.createElement({ type: "ellipse", ...params, }); - expect(getContainerCoords(ellipse)).toEqual({ + expect(getContainerCoords(element)).toEqual({ x: 44.2893218813452455, y: 39.64466094067262, }); }); + it("should compute coords correctly when rectangle", () => { - const rectangle = API.createElement({ + const element = API.createElement({ type: "rectangle", ...params, }); - expect(getContainerCoords(rectangle)).toEqual({ - x: 10, - y: 20, + expect(getContainerCoords(element)).toEqual({ + x: 15, + y: 25, + }); + }); + + it("should compute coords correctly when diamond", () => { + const element = API.createElement({ + type: "diamond", + ...params, + }); + expect(getContainerCoords(element)).toEqual({ + x: 65, + y: 50, }); }); }); @@ -229,6 +244,7 @@ describe("Test measureText", () => { width: 178, height: 194, }; + it("should compute container height correctly for rectangle", () => { const element = API.createElement({ type: "rectangle", @@ -236,6 +252,7 @@ describe("Test measureText", () => { }); expect(computeContainerHeightForBoundText(element, 150)).toEqual(160); }); + it("should compute container height correctly for ellipse", () => { const element = API.createElement({ type: "ellipse", @@ -243,5 +260,57 @@ describe("Test measureText", () => { }); expect(computeContainerHeightForBoundText(element, 150)).toEqual(212); }); + + it("should compute container height correctly for diamond", () => { + const element = API.createElement({ + type: "diamond", + ...params, + }); + expect(computeContainerHeightForBoundText(element, 150)).toEqual(300); + }); + }); + + describe("Test getMaxContainerWidth", () => { + const params = { + width: 178, + height: 194, + }; + + it("should return max width when container is rectangle", () => { + const container = API.createElement({ type: "rectangle", ...params }); + expect(getMaxContainerWidth(container)).toBe(168); + }); + + it("should return max width when container is ellipse", () => { + const container = API.createElement({ type: "ellipse", ...params }); + expect(getMaxContainerWidth(container)).toBe(116); + }); + + it("should return max width when container is diamond", () => { + const container = API.createElement({ type: "diamond", ...params }); + expect(getMaxContainerWidth(container)).toBe(79); + }); + }); + + describe("Test getMaxContainerHeight", () => { + const params = { + width: 178, + height: 194, + }; + + it("should return max height when container is rectangle", () => { + const container = API.createElement({ type: "rectangle", ...params }); + expect(getMaxContainerHeight(container)).toBe(184); + }); + + it("should return max height when container is ellipse", () => { + const container = API.createElement({ type: "ellipse", ...params }); + expect(getMaxContainerHeight(container)).toBe(127); + }); + + it("should return max height when container is diamond", () => { + const container = API.createElement({ type: "diamond", ...params }); + expect(getMaxContainerHeight(container)).toBe(87); + }); }); }); diff --git a/src/element/textElement.ts b/src/element/textElement.ts index ed4d2762..b91e9f2f 100644 --- a/src/element/textElement.ts +++ b/src/element/textElement.ts @@ -12,7 +12,6 @@ import { BOUND_TEXT_PADDING, TEXT_ALIGN, VERTICAL_ALIGN } from "../constants"; import { MaybeTransformHandleType } from "./transformHandles"; import Scene from "../scene/Scene"; import { isTextElement } from "."; -import { getMaxContainerHeight, getMaxContainerWidth } from "./newElement"; import { isBoundToContainer, isImageElement, @@ -244,31 +243,25 @@ const computeBoundTextPosition = ( const containerCoords = getContainerCoords(container); const maxContainerHeight = getMaxContainerHeight(container); const maxContainerWidth = getMaxContainerWidth(container); - const padding = container.type === "ellipse" ? 0 : BOUND_TEXT_PADDING; let x; let y; if (boundTextElement.verticalAlign === VERTICAL_ALIGN.TOP) { - y = containerCoords.y + padding; + y = containerCoords.y; } else if (boundTextElement.verticalAlign === VERTICAL_ALIGN.BOTTOM) { - y = - containerCoords.y + - (maxContainerHeight - boundTextElement.height + padding); + y = containerCoords.y + (maxContainerHeight - boundTextElement.height); } else { y = containerCoords.y + - (maxContainerHeight / 2 - boundTextElement.height / 2 + padding); + (maxContainerHeight / 2 - boundTextElement.height / 2); } if (boundTextElement.textAlign === TEXT_ALIGN.LEFT) { - x = containerCoords.x + padding; + x = containerCoords.x; } else if (boundTextElement.textAlign === TEXT_ALIGN.RIGHT) { - x = - containerCoords.x + - (maxContainerWidth - boundTextElement.width + padding); + x = containerCoords.x + (maxContainerWidth - boundTextElement.width); } else { x = - containerCoords.x + - (maxContainerWidth / 2 - boundTextElement.width / 2 + padding); + containerCoords.x + (maxContainerWidth / 2 - boundTextElement.width / 2); } return { x, y }; }; @@ -636,20 +629,22 @@ export const getContainerCenter = ( }; export const getContainerCoords = (container: NonDeletedExcalidrawElement) => { + let offsetX = BOUND_TEXT_PADDING; + let offsetY = BOUND_TEXT_PADDING; + if (container.type === "ellipse") { // The derivation of coordinates is explained in https://github.com/excalidraw/excalidraw/pull/6172 - const offsetX = - (container.width / 2) * (1 - Math.sqrt(2) / 2) + BOUND_TEXT_PADDING; - const offsetY = - (container.height / 2) * (1 - Math.sqrt(2) / 2) + BOUND_TEXT_PADDING; - return { - x: container.x + offsetX, - y: container.y + offsetY, - }; + offsetX += (container.width / 2) * (1 - Math.sqrt(2) / 2); + offsetY += (container.height / 2) * (1 - Math.sqrt(2) / 2); + } + // The derivation of coordinates is explained in https://github.com/excalidraw/excalidraw/pull/6265 + if (container.type === "diamond") { + offsetX += container.width / 4; + offsetY += container.height / 4; } return { - x: container.x, - y: container.y, + x: container.x + offsetX, + y: container.y + offsetY, }; }; @@ -767,5 +762,63 @@ export const computeContainerHeightForBoundText = ( if (isArrowElement(container)) { return boundTextElementHeight + BOUND_TEXT_PADDING * 8 * 2; } + if (container.type === "diamond") { + return 2 * boundTextElementHeight; + } return boundTextElementHeight + BOUND_TEXT_PADDING * 2; }; + +export const getMaxContainerWidth = (container: ExcalidrawElement) => { + const width = getContainerDims(container).width; + if (isArrowElement(container)) { + const containerWidth = width - BOUND_TEXT_PADDING * 8 * 2; + if (containerWidth <= 0) { + const boundText = getBoundTextElement(container); + if (boundText) { + return boundText.width; + } + return BOUND_TEXT_PADDING * 8 * 2; + } + return containerWidth; + } + + if (container.type === "ellipse") { + // The width of the largest rectangle inscribed inside an ellipse is + // Math.round((ellipse.width / 2) * Math.sqrt(2)) which is derived from + // equation of an ellipse -https://github.com/excalidraw/excalidraw/pull/6172 + return Math.round((width / 2) * Math.sqrt(2)) - BOUND_TEXT_PADDING * 2; + } + if (container.type === "diamond") { + // The width of the largest rectangle inscribed inside a rhombus is + // Math.round(width / 2) - https://github.com/excalidraw/excalidraw/pull/6265 + return Math.round(width / 2) - BOUND_TEXT_PADDING * 2; + } + return width - BOUND_TEXT_PADDING * 2; +}; + +export const getMaxContainerHeight = (container: ExcalidrawElement) => { + const height = getContainerDims(container).height; + if (isArrowElement(container)) { + const containerHeight = height - BOUND_TEXT_PADDING * 8 * 2; + if (containerHeight <= 0) { + const boundText = getBoundTextElement(container); + if (boundText) { + return boundText.height; + } + return BOUND_TEXT_PADDING * 8 * 2; + } + return height; + } + if (container.type === "ellipse") { + // The height of the largest rectangle inscribed inside an ellipse is + // Math.round((ellipse.height / 2) * Math.sqrt(2)) which is derived from + // equation of an ellipse - https://github.com/excalidraw/excalidraw/pull/6172 + return Math.round((height / 2) * Math.sqrt(2)) - BOUND_TEXT_PADDING * 2; + } + if (container.type === "diamond") { + // The height of the largest rectangle inscribed inside a rhombus is + // Math.round(height / 2) - https://github.com/excalidraw/excalidraw/pull/6265 + return Math.round(height / 2) - BOUND_TEXT_PADDING * 2; + } + return height - BOUND_TEXT_PADDING * 2; +}; diff --git a/src/element/textWysiwyg.test.tsx b/src/element/textWysiwyg.test.tsx index 87a6e0bf..fb41a381 100644 --- a/src/element/textWysiwyg.test.tsx +++ b/src/element/textWysiwyg.test.tsx @@ -791,7 +791,7 @@ describe("textWysiwyg", () => { text = h.elements[1] as ExcalidrawTextElementWithContainer; expect(text.text).toBe("Hello \nWorld!"); expect(text.originalText).toBe("Hello World!"); - expect(text.y).toBe(27.5); + expect(text.y).toBe(57.5); expect(text.x).toBe(rectangle.x + BOUND_TEXT_PADDING); expect(text.height).toBe(APPROX_LINE_HEIGHT * 2); expect(text.width).toBe(rectangle.width - BOUND_TEXT_PADDING * 2); @@ -825,7 +825,7 @@ describe("textWysiwyg", () => { expect(text.text).toBe("Hello"); expect(text.originalText).toBe("Hello"); - expect(text.y).toBe(40); + expect(text.y).toBe(57.5); expect(text.x).toBe(rectangle.x + BOUND_TEXT_PADDING); expect(text.height).toBe(APPROX_LINE_HEIGHT); expect(text.width).toBe(rectangle.width - BOUND_TEXT_PADDING * 2); @@ -930,6 +930,8 @@ describe("textWysiwyg", () => { editor.select(); fireEvent.click(screen.getByTitle("Left")); + await new Promise((r) => setTimeout(r, 0)); + fireEvent.click(screen.getByTitle("Align bottom")); await new Promise((r) => setTimeout(r, 0)); @@ -1278,7 +1280,7 @@ describe("textWysiwyg", () => { expect([h.elements[1].x, h.elements[1].y]).toMatchInlineSnapshot(` Array [ 15, - 20, + 25, ] `); }); @@ -1290,7 +1292,7 @@ describe("textWysiwyg", () => { expect([h.elements[1].x, h.elements[1].y]).toMatchInlineSnapshot(` Array [ -25, - 20, + 25, ] `); }); @@ -1302,7 +1304,7 @@ describe("textWysiwyg", () => { expect([h.elements[1].x, h.elements[1].y]).toMatchInlineSnapshot(` Array [ 174, - 20, + 25, ] `); }); diff --git a/src/element/textWysiwyg.tsx b/src/element/textWysiwyg.tsx index 5536632b..d04dfb8d 100644 --- a/src/element/textWysiwyg.tsx +++ b/src/element/textWysiwyg.tsx @@ -24,14 +24,16 @@ import { mutateElement } from "./mutateElement"; import { getApproxLineHeight, getBoundTextElementId, - getBoundTextElementOffset, getContainerCoords, getContainerDims, getContainerElement, getTextElementAngle, getTextWidth, normalizeText, + redrawTextBoundingBox, wrapText, + getMaxContainerHeight, + getMaxContainerWidth, } from "./textElement"; import { actionDecreaseFontSize, @@ -39,7 +41,6 @@ import { } from "../actions/actionProperties"; import { actionZoomIn, actionZoomOut } from "../actions/actionCanvas"; import App from "../components/App"; -import { getMaxContainerHeight, getMaxContainerWidth } from "./newElement"; import { LinearElementEditor } from "./linearElementEditor"; import { parseClipboard } from "../clipboard"; @@ -231,10 +232,6 @@ export const textWysiwyg = ({ // Start pushing text upward until a diff of 30px (padding) // is reached else { - const padding = - container.type === "ellipse" - ? 0 - : getBoundTextElementOffset(updatedTextElement); const containerCoords = getContainerCoords(container); // vertically center align the text @@ -245,8 +242,7 @@ export const textWysiwyg = ({ } } if (verticalAlign === VERTICAL_ALIGN.BOTTOM) { - coordY = - containerCoords.y + (maxHeight - textElementHeight + padding); + coordY = containerCoords.y + (maxHeight - textElementHeight); } } } @@ -616,6 +612,7 @@ export const textWysiwyg = ({ ), }); } + redrawTextBoundingBox(updateElement, container); } onSubmit({ diff --git a/src/tests/linearElementEditor.test.tsx b/src/tests/linearElementEditor.test.tsx index d4b1d903..3e5ebb8c 100644 --- a/src/tests/linearElementEditor.test.tsx +++ b/src/tests/linearElementEditor.test.tsx @@ -17,8 +17,11 @@ import { KEYS } from "../keys"; import { LinearElementEditor } from "../element/linearElementEditor"; import { queryByTestId, queryByText } from "@testing-library/react"; import { resize, rotate } from "./utils"; -import { getBoundTextElementPosition, wrapText } from "../element/textElement"; -import { getMaxContainerWidth } from "../element/newElement"; +import { + getBoundTextElementPosition, + wrapText, + getMaxContainerWidth, +} from "../element/textElement"; import * as textElementUtils from "../element/textElement"; import { ROUNDNESS } from "../constants";