From 88ff32e9b389b15577f0a6fa73df92a6eda7f914 Mon Sep 17 00:00:00 2001 From: Aakansha Doshi Date: Tue, 21 Feb 2023 12:36:43 +0530 Subject: [PATCH] fix: improve text wrapping in ellipse and alignment (#6172) * fix: improve text wrapping in ellipse * compute height when font properties updated * fix alignment * fix alignment when resizing * fix * ad padding * always compute height when redrawing bounding box and refactor * lint * fix specs * fix * redraw text bounding box when pasted or refreshed * fix * Add specs * fix * restore on font load * add comments --- src/components/App.tsx | 10 ++ src/element/newElement.ts | 10 ++ src/element/textElement.test.ts | 53 ++++++++- src/element/textElement.ts | 194 +++++++++++++++++++------------ src/element/textWysiwyg.test.tsx | 52 ++++----- src/element/textWysiwyg.tsx | 14 ++- 6 files changed, 225 insertions(+), 108 deletions(-) diff --git a/src/components/App.tsx b/src/components/App.tsx index a48510bf9..effa604bf 100644 --- a/src/components/App.tsx +++ b/src/components/App.tsx @@ -108,6 +108,7 @@ import { textWysiwyg, transformElements, updateTextElement, + redrawTextBoundingBox, } from "../element"; import { bindOrUnbindLinearElement, @@ -264,6 +265,7 @@ import { getBoundTextElement, getContainerCenter, getContainerDims, + getContainerElement, getTextBindableContainerAtPosition, isValidTextContainer, } from "../element/textElement"; @@ -1637,6 +1639,14 @@ class App extends React.Component { } this.scene.replaceAllElements(nextElements); + + nextElements.forEach((nextElement) => { + if (isTextElement(nextElement) && isBoundToContainer(nextElement)) { + const container = getContainerElement(nextElement); + redrawTextBoundingBox(nextElement, container); + } + }); + this.history.resumeRecording(); this.setState( diff --git a/src/element/newElement.ts b/src/element/newElement.ts index 8e7b8ee8a..c7f33a462 100644 --- a/src/element/newElement.ts +++ b/src/element/newElement.ts @@ -290,6 +290,11 @@ export const getMaxContainerWidth = (container: ExcalidrawElement) => { 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; }; @@ -306,6 +311,11 @@ export const getMaxContainerHeight = (container: ExcalidrawElement) => { 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; }; diff --git a/src/element/textElement.test.ts b/src/element/textElement.test.ts index e1b9ff6f0..91974aca2 100644 --- a/src/element/textElement.test.ts +++ b/src/element/textElement.test.ts @@ -1,5 +1,11 @@ import { BOUND_TEXT_PADDING } from "../constants"; -import { measureText, wrapText } from "./textElement"; +import { API } from "../tests/helpers/api"; +import { + computeContainerHeightForBoundText, + getContainerCoords, + measureText, + wrapText, +} from "./textElement"; import { FontString } from "./types"; describe("Test wrapText", () => { @@ -193,4 +199,49 @@ 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({ + type: "ellipse", + ...params, + }); + expect(getContainerCoords(ellipse)).toEqual({ + x: 44.2893218813452455, + y: 39.64466094067262, + }); + }); + it("should compute coords correctly when rectangle", () => { + const rectangle = API.createElement({ + type: "rectangle", + ...params, + }); + expect(getContainerCoords(rectangle)).toEqual({ + x: 10, + y: 20, + }); + }); + }); + + describe("Test computeContainerHeightForBoundText", () => { + const params = { + width: 178, + height: 194, + }; + it("should compute container height correctly for rectangle", () => { + const element = API.createElement({ + type: "rectangle", + ...params, + }); + expect(computeContainerHeightForBoundText(element, 150)).toEqual(160); + }); + it("should compute container height correctly for ellipse", () => { + const element = API.createElement({ + type: "ellipse", + ...params, + }); + expect(computeContainerHeightForBoundText(element, 150)).toEqual(212); + }); + }); }); diff --git a/src/element/textElement.ts b/src/element/textElement.ts index c726c1c3f..ed4d27629 100644 --- a/src/element/textElement.ts +++ b/src/element/textElement.ts @@ -44,68 +44,69 @@ export const redrawTextBoundingBox = ( container: ExcalidrawElement | null, ) => { let maxWidth = undefined; - let text = textElement.text; + + const boundTextUpdates = { + x: textElement.x, + y: textElement.y, + text: textElement.text, + width: textElement.width, + height: textElement.height, + baseline: textElement.baseline, + }; + + boundTextUpdates.text = textElement.text; + if (container) { maxWidth = getMaxContainerWidth(container); - text = wrapText( + boundTextUpdates.text = wrapText( textElement.originalText, getFontString(textElement), maxWidth, ); } - const metrics = measureText(text, getFontString(textElement), maxWidth); - let coordY = textElement.y; - let coordX = textElement.x; - // Resize container and vertically center align the text + const metrics = measureText( + boundTextUpdates.text, + getFontString(textElement), + maxWidth, + ); + + boundTextUpdates.width = metrics.width; + boundTextUpdates.height = metrics.height; + boundTextUpdates.baseline = metrics.baseline; + if (container) { - if (!isArrowElement(container)) { - const containerDims = getContainerDims(container); - let nextHeight = containerDims.height; - if (textElement.verticalAlign === VERTICAL_ALIGN.TOP) { - coordY = container.y; - } else if (textElement.verticalAlign === VERTICAL_ALIGN.BOTTOM) { - coordY = - container.y + - containerDims.height - - metrics.height - - BOUND_TEXT_PADDING; - } else { - coordY = container.y + containerDims.height / 2 - metrics.height / 2; - if (metrics.height > getMaxContainerHeight(container)) { - nextHeight = metrics.height + BOUND_TEXT_PADDING * 2; - coordY = container.y + nextHeight / 2 - metrics.height / 2; - } - } - if (textElement.textAlign === TEXT_ALIGN.LEFT) { - coordX = container.x + BOUND_TEXT_PADDING; - } else if (textElement.textAlign === TEXT_ALIGN.RIGHT) { - coordX = - container.x + - containerDims.width - - metrics.width - - BOUND_TEXT_PADDING; - } else { - coordX = container.x + containerDims.width / 2 - metrics.width / 2; - } - updateOriginalContainerCache(container.id, nextHeight); - mutateElement(container, { height: nextHeight }); - } else { + if (isArrowElement(container)) { const centerX = textElement.x + textElement.width / 2; const centerY = textElement.y + textElement.height / 2; const diffWidth = metrics.width - textElement.width; const diffHeight = metrics.height - textElement.height; - coordY = centerY - (textElement.height + diffHeight) / 2; - coordX = centerX - (textElement.width + diffWidth) / 2; + boundTextUpdates.x = centerY - (textElement.height + diffHeight) / 2; + boundTextUpdates.y = centerX - (textElement.width + diffWidth) / 2; + } else { + const containerDims = getContainerDims(container); + let maxContainerHeight = getMaxContainerHeight(container); + + let nextHeight = containerDims.height; + if (metrics.height > maxContainerHeight) { + nextHeight = computeContainerHeightForBoundText( + container, + metrics.height, + ); + mutateElement(container, { height: nextHeight }); + maxContainerHeight = getMaxContainerHeight(container); + updateOriginalContainerCache(container.id, nextHeight); + } + const updatedTextElement = { + ...textElement, + ...boundTextUpdates, + } as ExcalidrawTextElementWithContainer; + const { x, y } = computeBoundTextPosition(container, updatedTextElement); + boundTextUpdates.x = x; + boundTextUpdates.y = y; } } - mutateElement(textElement, { - width: metrics.width, - height: metrics.height, - baseline: metrics.baseline, - y: coordY, - x: coordX, - text, - }); + + mutateElement(textElement, boundTextUpdates); }; export const bindTextToShapeAfterDuplication = ( @@ -197,7 +198,11 @@ export const handleBindTextResize = ( } // increase height in case text element height exceeds if (nextHeight > maxHeight) { - containerHeight = nextHeight + getBoundTextElementOffset(textElement) * 2; + containerHeight = computeContainerHeightForBoundText( + container, + nextHeight, + ); + const diff = containerHeight - containerDims.height; // fix the y coord when resizing from ne/nw/n const updatedY = @@ -217,48 +222,57 @@ export const handleBindTextResize = ( text, width: nextWidth, height: nextHeight, - baseline: nextBaseLine, }); + if (!isArrowElement(container)) { - updateBoundTextPosition( - container, - textElement as ExcalidrawTextElementWithContainer, + mutateElement( + textElement, + computeBoundTextPosition( + container, + textElement as ExcalidrawTextElementWithContainer, + ), ); } } }; -const updateBoundTextPosition = ( +const computeBoundTextPosition = ( container: ExcalidrawElement, boundTextElement: ExcalidrawTextElementWithContainer, ) => { - const containerDims = getContainerDims(container); - const boundTextElementPadding = getBoundTextElementOffset(boundTextElement); + 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 = container.y + boundTextElementPadding; + y = containerCoords.y + padding; } else if (boundTextElement.verticalAlign === VERTICAL_ALIGN.BOTTOM) { y = - container.y + - containerDims.height - - boundTextElement.height - - boundTextElementPadding; + containerCoords.y + + (maxContainerHeight - boundTextElement.height + padding); } else { - y = container.y + containerDims.height / 2 - boundTextElement.height / 2; + y = + containerCoords.y + + (maxContainerHeight / 2 - boundTextElement.height / 2 + padding); } - const x = - boundTextElement.textAlign === TEXT_ALIGN.LEFT - ? container.x + boundTextElementPadding - : boundTextElement.textAlign === TEXT_ALIGN.RIGHT - ? container.x + - containerDims.width - - boundTextElement.width - - boundTextElementPadding - : container.x + containerDims.width / 2 - boundTextElement.width / 2; - - mutateElement(boundTextElement, { x, y }); + if (boundTextElement.textAlign === TEXT_ALIGN.LEFT) { + x = containerCoords.x + padding; + } else if (boundTextElement.textAlign === TEXT_ALIGN.RIGHT) { + x = + containerCoords.x + + (maxContainerWidth - boundTextElement.width + padding); + } else { + x = + containerCoords.x + + (maxContainerWidth / 2 - boundTextElement.width / 2 + padding); + } + return { x, y }; }; + // https://github.com/grassator/canvas-text-editor/blob/master/lib/FontMetrics.js export const measureText = ( text: string, @@ -621,6 +635,24 @@ export const getContainerCenter = ( return { x: midSegmentMidpoint[0], y: midSegmentMidpoint[1] }; }; +export const getContainerCoords = (container: NonDeletedExcalidrawElement) => { + 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, + }; + } + return { + x: container.x, + y: container.y, + }; +}; + export const getTextElementAngle = (textElement: ExcalidrawTextElement) => { const container = getContainerElement(textElement); if (!container || isArrowElement(container)) { @@ -633,12 +665,13 @@ export const getBoundTextElementOffset = ( boundTextElement: ExcalidrawTextElement | null, ) => { const container = getContainerElement(boundTextElement); - if (!container) { + if (!container || !boundTextElement) { return 0; } if (isArrowElement(container)) { return BOUND_TEXT_PADDING * 8; } + return BOUND_TEXT_PADDING; }; @@ -723,3 +756,16 @@ export const isValidTextContainer = (element: ExcalidrawElement) => { isArrowElement(element) ); }; + +export const computeContainerHeightForBoundText = ( + container: NonDeletedExcalidrawElement, + boundTextElementHeight: number, +) => { + if (container.type === "ellipse") { + return Math.round((boundTextElementHeight / Math.sqrt(2)) * 2); + } + if (isArrowElement(container)) { + return boundTextElementHeight + BOUND_TEXT_PADDING * 8 * 2; + } + return boundTextElementHeight + BOUND_TEXT_PADDING * 2; +}; diff --git a/src/element/textWysiwyg.test.tsx b/src/element/textWysiwyg.test.tsx index c61d4e68b..87a6e0bf5 100644 --- a/src/element/textWysiwyg.test.tsx +++ b/src/element/textWysiwyg.test.tsx @@ -791,9 +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( - rectangle.y + rectangle.height / 2 - (APPROX_LINE_HEIGHT * 2) / 2, - ); + expect(text.y).toBe(27.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); @@ -827,9 +825,7 @@ describe("textWysiwyg", () => { expect(text.text).toBe("Hello"); expect(text.originalText).toBe("Hello"); - expect(text.y).toBe( - rectangle.y + rectangle.height / 2 - APPROX_LINE_HEIGHT / 2, - ); + expect(text.y).toBe(40); 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); @@ -1248,7 +1244,7 @@ describe("textWysiwyg", () => { expect([h.elements[1].x, h.elements[1].y]).toMatchInlineSnapshot(` Array [ 15, - 20, + 25, ] `); }); @@ -1259,7 +1255,7 @@ describe("textWysiwyg", () => { expect([h.elements[1].x, h.elements[1].y]).toMatchInlineSnapshot(` Array [ 94.5, - 20, + 25, ] `); }); @@ -1269,22 +1265,22 @@ describe("textWysiwyg", () => { fireEvent.click(screen.getByTitle("Align top")); expect([h.elements[1].x, h.elements[1].y]).toMatchInlineSnapshot(` - Array [ - 174, - 20, - ] - `); + Array [ + 174, + 25, + ] + `); }); it("when center left", async () => { fireEvent.click(screen.getByTitle("Center vertically")); fireEvent.click(screen.getByTitle("Left")); expect([h.elements[1].x, h.elements[1].y]).toMatchInlineSnapshot(` - Array [ - 15, - 25, - ] - `); + Array [ + 15, + 20, + ] + `); }); it("when center center", async () => { @@ -1292,11 +1288,11 @@ describe("textWysiwyg", () => { fireEvent.click(screen.getByTitle("Center vertically")); expect([h.elements[1].x, h.elements[1].y]).toMatchInlineSnapshot(` - Array [ - -25, - 25, - ] - `); + Array [ + -25, + 20, + ] + `); }); it("when center right", async () => { @@ -1304,11 +1300,11 @@ describe("textWysiwyg", () => { fireEvent.click(screen.getByTitle("Center vertically")); expect([h.elements[1].x, h.elements[1].y]).toMatchInlineSnapshot(` - Array [ - 174, - 25, - ] - `); + Array [ + 174, + 20, + ] + `); }); it("when bottom left", async () => { diff --git a/src/element/textWysiwyg.tsx b/src/element/textWysiwyg.tsx index 7b0098ce0..5536632b4 100644 --- a/src/element/textWysiwyg.tsx +++ b/src/element/textWysiwyg.tsx @@ -25,6 +25,7 @@ import { getApproxLineHeight, getBoundTextElementId, getBoundTextElementOffset, + getContainerCoords, getContainerDims, getContainerElement, getTextElementAngle, @@ -230,19 +231,22 @@ 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 if (verticalAlign === VERTICAL_ALIGN.MIDDLE) { if (!isArrowElement(container)) { coordY = - container.y + containerDims.height / 2 - textElementHeight / 2; + containerCoords.y + maxHeight / 2 - textElementHeight / 2; } } if (verticalAlign === VERTICAL_ALIGN.BOTTOM) { coordY = - container.y + - containerDims.height - - textElementHeight - - getBoundTextElementOffset(updatedTextElement); + containerCoords.y + (maxHeight - textElementHeight + padding); } } }