Skip to content

Conversation

@Mrazator
Copy link
Member

@Mrazator Mrazator commented Feb 15, 2024

TLDR; Following is the continuation of #7669 moving away from imprecise (integer-based) canvas-text metrics into the relative font metrics encoded within the font itself (read by https://opentype.js.org/font-inspector.html).

Business value:

  • Improved text rendering performance i.e. during resize / undo-redo etc. by completely eliminating expensive baseline calculation (no DOM-base calculation, nor relying on canvas metrics, just simple math with constants).
  • Due to the above, it also improved consistency across browsers, not relying on related DOM inconsistencies and allowing server-side use cases.
  • Removed "bouncing" of text during resize, due to more precise calculation based on relative font metrics.
  • Maintained comparable layout shift as with DOM baseline measurements. In certain scenarios (especially when zoomed-in) it's a bit better, in some other situations its a bit worse (depending on a font & browser).

Technical details:

  • Hardcoded metrics for default fonts
    • We could read the metrics for custom fonts and extend the hardcoded ones in the future
  • Replaced half of the line gap in feat: text measurements based on canvas metrics #7669 with the whole line gap due to a smaller layout shift across the default fonts

Todo:

  • Add changelog
  • Test stability across different browsers
  • Test stability across different OSs
  • Double-check past shift issues in Safari
  • Double-check deprecating the baseline prop
  • Embed a Helvetice-like font with hardcoded metrics - right now using default MacOS Helvetica metrics, which might not work well outside MacOS
    • not needed for now, fallbacking to similar fonts when Helvetica is not present does not result in layout shifts
  • Update SVG export to use alphabetic baseline & vertical offset calculated out of font metrics
    • the hardcoded vertical offset could break exports based on SVG, which do not count with the offset (i.e. PDF export)
  • Remove offset introduced in feat: improve text measurements in bound containers #6187 which causes layout shifts in text containers
  • Remove rounding of font sizes for Safari as it no longer does it

Related issues:

  • Subpixel rendering issues across browsers when resizing small text slowly (line height is bouncing). Turning imageSmoothingEnabled on/off didn't have any effect. Noticeable mostly on lower PPI with devicePixelRatio == 1.
  • Performance bottleneck remains re-generating the element shape on every single resize, which keeps the GPU busy and delays the browser's plans to re-schedule new tasks - results in a lot of dropped frames, even though there is a place for few more tasks, as visible on the screenshot below.
  • Safari does not handle well scaling of textarea when zoomed in, which results in blurry text inside the wysiwyg editor.
  • Safari (16 and older) - during text resize the fonts seem rounded which does not deliver the smoothest experience. Could be related to https://bugs.webkit.org/show_bug.cgi?id=46987 - the fix landed in Safari TP 174 (released july 23), fixed likely since 17.0. (october 23).
  • Text in wysiwyg is often cut (both horizontally and vertically), especially in Virgil, as the glyphs often reach outside our metrics.
  • Selection bounding box vs the text-area bounding box are slightly off.
  • Horizontal shift in some cases, unrelated to the baseline itself feat: text measurements based on font metrics #7693 (comment).

Comparison:

  • When resizing 333 text elements
  • Shows ~75% task time reduction (completely removing force layout)
  • Shows gaps between tasks waiting for GPU to finish (next bottleneck is rendering)
Screenshot 2024-02-16 at 11 06 51
Resizing.mov
Layout.shift.mov

@vercel
Copy link

vercel bot commented Feb 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
excalidraw ✅ Ready (Inspect) Visit Preview Mar 4, 2024 4:18pm
excalidraw-package-example ✅ Ready (Inspect) Visit Preview Mar 4, 2024 4:18pm
excalidraw-package-example-with-nextjs ✅ Ready (Inspect) Visit Preview Mar 4, 2024 4:18pm
1 Ignored Deployment
Name Status Preview Updated (UTC)
docs ⬜️ Ignored (Inspect) Visit Preview Mar 4, 2024 4:18pm

@github-actions
Copy link

github-actions bot commented Feb 15, 2024

Coverage Report

Status Category Percentage Covered / Total
🔴 Lines 64.86% (🎯 70%) 46105 / 71082
🔴 Statements 64.86% (🎯 70%) 46105 / 71082
🔴 Functions 66.14% (🎯 68%) 1397 / 2112
🟢 Branches 80.08% (🎯 70%) 5654 / 7060
File Coverage
File Stmts % Branch % Funcs % Lines Uncovered Lines
Changed Files
packages/excalidraw/actions/actionBoundText.tsx 97.81% 84.61% 100% 97.81% 108-109, 131, 133-134, 238, 240
packages/excalidraw/data/restore.ts 89.9% 83.85% 78.57% 89.9% 83-86, 152, 170-171, 206-208, 238-242, 363, 393-403, 443-444, 453-461, 468-477, 486-495, 513-518
packages/excalidraw/element/newElement.ts 91.97% 90.32% 85% 91.97% 139-144, 147-154, 174-188, 206, 341-342, 450, 456-457, 472-489, 526-527
packages/excalidraw/element/resizeElements.ts 94.07% 85.27% 92.3% 94.07% 169, 182-190, 227-228, 295-299, 306-307, 452-453, 504-505, 601-607, 681-682, 720-721, 830-831, 842-843, 932-941, 985-995
packages/excalidraw/element/textElement.ts 94.7% 95.12% 95% 94.7% 179-180, 220, 362-363, 575-582, 594-615, 629-630, 645-646, 685-692, 790-791, 806-807
packages/excalidraw/element/textWysiwyg.tsx 84.71% 79.34% 81.25% 84.71% 63-64, 66-67, 120-121, 308-338, 363-369, 371-373, 377, 484-486, 500-501, 513-518, 549-550, 558-559, 582, 588-616, 633, 635, 638-643, 670-673
packages/excalidraw/element/types.ts 0% 0% 0% 0% 1-293
packages/excalidraw/renderer/renderElement.ts 83.85% 76.86% 100% 83.85% 83-85, 107-108, 166-167, 171-172, 230-231, 280, 326-327, 340-357, 368-371, 385, 408-409, 411-412, 433, 594, 596-609, 670-672, 700-710, 751-759, 765-766, 770-833, 838-840, 893-895, 917, 920, 947-948
packages/excalidraw/renderer/staticSvgScene.ts 53.11% 59.01% 100% 53.11% 47-48, 65-66, 95-106, 135-138, 149-151, 173-272, 275-367, 369-406, 436, 438-439, 539-541, 556, 558, 568-571, 573-594, 597-599, 613-614, 642-657
Generated in workflow #2033

@Mrazator Mrazator force-pushed the mrazator/font-text-metrics branch from a88b96f to 5192efc Compare February 16, 2024 16:36
@ad1992
Copy link
Member

ad1992 commented Feb 19, 2024

@Mrazator did some testing across browsers and it works very well ✨. The only issue that remains is LS in very High and low zoom levels which is in prod as well hence not a blocker for this PR.

Additionally, can you confirm if all the test cases mentioned in this PR - #6187 (check Testing scenarios) works fine ?

@Mrazator
Copy link
Member Author

@Mrazator did some testing across browsers and it works very well ✨. The only issue that remains is LS in very High and low zoom levels which is in prod as well hence not a blocker for this PR.

@ad1992 I am aware of certain existing issues when zoomed in completely, but not the other way around. For future reference, could you give an example of the mentioned LS when zoomed out completely?

@Mrazator
Copy link
Member Author

Additionally, can you confirm if all the test cases mentioned in this PR - #6187 (check Testing scenarios) works fine ?

Based on the test cases I additionally found a horizontal shift, which is also in prod and unrelated to the baseline.

Screen.Recording.2024-02-26.at.13.01.40.mov

@dwelle
Copy link
Member

dwelle commented Mar 5, 2024

Awesome! Let's see what we break! 🔥

@Mrazator Mrazator merged commit 7e471b5 into master Mar 5, 2024
@Mrazator Mrazator deleted the mrazator/font-text-metrics branch March 5, 2024 19:33
@bric3
Copy link

bric3 commented Oct 1, 2024

Awesome! Let's see what we break! 🔥

Hi there, I'm the author of the IntelliJ Excaldraw plugin (actualluy any Jetbrains IDE based on IJ). In my IDE I'm packaging the latest release 0.17.3, one of our user sketched some documents on excalidraw.com and opened them in the IJ plugin, however the text wasn't showing up, I discovered that the text elements didn't have the baseline property, after a quick search I found this great PR on performance.

However, I believe by removing the baseline it break backward compatibility. Maybe that's expected. I could remediate the situation by upgrading excalidraw, but I'm not sure I should use the trunk version (nor if I can, I don't know if it's available in yarn, as I'm not a frontend dev I lack a lot of knowledge/skills in this area)

@dwelle
Copy link
Member

dwelle commented Oct 1, 2024

Hi. That's right, you'd have to upgrade to latest, which isn't on stable yet, although you can install it from the next tag: yarn add @excalidraw/excalidraw@next.

In my IDE I'm packaging the latest release 0.17.3,

If nothing else, I'd upgrade to 0.17.6 which contains some security fixes.

@bric3
Copy link

bric3 commented Oct 1, 2024

If nothing else, I'd upgrade to 0.17.6 which contains some security fixes.

Interesting I wasn't aware there were newer versions, the last I saw was 0.17.3 in https://github.com/excalidraw/excalidraw/releases.

Hi. That's right, you'd have to upgrade to latest, which isn't on stable yet, although you can install it from the next tag: yarn add @excalidraw/excalidraw@next.

That said I wonder if that's a good idea for the plugin as not everything might be ironed out. Given my poor skills in TS / JS, I'd rather hold off the next stable version hopefully soon (but I know how it is with OSS, when it's ready :) ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants