From 83383977f58182e3dd80dc2ade2b9b51434455fe Mon Sep 17 00:00:00 2001 From: Aakansha Doshi Date: Wed, 22 Mar 2023 11:32:38 +0530 Subject: [PATCH] feat: add line height attribute to text element (#6360) * feat: add line height attribute to text element * lint * update line height when redrawing text bounding box * fix tests * retain line height when pasting styles * fix test * create a util for calculating ling height using old algo * update line height when resizing multiple text elements * make line height backward compatible * udpate line height for older element when font size updated * remove logs * Add specs * lint * review fixes * simplify by changing `lineHeight` from px to unitless * make param non-optional * update comment * fix: jumping text due to font size being calculated incorrectly * update line height when font family is updated * lint * Add spec * more specs * rename to getDefaultLineHeight * fix getting lineHeight for potentially undefined fontFamily * reduce duplication * fix fallback * refactor and comment tweaks * fix --------- Co-authored-by: dwelle --- src/actions/actionBoundText.tsx | 1 + src/actions/actionProperties.tsx | 2 + src/actions/actionStyles.ts | 17 ++- src/components/App.tsx | 39 +++--- src/data/restore.ts | 22 +++- src/element/newElement.ts | 13 +- src/element/resizeElements.ts | 28 +++-- src/element/textElement.test.ts | 60 ++++++--- src/element/textElement.ts | 116 ++++++++++++++---- src/element/textWysiwyg.test.tsx | 65 +++++++--- src/element/textWysiwyg.tsx | 21 ++-- src/element/types.ts | 5 + src/renderer/renderElement.ts | 20 +-- .../linearElementEditor.test.tsx.snap | 2 +- src/tests/clipboard.test.tsx | 34 +++-- .../data/__snapshots__/restore.test.ts.snap | 4 +- src/tests/helpers/api.ts | 6 +- src/tests/linearElementEditor.test.tsx | 14 +-- 18 files changed, 326 insertions(+), 143 deletions(-) diff --git a/src/actions/actionBoundText.tsx b/src/actions/actionBoundText.tsx index bdf1df60..639a003f 100644 --- a/src/actions/actionBoundText.tsx +++ b/src/actions/actionBoundText.tsx @@ -45,6 +45,7 @@ export const actionUnbindText = register({ const { width, height } = measureText( boundTextElement.originalText, getFontString(boundTextElement), + boundTextElement.lineHeight, ); const originalContainerHeight = getOriginalContainerHeightFromCache( element.id, diff --git a/src/actions/actionProperties.tsx b/src/actions/actionProperties.tsx index 309e46bd..08420d9e 100644 --- a/src/actions/actionProperties.tsx +++ b/src/actions/actionProperties.tsx @@ -54,6 +54,7 @@ import { mutateElement, newElementWith } from "../element/mutateElement"; import { getBoundTextElement, getContainerElement, + getDefaultLineHeight, } from "../element/textElement"; import { isBoundToContainer, @@ -637,6 +638,7 @@ export const actionChangeFontFamily = register({ oldElement, { fontFamily: value, + lineHeight: getDefaultLineHeight(value), }, ); redrawTextBoundingBox(newElement, getContainerElement(oldElement)); diff --git a/src/actions/actionStyles.ts b/src/actions/actionStyles.ts index b2be3853..aaa59d32 100644 --- a/src/actions/actionStyles.ts +++ b/src/actions/actionStyles.ts @@ -12,7 +12,10 @@ import { DEFAULT_FONT_FAMILY, DEFAULT_TEXT_ALIGN, } from "../constants"; -import { getBoundTextElement } from "../element/textElement"; +import { + getBoundTextElement, + getDefaultLineHeight, +} from "../element/textElement"; import { hasBoundTextElement, canApplyRoundnessTypeToElement, @@ -92,12 +95,18 @@ export const actionPasteStyles = register({ }); if (isTextElement(newElement)) { + const fontSize = + elementStylesToCopyFrom?.fontSize || DEFAULT_FONT_SIZE; + const fontFamily = + elementStylesToCopyFrom?.fontFamily || DEFAULT_FONT_FAMILY; newElement = newElementWith(newElement, { - fontSize: elementStylesToCopyFrom?.fontSize || DEFAULT_FONT_SIZE, - fontFamily: - elementStylesToCopyFrom?.fontFamily || DEFAULT_FONT_FAMILY, + fontSize, + fontFamily, textAlign: elementStylesToCopyFrom?.textAlign || DEFAULT_TEXT_ALIGN, + lineHeight: + elementStylesToCopyFrom.lineHeight || + getDefaultLineHeight(fontFamily), }); let container = null; if (newElement.containerId) { diff --git a/src/components/App.tsx b/src/components/App.tsx index 714c349c..624d4e34 100644 --- a/src/components/App.tsx +++ b/src/components/App.tsx @@ -260,13 +260,14 @@ import throttle from "lodash.throttle"; import { fileOpen, FileSystemHandle } from "../data/filesystem"; import { bindTextToShapeAfterDuplication, - getApproxLineHeight, getApproxMinLineHeight, getApproxMinLineWidth, getBoundTextElement, getContainerCenter, getContainerDims, getContainerElement, + getDefaultLineHeight, + getLineHeightInPx, getTextBindableContainerAtPosition, isMeasureTextSupported, isValidTextContainer, @@ -1731,12 +1732,14 @@ class App extends React.Component { (acc: ExcalidrawTextElement[], line, idx) => { const text = line.trim(); + const lineHeight = getDefaultLineHeight(textElementProps.fontFamily); if (text.length) { const element = newTextElement({ ...textElementProps, x, y: currentY, text, + lineHeight, }); acc.push(element); currentY += element.height + LINE_GAP; @@ -1745,14 +1748,9 @@ class App extends React.Component { // add paragraph only if previous line was not empty, IOW don't add // more than one empty line if (prevLine) { - const defaultLineHeight = getApproxLineHeight( - getFontString({ - fontSize: textElementProps.fontSize, - fontFamily: textElementProps.fontFamily, - }), - ); - - currentY += defaultLineHeight + LINE_GAP; + currentY += + getLineHeightInPx(textElementProps.fontSize, lineHeight) + + LINE_GAP; } } @@ -2607,6 +2605,13 @@ class App extends React.Component { existingTextElement = this.getTextElementAtPosition(sceneX, sceneY); } + const fontFamily = + existingTextElement?.fontFamily || this.state.currentItemFontFamily; + + const lineHeight = + existingTextElement?.lineHeight || getDefaultLineHeight(fontFamily); + const fontSize = this.state.currentItemFontSize; + if ( !existingTextElement && shouldBindToContainer && @@ -2614,11 +2619,14 @@ class App extends React.Component { !isArrowElement(container) ) { const fontString = { - fontSize: this.state.currentItemFontSize, - fontFamily: this.state.currentItemFontFamily, + fontSize, + fontFamily, }; - const minWidth = getApproxMinLineWidth(getFontString(fontString)); - const minHeight = getApproxMinLineHeight(getFontString(fontString)); + const minWidth = getApproxMinLineWidth( + getFontString(fontString), + lineHeight, + ); + const minHeight = getApproxMinLineHeight(fontSize, lineHeight); const containerDims = getContainerDims(container); const newHeight = Math.max(containerDims.height, minHeight); const newWidth = Math.max(containerDims.width, minWidth); @@ -2652,8 +2660,8 @@ class App extends React.Component { opacity: this.state.currentItemOpacity, roundness: null, text: "", - fontSize: this.state.currentItemFontSize, - fontFamily: this.state.currentItemFontFamily, + fontSize, + fontFamily, textAlign: parentCenterPosition ? "center" : this.state.currentItemTextAlign, @@ -2663,6 +2671,7 @@ class App extends React.Component { containerId: shouldBindToContainer ? container?.id : undefined, groupIds: container?.groupIds ?? [], locked: false, + lineHeight, }); if (!existingTextElement && shouldBindToContainer && container) { diff --git a/src/data/restore.ts b/src/data/restore.ts index 98df4547..270e06d9 100644 --- a/src/data/restore.ts +++ b/src/data/restore.ts @@ -35,6 +35,7 @@ import { getUpdatedTimestamp, updateActiveTool } from "../utils"; import { arrayToMap } from "../utils"; import oc from "open-color"; import { MarkOptional, Mutable } from "../utility-types"; +import { detectLineHeight, getDefaultLineHeight } from "../element/textElement"; type RestoredAppState = Omit< AppState, @@ -165,17 +166,32 @@ const restoreElement = ( const [fontPx, _fontFamily]: [string, string] = ( element as any ).font.split(" "); - fontSize = parseInt(fontPx, 10); + fontSize = parseFloat(fontPx); fontFamily = getFontFamilyByName(_fontFamily); } + const text = element.text ?? ""; + element = restoreElementWithProperties(element, { fontSize, fontFamily, - text: element.text ?? "", + text, textAlign: element.textAlign || DEFAULT_TEXT_ALIGN, verticalAlign: element.verticalAlign || DEFAULT_VERTICAL_ALIGN, containerId: element.containerId ?? null, - originalText: element.originalText || element.text, + originalText: element.originalText || text, + // line-height might not be specified either when creating elements + // programmatically, or when importing old diagrams. + // For the latter we want to detect the original line height which + // will likely differ from our per-font fixed line height we now use, + // to maintain backward compatibility. + lineHeight: + element.lineHeight || + (element.height + ? // detect line-height from current element height and font-size + detectLineHeight(element) + : // no element height likely means programmatic use, so default + // to a fixed line height + getDefaultLineHeight(element.fontFamily)), }); if (refreshDimensions) { diff --git a/src/element/newElement.ts b/src/element/newElement.ts index 0fe16b55..6581097c 100644 --- a/src/element/newElement.ts +++ b/src/element/newElement.ts @@ -29,6 +29,7 @@ import { normalizeText, wrapText, getMaxContainerWidth, + getDefaultLineHeight, } from "./textElement"; import { VERTICAL_ALIGN } from "../constants"; import { isArrowElement } from "./typeChecks"; @@ -137,10 +138,12 @@ export const newTextElement = ( textAlign: TextAlign; verticalAlign: VerticalAlign; containerId?: ExcalidrawTextContainer["id"]; + lineHeight?: ExcalidrawTextElement["lineHeight"]; } & ElementConstructorOpts, ): NonDeleted => { + const lineHeight = opts.lineHeight || getDefaultLineHeight(opts.fontFamily); const text = normalizeText(opts.text); - const metrics = measureText(text, getFontString(opts)); + const metrics = measureText(text, getFontString(opts), lineHeight); const offsets = getTextElementPositionOffsets(opts, metrics); const textElement = newElementWith( { @@ -156,6 +159,7 @@ export const newTextElement = ( height: metrics.height, containerId: opts.containerId || null, originalText: text, + lineHeight, }, {}, ); @@ -176,6 +180,7 @@ const getAdjustedDimensions = ( const { width: nextWidth, height: nextHeight } = measureText( nextText, getFontString(element), + element.lineHeight, ); const { textAlign, verticalAlign } = element; let x: number; @@ -185,7 +190,11 @@ const getAdjustedDimensions = ( verticalAlign === VERTICAL_ALIGN.MIDDLE && !element.containerId ) { - const prevMetrics = measureText(element.text, getFontString(element)); + const prevMetrics = measureText( + element.text, + getFontString(element), + element.lineHeight, + ); const offsets = getTextElementPositionOffsets(element, { width: nextWidth - prevMetrics.width, height: nextHeight - prevMetrics.height, diff --git a/src/element/resizeElements.ts b/src/element/resizeElements.ts index 05eb630e..a49559a7 100644 --- a/src/element/resizeElements.ts +++ b/src/element/resizeElements.ts @@ -39,13 +39,13 @@ import { import { Point, PointerDownState } from "../types"; import Scene from "../scene/Scene"; import { - getApproxMinLineHeight, getApproxMinLineWidth, getBoundTextElement, getBoundTextElementId, getContainerElement, handleBindTextResize, getMaxContainerWidth, + getApproxMinLineHeight, } from "./textElement"; export const normalizeAngle = (angle: number): number => { @@ -360,7 +360,7 @@ export const resizeSingleElement = ( let scaleX = atStartBoundsWidth / boundsCurrentWidth; let scaleY = atStartBoundsHeight / boundsCurrentHeight; - let boundTextFont: { fontSize?: number } = {}; + let boundTextFontSize: number | null = null; const boundTextElement = getBoundTextElement(element); if (transformHandleDirection.includes("e")) { @@ -410,9 +410,7 @@ export const resizeSingleElement = ( boundTextElement.id, ) as typeof boundTextElement | undefined; if (stateOfBoundTextElementAtResize) { - boundTextFont = { - fontSize: stateOfBoundTextElementAtResize.fontSize, - }; + boundTextFontSize = stateOfBoundTextElementAtResize.fontSize; } if (shouldMaintainAspectRatio) { const updatedElement = { @@ -428,12 +426,16 @@ export const resizeSingleElement = ( if (nextFontSize === null) { return; } - boundTextFont = { - fontSize: nextFontSize, - }; + boundTextFontSize = nextFontSize; } else { - const minWidth = getApproxMinLineWidth(getFontString(boundTextElement)); - const minHeight = getApproxMinLineHeight(getFontString(boundTextElement)); + const minWidth = getApproxMinLineWidth( + getFontString(boundTextElement), + boundTextElement.lineHeight, + ); + const minHeight = getApproxMinLineHeight( + boundTextElement.fontSize, + boundTextElement.lineHeight, + ); eleNewWidth = Math.ceil(Math.max(eleNewWidth, minWidth)); eleNewHeight = Math.ceil(Math.max(eleNewHeight, minHeight)); } @@ -566,8 +568,10 @@ export const resizeSingleElement = ( }); mutateElement(element, resizedElement); - if (boundTextElement && boundTextFont) { - mutateElement(boundTextElement, { fontSize: boundTextFont.fontSize }); + if (boundTextElement && boundTextFontSize != null) { + mutateElement(boundTextElement, { + fontSize: boundTextFontSize, + }); } handleBindTextResize(element, transformHandleDirection); } diff --git a/src/element/textElement.test.ts b/src/element/textElement.test.ts index be6d1a8a..aa7df8ee 100644 --- a/src/element/textElement.test.ts +++ b/src/element/textElement.test.ts @@ -1,4 +1,4 @@ -import { BOUND_TEXT_PADDING } from "../constants"; +import { BOUND_TEXT_PADDING, FONT_FAMILY } from "../constants"; import { API } from "../tests/helpers/api"; import { computeContainerDimensionForBoundText, @@ -6,6 +6,9 @@ import { getMaxContainerWidth, getMaxContainerHeight, wrapText, + detectLineHeight, + getLineHeightInPx, + getDefaultLineHeight, } from "./textElement"; import { FontString } from "./types"; @@ -40,9 +43,7 @@ describe("Test wrapText", () => { { desc: "break all words when width of each word is less than container width", width: 80, - res: `Hello -whats -up`, + res: `Hello \nwhats \nup`, }, { desc: "break all characters when width of each character is less than container width", @@ -64,8 +65,7 @@ p`, desc: "break words as per the width", width: 140, - res: `Hello whats -up`, + res: `Hello whats \nup`, }, { desc: "fit the container", @@ -95,9 +95,7 @@ whats up`; { desc: "break all words when width of each word is less than container width", width: 80, - res: `Hello -whats -up`, + res: `Hello\nwhats \nup`, }, { desc: "break all characters when width of each character is less than container width", @@ -143,11 +141,7 @@ whats up`, { desc: "fit characters of long string as per container width", width: 170, - res: `hellolongtextth -isiswhatsupwith -youIamtypingggg -gandtypinggg -break it now`, + res: `hellolongtextth\nisiswhatsupwith\nyouIamtypingggg\ngandtypinggg \nbreak it now`, }, { @@ -166,8 +160,7 @@ now`, desc: "fit the long text when container width is greater than text length and move the rest to next line", width: 600, - res: `hellolongtextthisiswhatsupwithyouIamtypingggggandtypinggg -break it now`, + res: `hellolongtextthisiswhatsupwithyouIamtypingggggandtypinggg \nbreak it now`, }, ].forEach((data) => { it(`should ${data.desc}`, () => { @@ -181,8 +174,7 @@ break it now`, const text = "Hello Excalidraw"; // Length of "Excalidraw" is 100 and exacty equal to max width const res = wrapText(text, font, 100); - expect(res).toEqual(`Hello -Excalidraw`); + expect(res).toEqual(`Hello \nExcalidraw`); }); it("should return the text as is if max width is invalid", () => { @@ -312,3 +304,35 @@ describe("Test measureText", () => { }); }); }); + +const textElement = API.createElement({ + type: "text", + text: "Excalidraw is a\nvirtual \nopensource \nwhiteboard for \nsketching \nhand-drawn like\ndiagrams", + fontSize: 20, + fontFamily: 1, + height: 175, +}); + +describe("Test detectLineHeight", () => { + it("should return correct line height", () => { + expect(detectLineHeight(textElement)).toBe(1.25); + }); +}); + +describe("Test getLineHeightInPx", () => { + it("should return correct line height", () => { + expect( + getLineHeightInPx(textElement.fontSize, textElement.lineHeight), + ).toBe(25); + }); +}); + +describe("Test getDefaultLineHeight", () => { + it("should return line height using default font family when not passed", () => { + //@ts-ignore + expect(getDefaultLineHeight()).toBe(1.25); + }); + it("should return correct line height", () => { + expect(getDefaultLineHeight(FONT_FAMILY.Cascadia)).toBe(1.2); + }); +}); diff --git a/src/element/textElement.ts b/src/element/textElement.ts index 8d1b0901..fd501c14 100644 --- a/src/element/textElement.ts +++ b/src/element/textElement.ts @@ -4,6 +4,7 @@ import { ExcalidrawTextContainer, ExcalidrawTextElement, ExcalidrawTextElementWithContainer, + FontFamilyValues, FontString, NonDeletedExcalidrawElement, } from "./types"; @@ -12,6 +13,7 @@ import { BOUND_TEXT_PADDING, DEFAULT_FONT_FAMILY, DEFAULT_FONT_SIZE, + FONT_FAMILY, TEXT_ALIGN, VERTICAL_ALIGN, } from "../constants"; @@ -41,12 +43,15 @@ export const normalizeText = (text: string) => { ); }; +export const splitIntoLines = (text: string) => { + return normalizeText(text).split("\n"); +}; + export const redrawTextBoundingBox = ( textElement: ExcalidrawTextElement, container: ExcalidrawElement | null, ) => { let maxWidth = undefined; - const boundTextUpdates = { x: textElement.x, y: textElement.y, @@ -68,6 +73,7 @@ export const redrawTextBoundingBox = ( const metrics = measureText( boundTextUpdates.text, getFontString(textElement), + textElement.lineHeight, ); boundTextUpdates.width = metrics.width; @@ -185,7 +191,11 @@ export const handleBindTextResize = ( maxWidth, ); } - const dimensions = measureText(text, getFontString(textElement)); + const dimensions = measureText( + text, + getFontString(textElement), + textElement.lineHeight, + ); nextHeight = dimensions.height; nextWidth = dimensions.width; } @@ -261,32 +271,52 @@ const computeBoundTextPosition = ( // https://github.com/grassator/canvas-text-editor/blob/master/lib/FontMetrics.js -export const measureText = (text: string, font: FontString) => { +export const measureText = ( + text: string, + font: FontString, + lineHeight: ExcalidrawTextElement["lineHeight"], +) => { text = text .split("\n") // replace empty lines with single space because leading/trailing empty // lines would be stripped from computation .map((x) => x || " ") .join("\n"); - - const height = getTextHeight(text, font); + const fontSize = parseFloat(font); + const height = getTextHeight(text, fontSize, lineHeight); const width = getTextWidth(text, font); return { width, height }; }; -const DUMMY_TEXT = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789".toLocaleUpperCase(); -const cacheApproxLineHeight: { [key: FontString]: number } = {}; +/** + * To get unitless line-height (if unknown) we can calculate it by dividing + * height-per-line by fontSize. + */ +export const detectLineHeight = (textElement: ExcalidrawTextElement) => { + const lineCount = splitIntoLines(textElement.text).length; + return (textElement.height / + lineCount / + textElement.fontSize) as ExcalidrawTextElement["lineHeight"]; +}; -export const getApproxLineHeight = (font: FontString) => { - if (cacheApproxLineHeight[font]) { - return cacheApproxLineHeight[font]; - } - const fontSize = parseInt(font); +/** + * We calculate the line height from the font size and the unitless line height, + * aligning with the W3C spec. + */ +export const getLineHeightInPx = ( + fontSize: ExcalidrawTextElement["fontSize"], + lineHeight: ExcalidrawTextElement["lineHeight"], +) => { + return fontSize * lineHeight; +}; - // Calculate line height relative to font size - cacheApproxLineHeight[font] = fontSize * 1.2; - return cacheApproxLineHeight[font]; +// FIXME rename to getApproxMinContainerHeight +export const getApproxMinLineHeight = ( + fontSize: ExcalidrawTextElement["fontSize"], + lineHeight: ExcalidrawTextElement["lineHeight"], +) => { + return getLineHeightInPx(fontSize, lineHeight) + BOUND_TEXT_PADDING * 2; }; let canvas: HTMLCanvasElement | undefined; @@ -309,7 +339,7 @@ const getLineWidth = (text: string, font: FontString) => { }; export const getTextWidth = (text: string, font: FontString) => { - const lines = text.replace(/\r\n?/g, "\n").split("\n"); + const lines = splitIntoLines(text); let width = 0; lines.forEach((line) => { width = Math.max(width, getLineWidth(line, font)); @@ -317,10 +347,13 @@ export const getTextWidth = (text: string, font: FontString) => { return width; }; -export const getTextHeight = (text: string, font: FontString) => { - const lines = text.replace(/\r\n?/g, "\n").split("\n"); - const lineHeight = getApproxLineHeight(font); - return lineHeight * lines.length; +export const getTextHeight = ( + text: string, + fontSize: number, + lineHeight: ExcalidrawTextElement["lineHeight"], +) => { + const lineCount = splitIntoLines(text).length; + return getLineHeightInPx(fontSize, lineHeight) * lineCount; }; export const wrapText = (text: string, font: FontString, maxWidth: number) => { @@ -468,21 +501,23 @@ export const charWidth = (() => { }; })(); -export const getApproxMinLineWidth = (font: FontString) => { +const DUMMY_TEXT = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789".toLocaleUpperCase(); + +// FIXME rename to getApproxMinContainerWidth +export const getApproxMinLineWidth = ( + font: FontString, + lineHeight: ExcalidrawTextElement["lineHeight"], +) => { const maxCharWidth = getMaxCharWidth(font); if (maxCharWidth === 0) { return ( - measureText(DUMMY_TEXT.split("").join("\n"), font).width + + measureText(DUMMY_TEXT.split("").join("\n"), font, lineHeight).width + BOUND_TEXT_PADDING * 2 ); } return maxCharWidth + BOUND_TEXT_PADDING * 2; }; -export const getApproxMinLineHeight = (font: FontString) => { - return getApproxLineHeight(font) + BOUND_TEXT_PADDING * 2; -}; - export const getMinCharWidth = (font: FontString) => { const cache = charWidth.getCache(font); if (!cache) { @@ -828,3 +863,32 @@ export const isMeasureTextSupported = () => { ); return width > 0; }; + +/** + * Unitless line height + * + * In previous versions we used `normal` line height, which browsers interpret + * differently, and based on font-family and font-size. + * + * To make line heights consistent across browsers we hardcode the values for + * each of our fonts based on most common average line-heights. + * See https://github.com/excalidraw/excalidraw/pull/6360#issuecomment-1477635971 + * where the values come from. + */ +const DEFAULT_LINE_HEIGHT = { + // ~1.25 is the average for Virgil in WebKit and Blink. + // Gecko (FF) uses ~1.28. + [FONT_FAMILY.Virgil]: 1.25 as ExcalidrawTextElement["lineHeight"], + // ~1.15 is the average for Virgil in WebKit and Blink. + // Gecko if all over the place. + [FONT_FAMILY.Helvetica]: 1.15 as ExcalidrawTextElement["lineHeight"], + // ~1.2 is the average for Virgil in WebKit and Blink, and kinda Gecko too + [FONT_FAMILY.Cascadia]: 1.2 as ExcalidrawTextElement["lineHeight"], +}; + +export const getDefaultLineHeight = (fontFamily: FontFamilyValues) => { + if (fontFamily) { + return DEFAULT_LINE_HEIGHT[fontFamily]; + } + return DEFAULT_LINE_HEIGHT[DEFAULT_FONT_FAMILY]; +}; diff --git a/src/element/textWysiwyg.test.tsx b/src/element/textWysiwyg.test.tsx index 25d26630..0873a2d2 100644 --- a/src/element/textWysiwyg.test.tsx +++ b/src/element/textWysiwyg.test.tsx @@ -783,7 +783,7 @@ describe("textWysiwyg", () => { rectangle.y + h.elements[0].height / 2 - text.height / 2, ); expect(text.x).toBe(25); - expect(text.height).toBe(48); + expect(text.height).toBe(50); expect(text.width).toBe(60); // Edit and text by removing second line and it should @@ -810,7 +810,7 @@ describe("textWysiwyg", () => { expect(text.text).toBe("Hello"); expect(text.originalText).toBe("Hello"); - expect(text.height).toBe(24); + expect(text.height).toBe(25); expect(text.width).toBe(50); expect(text.y).toBe( rectangle.y + h.elements[0].height / 2 - text.height / 2, @@ -903,7 +903,7 @@ describe("textWysiwyg", () => { expect([h.elements[1].x, h.elements[1].y]).toMatchInlineSnapshot(` Array [ 85, - 5, + 4.5, ] `); @@ -929,7 +929,7 @@ describe("textWysiwyg", () => { expect([h.elements[1].x, h.elements[1].y]).toMatchInlineSnapshot(` Array [ 15, - 66, + 65, ] `); @@ -1067,9 +1067,9 @@ describe("textWysiwyg", () => { mouse.moveTo(rectangle.x + 100, rectangle.y + 50); mouse.up(rectangle.x + 100, rectangle.y + 50); expect(rectangle.x).toBe(80); - expect(rectangle.y).toBe(-35); + expect(rectangle.y).toBe(-40); expect(text.x).toBe(85); - expect(text.y).toBe(-30); + expect(text.y).toBe(-35); Keyboard.withModifierKeys({ ctrl: true }, () => { Keyboard.keyPress(KEYS.Z); @@ -1112,7 +1112,7 @@ describe("textWysiwyg", () => { target: { value: "Online whiteboard collaboration made easy" }, }); editor.blur(); - expect(rectangle.height).toBe(178); + expect(rectangle.height).toBe(185); mouse.select(rectangle); fireEvent.contextMenu(GlobalTestState.canvas, { button: 2, @@ -1186,6 +1186,41 @@ describe("textWysiwyg", () => { ); }); + it("should update line height when font family updated", async () => { + Keyboard.keyPress(KEYS.ENTER); + expect(getOriginalContainerHeightFromCache(rectangle.id)).toBe(75); + + const editor = document.querySelector( + ".excalidraw-textEditorContainer > textarea", + ) as HTMLTextAreaElement; + + await new Promise((r) => setTimeout(r, 0)); + fireEvent.change(editor, { target: { value: "Hello World!" } }); + editor.blur(); + expect( + (h.elements[1] as ExcalidrawTextElementWithContainer).lineHeight, + ).toEqual(1.25); + + mouse.select(rectangle); + Keyboard.keyPress(KEYS.ENTER); + + fireEvent.click(screen.getByTitle(/code/i)); + expect( + (h.elements[1] as ExcalidrawTextElementWithContainer).fontFamily, + ).toEqual(FONT_FAMILY.Cascadia); + expect( + (h.elements[1] as ExcalidrawTextElementWithContainer).lineHeight, + ).toEqual(1.2); + + fireEvent.click(screen.getByTitle(/normal/i)); + expect( + (h.elements[1] as ExcalidrawTextElementWithContainer).fontFamily, + ).toEqual(FONT_FAMILY.Helvetica); + expect( + (h.elements[1] as ExcalidrawTextElementWithContainer).lineHeight, + ).toEqual(1.15); + }); + describe("should align correctly", () => { let editor: HTMLTextAreaElement; @@ -1245,7 +1280,7 @@ describe("textWysiwyg", () => { expect([h.elements[1].x, h.elements[1].y]).toMatchInlineSnapshot(` Array [ 15, - 45.5, + 45, ] `); }); @@ -1257,7 +1292,7 @@ describe("textWysiwyg", () => { expect([h.elements[1].x, h.elements[1].y]).toMatchInlineSnapshot(` Array [ 30, - 45.5, + 45, ] `); }); @@ -1269,7 +1304,7 @@ describe("textWysiwyg", () => { expect([h.elements[1].x, h.elements[1].y]).toMatchInlineSnapshot(` Array [ 45, - 45.5, + 45, ] `); }); @@ -1281,7 +1316,7 @@ describe("textWysiwyg", () => { expect([h.elements[1].x, h.elements[1].y]).toMatchInlineSnapshot(` Array [ 15, - 66, + 65, ] `); }); @@ -1292,7 +1327,7 @@ describe("textWysiwyg", () => { expect([h.elements[1].x, h.elements[1].y]).toMatchInlineSnapshot(` Array [ 30, - 66, + 65, ] `); }); @@ -1303,7 +1338,7 @@ describe("textWysiwyg", () => { expect([h.elements[1].x, h.elements[1].y]).toMatchInlineSnapshot(` Array [ 45, - 66, + 65, ] `); }); @@ -1333,7 +1368,7 @@ describe("textWysiwyg", () => { const textElement = h.elements[1] as ExcalidrawTextElement; expect(textElement.width).toBe(600); - expect(textElement.height).toBe(24); + expect(textElement.height).toBe(25); expect(textElement.textAlign).toBe(TEXT_ALIGN.LEFT); expect((textElement as ExcalidrawTextElement).text).toBe( "Excalidraw is an opensource virtual collaborative whiteboard", @@ -1365,7 +1400,7 @@ describe("textWysiwyg", () => { ], fillStyle: "hachure", groupIds: [], - height: 34, + height: 35, isDeleted: false, link: null, locked: false, diff --git a/src/element/textWysiwyg.tsx b/src/element/textWysiwyg.tsx index e2810e81..4036719d 100644 --- a/src/element/textWysiwyg.tsx +++ b/src/element/textWysiwyg.tsx @@ -22,7 +22,6 @@ import { import { AppState } from "../types"; import { mutateElement } from "./mutateElement"; import { - getApproxLineHeight, getBoundTextElementId, getContainerCoords, getContainerDims, @@ -150,9 +149,7 @@ export const textWysiwyg = ({ return; } const { textAlign, verticalAlign } = updatedTextElement; - const approxLineHeight = getApproxLineHeight( - getFontString(updatedTextElement), - ); + if (updatedTextElement && isTextElement(updatedTextElement)) { let coordX = updatedTextElement.x; let coordY = updatedTextElement.y; @@ -213,7 +210,7 @@ export const textWysiwyg = ({ if (!isArrowElement(container) && textElementHeight > maxHeight) { const diff = Math.min( textElementHeight - maxHeight, - approxLineHeight, + element.lineHeight, ); mutateElement(container, { height: containerDims.height + diff }); return; @@ -226,7 +223,7 @@ export const textWysiwyg = ({ ) { const diff = Math.min( maxHeight - textElementHeight, - approxLineHeight, + element.lineHeight, ); mutateElement(container, { height: containerDims.height - diff }); } @@ -266,10 +263,6 @@ export const textWysiwyg = ({ editable.selectionEnd = editable.value.length - diff; } - const lines = updatedTextElement.originalText.split("\n"); - const lineHeight = updatedTextElement.containerId - ? approxLineHeight - : updatedTextElement.height / lines.length; if (!container) { maxWidth = (appState.width - 8 - viewportX) / appState.zoom.value; textElementWidth = Math.min(textElementWidth, maxWidth); @@ -282,7 +275,7 @@ export const textWysiwyg = ({ Object.assign(editable.style, { font: getFontString(updatedTextElement), // must be defined *after* font ¯\_(ツ)_/¯ - lineHeight: `${lineHeight}px`, + lineHeight: element.lineHeight, width: `${textElementWidth}px`, height: `${textElementHeight}px`, left: `${viewportX}px`, @@ -388,7 +381,11 @@ export const textWysiwyg = ({ font, getMaxContainerWidth(container!), ); - const { width, height } = measureText(wrappedText, font); + const { width, height } = measureText( + wrappedText, + font, + updatedTextElement.lineHeight, + ); editable.style.width = `${width}px`; editable.style.height = `${height}px`; } diff --git a/src/element/types.ts b/src/element/types.ts index e99b7e89..4b4bad74 100644 --- a/src/element/types.ts +++ b/src/element/types.ts @@ -135,6 +135,11 @@ export type ExcalidrawTextElement = _ExcalidrawElementBase & verticalAlign: VerticalAlign; containerId: ExcalidrawGenericElement["id"] | null; originalText: string; + /** + * Unitless line height (aligned to W3C). To get line height in px, multiply + * with font size (using `getLineHeightInPx` helper). + */ + lineHeight: number & { _brand: "unitlessLineHeight" }; }>; export type ExcalidrawBindableElement = diff --git a/src/renderer/renderElement.ts b/src/renderer/renderElement.ts index e49a1f46..0861315c 100644 --- a/src/renderer/renderElement.ts +++ b/src/renderer/renderElement.ts @@ -40,10 +40,10 @@ import { } from "../constants"; import { getStroke, StrokeOptions } from "perfect-freehand"; import { - getApproxLineHeight, getBoundTextElement, getContainerCoords, getContainerElement, + getLineHeightInPx, getMaxContainerHeight, getMaxContainerWidth, } from "../element/textElement"; @@ -279,9 +279,7 @@ const drawElementOnCanvas = ( // Canvas does not support multiline text by default const lines = element.text.replace(/\r\n?/g, "\n").split("\n"); - const lineHeight = element.containerId - ? getApproxLineHeight(getFontString(element)) - : element.height / lines.length; + const horizontalOffset = element.textAlign === "center" ? element.width / 2 @@ -290,11 +288,16 @@ const drawElementOnCanvas = ( : 0; context.textBaseline = "bottom"; + const lineHeightPx = getLineHeightInPx( + element.fontSize, + element.lineHeight, + ); + for (let index = 0; index < lines.length; index++) { context.fillText( lines[index], horizontalOffset, - (index + 1) * lineHeight, + (index + 1) * lineHeightPx, ); } context.restore(); @@ -1313,7 +1316,10 @@ export const renderElementToSvg = ( }) rotate(${degree} ${cx} ${cy})`, ); const lines = element.text.replace(/\r\n?/g, "\n").split("\n"); - const lineHeight = element.height / lines.length; + const lineHeightPx = getLineHeightInPx( + element.fontSize, + element.lineHeight, + ); const horizontalOffset = element.textAlign === "center" ? element.width / 2 @@ -1331,7 +1337,7 @@ export const renderElementToSvg = ( const text = svgRoot.ownerDocument!.createElementNS(SVG_NS, "text"); text.textContent = lines[i]; text.setAttribute("x", `${horizontalOffset}`); - text.setAttribute("y", `${i * lineHeight}`); + text.setAttribute("y", `${i * lineHeightPx}`); text.setAttribute("font-family", getFontFamilyString(element)); text.setAttribute("font-size", `${element.fontSize}px`); text.setAttribute("fill", element.strokeColor); diff --git a/src/tests/__snapshots__/linearElementEditor.test.tsx.snap b/src/tests/__snapshots__/linearElementEditor.test.tsx.snap index f84eab3a..a2f142b6 100644 --- a/src/tests/__snapshots__/linearElementEditor.test.tsx.snap +++ b/src/tests/__snapshots__/linearElementEditor.test.tsx.snap @@ -5,7 +5,7 @@ exports[`Test Linear Elements Test bound text element should match styles for te class="excalidraw-wysiwyg" data-type="wysiwyg" dir="auto" - style="position: absolute; display: inline-block; min-height: 1em; margin: 0px; padding: 0px; border: 0px; outline: 0; resize: none; background: transparent; overflow: hidden; z-index: var(--zIndex-wysiwyg); word-break: break-word; white-space: pre-wrap; overflow-wrap: break-word; box-sizing: content-box; width: 10.5px; height: 24px; left: 35px; top: 8px; transform: translate(0px, 0px) scale(1) rotate(0deg); text-align: center; vertical-align: middle; color: rgb(0, 0, 0); opacity: 1; filter: var(--theme-filter); max-height: -8px; font: Emoji 20px 20px; line-height: 24px; font-family: Virgil, Segoe UI Emoji;" + style="position: absolute; display: inline-block; min-height: 1em; margin: 0px; padding: 0px; border: 0px; outline: 0; resize: none; background: transparent; overflow: hidden; z-index: var(--zIndex-wysiwyg); word-break: break-word; white-space: pre-wrap; overflow-wrap: break-word; box-sizing: content-box; width: 10.5px; height: 25px; left: 35px; top: 7.5px; transform: translate(0px, 0px) scale(1) rotate(0deg); text-align: center; vertical-align: middle; color: rgb(0, 0, 0); opacity: 1; filter: var(--theme-filter); max-height: -7.5px; font: Emoji 20px 20px; line-height: 1.25; font-family: Virgil, Segoe UI Emoji;" tabindex="0" wrap="off" /> diff --git a/src/tests/clipboard.test.tsx b/src/tests/clipboard.test.tsx index 26fcf4f0..1fdc0f45 100644 --- a/src/tests/clipboard.test.tsx +++ b/src/tests/clipboard.test.tsx @@ -3,8 +3,10 @@ import { render, waitFor, GlobalTestState } from "./test-utils"; import { Pointer, Keyboard } from "./helpers/ui"; import ExcalidrawApp from "../excalidraw-app"; import { KEYS } from "../keys"; -import { getApproxLineHeight } from "../element/textElement"; -import { getFontString } from "../utils"; +import { + getDefaultLineHeight, + getLineHeightInPx, +} from "../element/textElement"; import { getElementBounds } from "../element"; import { NormalizedZoomValue } from "../types"; @@ -118,12 +120,10 @@ describe("paste text as single lines", () => { it("should space items correctly", async () => { const text = "hkhkjhki\njgkjhffjh\njgkjhffjh"; - const lineHeight = - getApproxLineHeight( - getFontString({ - fontSize: h.app.state.currentItemFontSize, - fontFamily: h.app.state.currentItemFontFamily, - }), + const lineHeightPx = + getLineHeightInPx( + h.app.state.currentItemFontSize, + getDefaultLineHeight(h.state.currentItemFontFamily), ) + 10 / h.app.state.zoom.value; mouse.moveTo(100, 100); @@ -135,19 +135,17 @@ describe("paste text as single lines", () => { for (let i = 1; i < h.elements.length; i++) { // eslint-disable-next-line @typescript-eslint/no-unused-vars const [fx, elY] = getElementBounds(h.elements[i]); - expect(elY).toEqual(firstElY + lineHeight * i); + expect(elY).toEqual(firstElY + lineHeightPx * i); } }); }); it("should leave a space for blank new lines", async () => { const text = "hkhkjhki\n\njgkjhffjh"; - const lineHeight = - getApproxLineHeight( - getFontString({ - fontSize: h.app.state.currentItemFontSize, - fontFamily: h.app.state.currentItemFontFamily, - }), + const lineHeightPx = + getLineHeightInPx( + h.app.state.currentItemFontSize, + getDefaultLineHeight(h.state.currentItemFontFamily), ) + 10 / h.app.state.zoom.value; mouse.moveTo(100, 100); @@ -158,7 +156,7 @@ describe("paste text as single lines", () => { const [fx, firstElY] = getElementBounds(h.elements[0]); // eslint-disable-next-line @typescript-eslint/no-unused-vars const [lx, lastElY] = getElementBounds(h.elements[1]); - expect(lastElY).toEqual(firstElY + lineHeight * 2); + expect(lastElY).toEqual(firstElY + lineHeightPx * 2); }); }); }); @@ -224,7 +222,7 @@ describe("Paste bound text container", () => { await sleep(1); expect(h.elements.length).toEqual(2); const container = h.elements[0]; - expect(container.height).toBe(354); + expect(container.height).toBe(368); expect(container.width).toBe(166); }); }); @@ -247,7 +245,7 @@ describe("Paste bound text container", () => { await sleep(1); expect(h.elements.length).toEqual(2); const container = h.elements[0]; - expect(container.height).toBe(740); + expect(container.height).toBe(770); expect(container.width).toBe(166); }); }); diff --git a/src/tests/data/__snapshots__/restore.test.ts.snap b/src/tests/data/__snapshots__/restore.test.ts.snap index b88803cd..e9a0da00 100644 --- a/src/tests/data/__snapshots__/restore.test.ts.snap +++ b/src/tests/data/__snapshots__/restore.test.ts.snap @@ -291,6 +291,7 @@ Object { "height": 100, "id": "id-text01", "isDeleted": false, + "lineHeight": 1.25, "link": null, "locked": false, "opacity": 100, @@ -312,7 +313,7 @@ Object { "verticalAlign": "middle", "width": 100, "x": -20, - "y": -8.4, + "y": -8.75, } `; @@ -329,6 +330,7 @@ Object { "height": 100, "id": "id-text01", "isDeleted": false, + "lineHeight": 1.25, "link": null, "locked": false, "opacity": 100, diff --git a/src/tests/helpers/api.ts b/src/tests/helpers/api.ts index 5cd0943a..bc8bfc8a 100644 --- a/src/tests/helpers/api.ts +++ b/src/tests/helpers/api.ts @@ -181,11 +181,13 @@ export class API { }); break; case "text": + const fontSize = rest.fontSize ?? appState.currentItemFontSize; + const fontFamily = rest.fontFamily ?? appState.currentItemFontFamily; element = newTextElement({ ...base, text: rest.text || "test", - fontSize: rest.fontSize ?? appState.currentItemFontSize, - fontFamily: rest.fontFamily ?? appState.currentItemFontFamily, + fontSize, + fontFamily, textAlign: rest.textAlign ?? appState.currentItemTextAlign, verticalAlign: rest.verticalAlign ?? DEFAULT_VERTICAL_ALIGN, containerId: rest.containerId ?? undefined, diff --git a/src/tests/linearElementEditor.test.tsx b/src/tests/linearElementEditor.test.tsx index 8c1d2932..ac4d801b 100644 --- a/src/tests/linearElementEditor.test.tsx +++ b/src/tests/linearElementEditor.test.tsx @@ -1031,7 +1031,7 @@ describe("Test Linear Elements", () => { expect({ width: container.width, height: container.height }) .toMatchInlineSnapshot(` Object { - "height": 128, + "height": 130, "width": 367, } `); @@ -1040,7 +1040,7 @@ describe("Test Linear Elements", () => { .toMatchInlineSnapshot(` Object { "x": 272, - "y": 46, + "y": 45, } `); expect((h.elements[1] as ExcalidrawTextElementWithContainer).text) @@ -1052,11 +1052,11 @@ describe("Test Linear Elements", () => { .toMatchInlineSnapshot(` Array [ 20, - 36, + 35, 502, - 94, + 95, 205.9061448421403, - 53, + 52.5, ] `); }); @@ -1090,7 +1090,7 @@ describe("Test Linear Elements", () => { expect({ width: container.width, height: container.height }) .toMatchInlineSnapshot(` Object { - "height": 128, + "height": 130, "width": 340, } `); @@ -1099,7 +1099,7 @@ describe("Test Linear Elements", () => { .toMatchInlineSnapshot(` Object { "x": 75, - "y": -4, + "y": -5, } `); expect(textElement.text).toMatchInlineSnapshot(`