Add aria-describedby automatically #1257
Open
Conversation
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
WalkthroughAdds multiple identified Tooltips and anchorSelect-based anchoring in App; extends Tooltip API (id, anchorSelect, event controls) and introduces previousActiveAnchor tracking with an effect that manages aria-describedby on anchors when tooltips show/hide. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant App as App.tsx
participant TC as TooltipController
participant TT as Tooltip
participant A as ActiveAnchor (HTMLElement)
participant PA as PreviousAnchor (HTMLElement)
U->>App: Trigger open/close (click/hover)
App->>TC: setActiveAnchor(newAnchor)
alt anchor changed
TC->>TC: previousActiveAnchorRef = activeAnchor
TC->>TC: activeAnchor = newAnchor
end
TC->>TT: render({ activeAnchor, previousActiveAnchor, id, anchorSelect, openEvents, closeEvents })
rect rgba(200,230,255,0.25)
note right of TT: on show -> manage aria-describedby
TT->>PA: remove tooltip id from aria-describedby (if present)
TT->>A: add tooltip id to aria-describedby (deduped)
end
U->>TT: close event (per closeEvents/globalCloseEvents)
TT->>A: remove tooltip id from aria-describedby
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Tooltip/Tooltip.tsx (1)
66-74: DefaultpreviousActiveAnchortonullto keep prop optional.Pairs with making it optional in types; preserves backward compatibility for direct
<Tooltip />use.- previousActiveAnchor, + previousActiveAnchor = null,
🧹 Nitpick comments (4)
src/components/TooltipController/TooltipController.tsx (1)
379-385: Use functional state update to avoid staleactiveAnchorin closure.Ensures
previousActiveAnchorRefalways sees the true previous anchor.- setActiveAnchor: (anchor: HTMLElement | null) => { - if (!anchor?.isSameNode(activeAnchor)) { - previousActiveAnchorRef.current = activeAnchor - } - setActiveAnchor(anchor) - }, + setActiveAnchor: (anchor: HTMLElement | null) => { + setActiveAnchor((prev) => { + if (!anchor?.isSameNode(prev)) { + previousActiveAnchorRef.current = prev + } + return anchor + }) + },src/components/Tooltip/Tooltip.tsx (1)
209-236: Aria cleanup: includeidin deps and add unmount cleanup.
- Missing
idin deps can leave stale tokens on id changes.- Add cleanup on unmount/hide edge-cases.
- useEffect(() => { + useEffect(() => { if (!id) return @@ - if (show) { + if (show) { removeAriaDescribedBy(previousActiveAnchor) const currentDescribedBy = getAriaDescribedBy(activeAnchor) const describedBy = [...new Set([...currentDescribedBy, id])].filter(Boolean).join(' ') activeAnchor?.setAttribute('aria-describedby', describedBy) } else { removeAriaDescribedBy(activeAnchor) } - }, [activeAnchor, show]) + return () => { + // best-effort cleanup on unmount or dependency change + removeAriaDescribedBy(activeAnchor) + } + }, [activeAnchor, show, id, previousActiveAnchor])Optional hardening: split on
/\s+/to normalize whitespace.src/App.tsx (2)
58-64: Avoid conflicting manualaria-describedbyon the anchor.Since the Tooltip now manages it, remove the hardcoded
aria-describedby="tooltip"on Line 44 to prevent stale/duplicate tokens.Outside this hunk, apply:
- aria-describedby="tooltip"
3-5: Import types from the module, not the .d file.Avoid coupling to the declaration filename; let TS resolve
.d.ts.Outside this hunk, apply:
-import { IPosition, TooltipRefProps } from 'components/Tooltip/TooltipTypes.d' +import { IPosition, TooltipRefProps } from 'components/Tooltip/TooltipTypes'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/test/__snapshots__/tooltip-attributes.spec.js.snapis excluded by!**/*.snapsrc/test/__snapshots__/tooltip-props.spec.js.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
src/App.tsx(4 hunks)src/components/Tooltip/Tooltip.tsx(2 hunks)src/components/Tooltip/TooltipTypes.d.ts(1 hunks)src/components/TooltipController/TooltipController.tsx(2 hunks)
🔇 Additional comments (5)
src/components/TooltipController/TooltipController.tsx (1)
84-84: OK: local ref for previous active anchor.Ref-based tracking makes sense and avoids extra renders.
src/App.tsx (4)
51-56: Good: assign explicitidto enable aria-describedby automation.Looks correct with controller wiring.
111-117: LGTM: anchorSelect demo with id and event maps.Matches the new API and will manage describedby automatically.
139-145: LGTM: id on float tooltip ensures unique describedby token.All good.
164-168: LGTM: click-positioned tooltip with stableid.Consistent with the new behavior.
| afterShow?: () => void | ||
| afterHide?: () => void | ||
| disableTooltip?: (anchorRef: HTMLElement | null) => boolean | ||
| previousActiveAnchor: HTMLElement | null |
There was a problem hiding this comment.
Don’t make controller-only prop public-required; mark optional and internal.
Requiring previousActiveAnchor on ITooltip breaks direct <Tooltip /> usage and leaks internals. Make it optional and annotate as internal.
Apply:
- previousActiveAnchor: HTMLElement | null
+ /** @internal Controlled by TooltipController. Do not pass manually. */
+ previousActiveAnchor?: HTMLElement | null📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| previousActiveAnchor: HTMLElement | null | |
| /** @internal Controlled by TooltipController. Do not pass manually. */ | |
| previousActiveAnchor?: HTMLElement | null |
🤖 Prompt for AI Agents
In src/components/Tooltip/TooltipTypes.d.ts around line 156, the ITooltip
property previousActiveAnchor is currently required and exposes controller
internals; change its type to be optional (previousActiveAnchor?: HTMLElement |
null) and annotate it as internal (e.g., add an inline comment or JSDoc like /**
@internal */) so it is no longer a public-required prop while preserving type
compatibility for internal controller usage.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/Tooltip/Tooltip.tsx (1)
66-74: IspreviousActiveAnchorintended to be public? If not, hide it.Surfacing this controller-only prop on the public
ITooltipprops leaks internal wiring. If external, document it; otherwise, mark it@internaland exclude from the published.d.ts, or pass via context only.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Tooltip/Tooltip.tsx(2 hunks)src/components/TooltipController/TooltipController.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/TooltipController/TooltipController.tsx
Comment on lines
209
to
237
| /** | ||
| * Add aria-describedby to activeAnchor when tooltip is active | ||
| */ | ||
| useEffect(() => { | ||
| if (!id) return | ||
|
|
||
| function getAriaDescribedBy(element: HTMLElement | null) { | ||
| return element?.getAttribute('aria-describedby')?.split(' ') || [] | ||
| } | ||
|
|
||
| function removeAriaDescribedBy(element: HTMLElement | null) { | ||
| const newDescribedBy = getAriaDescribedBy(element).filter((s) => s !== id) | ||
| if (newDescribedBy.length) { | ||
| element?.setAttribute('aria-describedby', newDescribedBy.join(' ')) | ||
| } else { | ||
| element?.removeAttribute('aria-describedby') | ||
| } | ||
| } | ||
|
|
||
| if (show) { | ||
| removeAriaDescribedBy(previousActiveAnchor) | ||
| const currentDescribedBy = getAriaDescribedBy(activeAnchor) | ||
| const describedBy = [...new Set([...currentDescribedBy, id])].filter(Boolean).join(' ') | ||
| activeAnchor?.setAttribute('aria-describedby', describedBy) | ||
| } else { | ||
| removeAriaDescribedBy(activeAnchor) | ||
| } | ||
| }, [activeAnchor, show, id, previousActiveAnchor]) | ||
|
|
There was a problem hiding this comment.
Add cleanup and harden aria-describedby token handling.
- Missing cleanup can leave stale
aria-describedbyon anchors (especially ifidchanges or Tooltip unmounts while visible). .split(' ')mishandles tabs/newlines and multiple spaces; normalize with/\s+/and filter empties.- Skip unnecessary DOM writes and avoid removing from the same node as current.
Apply:
/**
* Add aria-describedby to activeAnchor when tooltip is active
*/
useEffect(() => {
if (!id) return
- function getAriaDescribedBy(element: HTMLElement | null) {
- return element?.getAttribute('aria-describedby')?.split(' ') || []
- }
+ function getAriaDescribedBy(element: HTMLElement | null) {
+ const raw = element?.getAttribute('aria-describedby') ?? ''
+ return raw.trim().split(/\s+/).filter(Boolean)
+ }
- function removeAriaDescribedBy(element: HTMLElement | null) {
- const newDescribedBy = getAriaDescribedBy(element).filter((s) => s !== id)
- if (newDescribedBy.length) {
- element?.setAttribute('aria-describedby', newDescribedBy.join(' '))
- } else {
- element?.removeAttribute('aria-describedby')
- }
- }
+ function removeAriaDescribedBy(element: HTMLElement | null) {
+ if (!element) return
+ const newDescribedBy = getAriaDescribedBy(element).filter((s) => s !== id)
+ if (newDescribedBy.length) {
+ const next = newDescribedBy.join(' ')
+ if (element.getAttribute('aria-describedby') !== next) {
+ element.setAttribute('aria-describedby', next)
+ }
+ } else {
+ element.removeAttribute('aria-describedby')
+ }
+ }
if (show) {
- removeAriaDescribedBy(previousActiveAnchor)
+ if (previousActiveAnchor && activeAnchor && !previousActiveAnchor.isSameNode(activeAnchor)) {
+ removeAriaDescribedBy(previousActiveAnchor)
+ }
const currentDescribedBy = getAriaDescribedBy(activeAnchor)
- const describedBy = [...new Set([...currentDescribedBy, id])].filter(Boolean).join(' ')
- activeAnchor?.setAttribute('aria-describedby', describedBy)
+ const describedBy = [...new Set([...currentDescribedBy, id])].filter(Boolean).join(' ')
+ if (activeAnchor && activeAnchor.getAttribute('aria-describedby') !== describedBy) {
+ activeAnchor.setAttribute('aria-describedby', describedBy)
+ }
} else {
removeAriaDescribedBy(activeAnchor)
}
- }, [activeAnchor, show, id, previousActiveAnchor])
+ return () => {
+ // Cleanup on id/anchor changes and unmount to avoid stale ARIA refs.
+ removeAriaDescribedBy(activeAnchor)
+ removeAriaDescribedBy(previousActiveAnchor)
+ }
+ }, [activeAnchor, show, id, previousActiveAnchor])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Add aria-describedby to activeAnchor when tooltip is active | |
| */ | |
| useEffect(() => { | |
| if (!id) return | |
| function getAriaDescribedBy(element: HTMLElement | null) { | |
| return element?.getAttribute('aria-describedby')?.split(' ') || [] | |
| } | |
| function removeAriaDescribedBy(element: HTMLElement | null) { | |
| const newDescribedBy = getAriaDescribedBy(element).filter((s) => s !== id) | |
| if (newDescribedBy.length) { | |
| element?.setAttribute('aria-describedby', newDescribedBy.join(' ')) | |
| } else { | |
| element?.removeAttribute('aria-describedby') | |
| } | |
| } | |
| if (show) { | |
| removeAriaDescribedBy(previousActiveAnchor) | |
| const currentDescribedBy = getAriaDescribedBy(activeAnchor) | |
| const describedBy = [...new Set([...currentDescribedBy, id])].filter(Boolean).join(' ') | |
| activeAnchor?.setAttribute('aria-describedby', describedBy) | |
| } else { | |
| removeAriaDescribedBy(activeAnchor) | |
| } | |
| }, [activeAnchor, show, id, previousActiveAnchor]) | |
| /** | |
| * Add aria-describedby to activeAnchor when tooltip is active | |
| */ | |
| useEffect(() => { | |
| if (!id) return | |
| function getAriaDescribedBy(element: HTMLElement | null) { | |
| const raw = element?.getAttribute('aria-describedby') ?? '' | |
| return raw.trim().split(/\s+/).filter(Boolean) | |
| } | |
| function removeAriaDescribedBy(element: HTMLElement | null) { | |
| if (!element) return | |
| const newDescribedBy = getAriaDescribedBy(element).filter((s) => s !== id) | |
| if (newDescribedBy.length) { | |
| const next = newDescribedBy.join(' ') | |
| if (element.getAttribute('aria-describedby') !== next) { | |
| element.setAttribute('aria-describedby', next) | |
| } | |
| } else { | |
| element.removeAttribute('aria-describedby') | |
| } | |
| } | |
| if (show) { | |
| if (previousActiveAnchor && activeAnchor && !previousActiveAnchor.isSameNode(activeAnchor)) { | |
| removeAriaDescribedBy(previousActiveAnchor) | |
| } | |
| const currentDescribedBy = getAriaDescribedBy(activeAnchor) | |
| const describedBy = [...new Set([...currentDescribedBy, id])].filter(Boolean).join(' ') | |
| if (activeAnchor && activeAnchor.getAttribute('aria-describedby') !== describedBy) { | |
| activeAnchor.setAttribute('aria-describedby', describedBy) | |
| } | |
| } else { | |
| removeAriaDescribedBy(activeAnchor) | |
| } | |
| return () => { | |
| // Cleanup on id/anchor changes and unmount to avoid stale ARIA refs. | |
| removeAriaDescribedBy(activeAnchor) | |
| removeAriaDescribedBy(previousActiveAnchor) | |
| } | |
| }, [activeAnchor, show, id, previousActiveAnchor]) |
🤖 Prompt for AI Agents
In src/components/Tooltip/Tooltip.tsx around lines 209-237, the useEffect that
manages aria-describedby needs robust token handling and cleanup: change
splitting to use .split(/\s+/).filter(Boolean) to normalize whitespace and
remove empty tokens; when adding/removing ensure you skip DOM writes if the
computed attribute would be unchanged; avoid removing the id from the same node
that you’re about to set on (i.e., only remove from previousActiveAnchor when
previousActiveAnchor !== activeAnchor); and add a cleanup function returned from
the effect that removes the current id from activeAnchor and
previousActiveAnchor when the component unmounts or id changes so no stale
aria-describedby remains. Ensure all string joins use ' ' and all DOM
set/removeAttribute calls are guarded by checks that the attribute actually
needs to change.
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.
Automatically applies
aria-describedbyto the active anchor when the tooltip is visible, but only if anidhas been provided to<Tooltip />Summary by CodeRabbit
New Features
Accessibility