Add keyboard-accessible tooltip for truncated ActionList.Description#7529
Add keyboard-accessible tooltip for truncated ActionList.Description#7529liuliu-dev wants to merge 23 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: a174e54 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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
bot
added
the
integration-tests: recommended
|
👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
https://github.com/primer/react into liuliu/add-keyboard-accessible-tooltip-for-actionlist
github-actions
bot
requested a deployment
to
storybook-preview-7529
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
liuliu-dev
force-pushed
the
liuliu/add-keyboard-accessible-tooltip-for-actionlist
branch
from
3b5770b to
8afa73c
Compare
TylerJDev
left a comment
TylerJDev
left a comment
There was a problem hiding this comment.
Left a few comments to get a better understanding! Let me know if you have questions or if I can go into more details!
| const truncateEnabled = truncate && !!setTruncatedText | ||
| useResizeObserver( | ||
| () => { | ||
| const el = containerRef.current | ||
| if (!el || !setTruncatedText) return | ||
| setTruncatedText(el.scrollWidth > el.clientWidth ? effectiveTitle : undefined) | ||
| }, | ||
| containerRef, | ||
| [truncateEnabled, effectiveTitle], | ||
| ) | ||
|
|
||
| // check on initial render | ||
| React.useEffect(() => { | ||
| if (!truncateEnabled || !containerRef.current) return | ||
| const el = containerRef.current | ||
| setTruncatedText(el.scrollWidth > el.clientWidth ? effectiveTitle : undefined) | ||
|
|
||
| return () => { | ||
| setTruncatedText(undefined) | ||
| } | ||
| }, [truncateEnabled, effectiveTitle, setTruncatedText]) |
There was a problem hiding this comment.
Is this logic only to determine if content is being truncated or not? Part of me wonders if there's a way we could manage this without an observer? One random thought was "guessing" if content was likely to be truncated based on size of the container + amount of text.
Curious what you think about this @siddharthkp, as you had a lot of good insights with the UnderlineNav work (which is very much unrelated, but comes back to trying to figure out when content is "overflowing" or truncating).
| /** | ||
| * Private API for use internally only. Renders the tooltip popover element before | ||
| * the trigger in the DOM instead of after it. | ||
| * | ||
| * This is used by ActionList items to ensure the tooltip's popover `<span>` does not | ||
| * sit between the ActionListContent and its adjacent SubGroup sibling, which would | ||
| * break CSS adjacent sibling (`+`) selectors used for expand/collapse. | ||
| * | ||
| * @default false | ||
| */ | ||
| _privateRenderBeforeTrigger?: boolean |
There was a problem hiding this comment.
I know we talked about it already, but is this only because of the CSS styling we had for siblings? I wonder if it would be possible to add another style that would apply correctly if truncation was being used.
There was a problem hiding this comment.
Yes! this was due to the sibling CSS. I moved away from changing Tooltip DOM order and instead updated the collapse selectors from adjacent sibling (+) to general sibling (~)
There was a problem hiding this comment.
I wonder if we could get away with the prop and conditional rendering if we adjust the style itself? This might come with its own challenges so I'm curious what you think!
There was a problem hiding this comment.
yeah the prop is removed now and only CSS is used
| useOnEscapePress(() => { | ||
| if (isPopoverOpen) { | ||
| closeTooltip() | ||
| } | ||
| }, [isPopoverOpen]) |
There was a problem hiding this comment.
I see we removed event.stopImmediatePropagation() and event.preventDefault(). I'm wondering if these were causing issues with the Tooltip and ActionList?
There was a problem hiding this comment.
Yes, those two lines were causing Overlay test failure. My understanding is that when Tooltip called preventDefault() and stopImmediatePropagation(), Overlay’s Escape handler could be skipped. I added them back to reproduce the failure: https://github.com/primer/react/actions/runs/22458779284/job/65047083815?pr=7529.
There was a problem hiding this comment.
I'm wondering why that test would fail. I was thinking it was cause it was using ActionList, but I see there's no ActionList usage in the test 🤔 Do we know if the changes are affecting components outside of ActionList?
There was a problem hiding this comment.
My current understanding is:
This is not ActionList related. Both Tooltip and Overlay use useOnEscapePress, and the test failure is exposed by ref composition in Tooltip. Before ref composition, the cloned trigger could drop the child’s original ref. After useMergedRefs, when Tooltip calls preventDefault() / stopImmediatePropagation(), it can block Overlay’s handler in the shared useOnEscapePress chain.
| <TooltipContext.Provider value={value}> | ||
| <> | ||
| {_privateRenderBeforeTrigger ? ( | ||
| <> | ||
| {tooltipPopover} | ||
| {triggerElement} | ||
| </> | ||
| ) : ( | ||
| <> | ||
| {triggerElement} | ||
| {tooltipPopover} | ||
| </> | ||
| )} | ||
| </> | ||
| </TooltipContext.Provider> |
There was a problem hiding this comment.
Remind me once more why we need the tooltip before the element in this case? Is this related to https://github.com/primer/react/pull/7529/changes#r2860941893?
There was a problem hiding this comment.
Not anymore with the CSS selector update.
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/14694 |
Related issue https://github.com/github/accessibility-audits/issues/14802
Changelog
When
ActionList.Descriptionis rendered withtruncate, the full text was only visible via a nativetitleattribute — which only appears on mouse hover, not keyboard focus (WCAG SC 2.1.1).This PR adds a Primer
<Tooltip>at theActionList.Itemlevel that shows the full description text on both hover and keyboard focus when the description is actually truncated.Rollout strategy
Testing & Reviewing
Merge checklist