feat: replace render-time text wrapping with browser-native wrapping#191
feat: replace render-time text wrapping with browser-native wrapping#191hyperfinitism wants to merge 1 commit intostats-organization:masterfrom
Conversation
|
@hyperfinitism is attempting to deploy a commit to the martin-mfg's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
martin-mfg
left a comment
There was a problem hiding this comment.
Thanks for the PR!
Please ignore the failing "Backend E2E test" pipeline. It determined that this PR generates different output than the master branch, which is expected here.
Currently the changes in this PR lead to visual changes in generated cards regardless of line wrapping. Especially the line height is increased. Could you please ensure there are no visual changes apart from the line wrapping? Maybe it makes sense to simply copy the HTML code which GitHub itself uses in the original repo pins on user profiles?
Doing this would also raise the question if it makes sense to use foreignObject for the whole content of gist/pin cards. I.e. not only the description, but also the title, star count, etc. What do you think?
| : "" | ||
| } | ||
|
|
||
| <text class="description" x="${X_OFFSET}" y="-5"> |
There was a problem hiding this comment.
Does removing the "-5" here not change the layout of the final card?
There was a problem hiding this comment.
Strictly speaking it does; the new position depends on the browser's HTML/font metrics rather than the SVG baseline, so the first-line baseline can drift by about < 1px. In practice the visual top of the description lines up at the same place and the appearance is nearly identical.
| return 1; | ||
| } | ||
|
|
||
| const spaceWidth = measureText(" ", fontSize); |
There was a problem hiding this comment.
Currently the code assumes that only singular, normal whitespaces are used. Could you please make it work with arbitrary whitespace?
You can use e.g. this repo description for testing:
"One two three four five six seven eight nine ten."
It contains different whitespace characters of different widths and I chose the length so that it actually wraps to 2 lines, but the current logic determines this will only be 1 line.
There was a problem hiding this comment.
Done. countWrappedLines now tokenizes into alternate words/whitespaces and measures each whitespace at its rendered width (ASCII whitespace collapsed to a single space per CSS white-space: normal; non-ASCII whitespace preserved, with U+3000 special-cased to 1 em since the width table is ASCII-only).
This is still an approximation: measureText reads from a fixed ASCII-only width table for a specific font and ignores kerning and other factors. More precise widths for all characters would require hard-coding a large (char, width) table, which felt out of scope. The estimate may still be off by a few percent for non-ASCII-heavy text, but it's used only to size the SVG; the browser does the real wrap inside the foreignObject.
f9af708 to
0c6113b
Compare
Use `foreignObject` with CSS line-clamp to let the browser handle text wrapping natively instead of manually wrapping on the server. This provides better font-aware wrapping while keeping server-side line estimation for SVG height calculation. Signed-off-by: Takuma IMAMURA <209989118+hyperfinitism@users.noreply.github.com>
0c6113b to
7da1078
Compare
While this approach maintains the greatest visual consistency, it has the following problems:
|
This PR uses
foreignObjectwith CSS line-clamp to let the browser handle text wrapping natively instead of manually wrapping on the server. This provides better font-aware wrapping while keeping server-side line estimation for SVG height calculation.Fixes anuraghazra#4862.
Example card