fix(Shuffle): clip wrapper is shorter than the glyph, cutting descenders - #1051
Open
Baltsar wants to merge 1 commit into
Open
fix(Shuffle): clip wrapper is shorter than the glyph, cutting descenders#1051Baltsar wants to merge 1 commit into
Baltsar wants to merge 1 commit into
Conversation
The clip wrapper is sized from the char box (Shuffle.jsx: `ch.getBoundingClientRect().height`), and `.shuffle-char` pins that box to `line-height: 1`. Every typeface has a content area taller than 1em, so the mask is shorter than the glyph and the bottom of g/j/p/q/y is cut off. Measured at font-size 64px, clip box vs content: Outfit 64px vs 72px -> 8px cut Playfair Display 64px vs 75px -> 11px cut Poppins 64px vs 77px -> 13px cut Invisible in the demo because the default face is Press Start 2P with text-transform: uppercase, and uppercase Latin has no descenders. Setting the char's own line-height to `normal` resolves it to the font's ascent + descent, so the wrapper is sized correctly per face. Line spacing stays with .shuffle-parent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
Shuffle.jsxsizes the clip wrapper from the character box:and
Shuffle.csspins that box with.shuffle-char { line-height: 1 }.Every typeface has a content area taller than
1em, so theoverflow: hiddenwrapper ends up shorter than the glyph it is meant to mask, and the bottom ofg j p q yis cut off.Measured in the browser at
font-size: 64px, wrapperclientHeightvsscrollHeight:It does not show up in the demo because the default face is Press Start 2P with
text-transform: uppercase, and uppercase Latin has no descenders. It appears as soon as someone changes the font or drops the uppercase — which is the normal thing to do with a copy-paste component.The fix
Give the character its own
line-height: normal, which resolves to the font's ascent + descent, so the wrapper is sized correctly per face. Line spacing stays where it was, on.shuffle-parent.content/andts-default/:.shuffle-char { line-height: normal }tailwind/andts-tailwind/: those variants ship no stylesheet, so the char inheritsleading-nonefrom the parent. Addedleading-normaltocharsClassinstead.After, same measurement:
Trade-off you should weigh
This makes the boxes taller. At 64px, Outfit goes 64 → 81px and Poppins 64 → 96px.
line-height: normalincludes the font's line gap, so it is a little more generous than strictly required. That changes the vertical rhythm for anyone already using the component, which is a real cost even though the current rendering is wrong.If you would rather keep the tight rhythm, the exact alternative is to size the wrapper from the real metrics instead of the char box:
That gives the minimum correct height with no line-gap slack, at the cost of a canvas measurement per face. Happy to switch this PR to that version if you prefer it — or to close it if you would rather solve it your own way.
I did not touch
public/r/*.json, since those arejsrepo buildoutput.How this was found, honestly
This is an AI-assisted contribution. I was building something similar — a masked text-animation generator — and hit exactly this class of bug in my own code: the mask box has to be derived from font metrics, not from a fixed line-height, or descenders get clipped the moment the typeface changes. While working through it with Claude I went looking for whether the same assumption existed elsewhere, found it here, and verified it in the browser.
Everything above is measured, not inferred. But two caveats worth stating plainly: I have not run your visual regression setup or checked the Shuffle demo page after the change, and I have not tested the
left/rightshuffle directions where the wrapper height isautoand the bug does not apply. Treat the numbers as solid and the visual sign-off as still owed.