Fix ellipsis truncation in Token

[iansan5653]

Text in Token is supposed to truncate with an ellipsis when the Token reaches 100% of the container width. But the TokenTextContainer has overflow: visible, which disables truncation. Removing that rule allows the preceding overflow: hidden line to activate.

However, in inline elements overflow cannot be hidden only in the x direction, so overflow: hidden will also clip descenders (like the tail on a "g"). To fix this, we also need to also change the line-height to normal.

Before:

After:

[changeset-bot]

🦋 Changeset detected

Latest commit: b9f3a69

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	94.65 KB (-0.01% 🔽)
dist/browser.umd.js	95.22 KB (-0.01% 🔽)

[joshblack]

@iansan5653 this might be out of scope for this PR, but from an a11y perspective would we need to provide a mechanism for someone to view the full label contents if it is truncated? Otherwise could this fall under: https://www.w3.org/TR/WCAG21/#resize-text

[iansan5653]

I totally agree. That being said I'm not sure what a good answer is in this case:

• We could encourage shorter text, but sometimes the text is user-controlled (ie in issue labels)
• We could allow the token to break apart across lines, but I have a feeling designers really won't like the look of that
• We could allow the text inside the token to wrap, but that would cause Token to have a dynamic height. One of the primary purposes of Token is to work with other tokens in the same group, so uniformity in height is really important
• We could show the text in a Tooltip on hover/focus. But what about when the Token is a non-focusable span? Should it always be focusable? Do we have a mechanism for showing a tooltip without duplicating the content for screen readers (ie, an aria-hidden tooltip)? Our Tooltip component is generally not accessible anyway, so that starts raising even more problems. Tokens on project board cards also already have tooltips, so there is also a conflict there. And how do we know when to show the tooltip? If we do it when there is no overflow, it will look buggy, but there is no API to detect text overflow. All kinds of concerns here unfortunately
• We could show the full text in a dialog on click. This also raises focus issues, and since Token is already clickable in some cases (renderable as a button with onClick), this will conflict
• We could make the text horizontally scrollable. This is another accessibility violation but maybe one that isn't as bad? But there is no CSS for a scrollable ellipsis overflow. And we risk showing horizontal scrollbar inside the token, which again breaks that height consistency
• We could put the full text in a title attribute, but this only fixes the problem for mouse users since title only renders on hover

I don't think this fix makes the accessibility problem worse - the current situation with visual overflow is a bug that does need to be fixed. Generally I think the first option (encourage shorter text) is preferred. When user-controlled though, we do have to do something.

[joshblack]

@iansan5653 thanks for taking the time to write this up! I appreciate it so much 🙏 And definitely agreed with what you shared around.

I'm not sure how much we could build-in to this component due to the different circumstances you shared, but what would you think of this as a rule of thumb:

1. If you can avoid truncation, please do that instead
2. If you need to truncate, use a tooltip
3. If the content that is being truncated is interactive, place the tooltip on the interactive element
4. If the content that is being truncated is non-interactive, use a toggletip

[iansan5653]

Yep, that sounds about right to me 👍. That seems like something we could use an interface guideline document for.

In the meantime, are you OK with merging this PR?

[joshblack]

@iansan5653 definitely 👍 it seems like this is similar to some other components so I'll make a note of this under the umbrella of truncation so we can revisit it!