Skip to content

Add aria-describedby automatically #1257

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 11, 2025
Merged

Add aria-describedby automatically #1257

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8878931
Add aria-describedby automatically
chardin1 Sep 17, 2025
37cab51
Minor adjustments
chardin1 Sep 17, 2025
c208987
Add cleanup function
chardin1 Sep 17, 2025
970f7ca
test: update snapshots
gabrieljablonski Oct 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,15 @@ function App() {
>
My button
</button>
<Tooltip place="bottom" anchorId={anchorId} isOpen={isDarkOpen} setIsOpen={setIsDarkOpen} />
<Tooltip
id="button1"
place="bottom"
anchorId={anchorId}
isOpen={isDarkOpen}
setIsOpen={setIsDarkOpen}
/>
<Tooltip
id="button2"
place="top"
variant="success"
anchorId="button2"
Expand Down Expand Up @@ -101,6 +108,7 @@ function App() {
<Tooltip id="anchor-select">Tooltip content</Tooltip>
<Tooltip
ref={tooltipRef}
id="tooltip-content"
anchorSelect="section[id='section-anchor-select'] > p > button"
place="bottom"
openEvents={{ click: true }}
Expand All @@ -127,6 +135,7 @@ function App() {
</div>
<Tooltip
anchorId="floatAnchor"
id="float-tooltip"
content={
toggle
? 'This is a float tooltip with a very very large content string'
Expand All @@ -151,6 +160,7 @@ function App() {
</div>
<Tooltip
anchorId="onClickAnchor"
id="onclick-tooltip"
content={`This is an on click tooltip (x:${position.x},y:${position.y})`}
events={['click']}
position={position}
Expand Down
30 changes: 30 additions & 0 deletions src/components/Tooltip/Tooltip.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ const Tooltip = ({
isOpen,
defaultIsOpen = false,
setIsOpen,
previousActiveAnchor,
activeAnchor,
setActiveAnchor,
border,
Expand Down Expand Up @@ -205,6 +206,35 @@ const Tooltip = ({
}, 10)
}

/**
* 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])

Comment on lines 209 to 244
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add cleanup and harden aria-describedby token handling.

  • Missing cleanup can leave stale aria-describedby on anchors (especially if id changes 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.

/**
* this replicates the effect from `handleShow()`
* when `isOpen` is changed from outside
Expand Down
1 change: 1 addition & 0 deletions src/components/Tooltip/TooltipTypes.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ export interface ITooltip {
afterShow?: () => void
afterHide?: () => void
disableTooltip?: (anchorRef: HTMLElement | null) => boolean
previousActiveAnchor: HTMLElement | null
Copy link

@coderabbitai coderabbitai bot Sep 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do agree, @chardin1 can you please check?

Also @gabrieljablonski can you please let me know your thoughts about that?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine to keep, since we already "expose" internals with activeAnchor and setActiveAnchor below.

Maybe just mark it down so we don't forget to move this to an internal interface later?

activeAnchor: HTMLElement | null
setActiveAnchor: (anchor: HTMLElement | null) => void
border?: CSSProperties['border']
Expand Down
11 changes: 10 additions & 1 deletion src/components/TooltipController/TooltipController.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ const TooltipController = React.forwardRef<TooltipRefProps, ITooltipController>(
const [tooltipPositionStrategy, setTooltipPositionStrategy] = useState(positionStrategy)
const [tooltipClassName, setTooltipClassName] = useState<string | null>(null)
const [activeAnchor, setActiveAnchor] = useState<HTMLElement | null>(null)
const previousActiveAnchorRef = useRef<HTMLElement | null>(null)
const styleInjectionRef = useRef(disableStyleInjection)
/**
* @todo Remove this in a future version (provider/wrapper method is deprecated)
Expand Down Expand Up @@ -375,7 +376,15 @@ const TooltipController = React.forwardRef<TooltipRefProps, ITooltipController>(
afterHide,
disableTooltip,
activeAnchor,
setActiveAnchor: (anchor: HTMLElement | null) => setActiveAnchor(anchor),
previousActiveAnchor: previousActiveAnchorRef.current,
setActiveAnchor: (anchor: HTMLElement | null) => {
setActiveAnchor((prev) => {
if (!anchor?.isSameNode(prev)) {
previousActiveAnchorRef.current = prev
}
return anchor
})
},
role,
}

Expand Down
3 changes: 3 additions & 0 deletions src/test/__snapshots__/tooltip-attributes.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
exports[`tooltip attributes basic tooltip 1`] = `
<div>
<span
aria-describedby="basic-example-attr"
data-tooltip-content="Hello World!"
data-tooltip-id="basic-example-attr"
>
Expand All @@ -26,6 +27,7 @@ exports[`tooltip attributes basic tooltip 1`] = `
exports[`tooltip attributes tooltip with class name 1`] = `
<div>
<span
aria-describedby="example-class-name-attr"
data-tooltip-class-name="tooltip-class-name"
data-tooltip-content="Hello World!"
data-tooltip-id="example-class-name-attr"
Expand All @@ -50,6 +52,7 @@ exports[`tooltip attributes tooltip with class name 1`] = `
exports[`tooltip attributes tooltip with place 1`] = `
<div>
<span
aria-describedby="example-place-attr"
data-tooltip-content="Hello World!"
data-tooltip-id="example-place-attr"
data-tooltip-place="right"
Expand Down
8 changes: 8 additions & 0 deletions src/test/__snapshots__/tooltip-props.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
exports[`tooltip props basic tooltip 1`] = `
<div>
<span
aria-describedby="basic-example"
data-tooltip-id="basic-example"
>
Lorem Ipsum
Expand All @@ -25,6 +26,7 @@ exports[`tooltip props basic tooltip 1`] = `
exports[`tooltip props clickable tooltip 1`] = `
<div>
<span
aria-describedby="example-clickable"
data-tooltip-id="example-clickable"
>
Lorem Ipsum
Expand All @@ -49,6 +51,7 @@ exports[`tooltip props clickable tooltip 1`] = `
exports[`tooltip props tooltip with custom position 1`] = `
<div>
<span
aria-describedby="example-place"
data-tooltip-id="example-place"
>
Lorem Ipsum
Expand Down Expand Up @@ -81,6 +84,7 @@ exports[`tooltip props tooltip with delay hide 1`] = `
exports[`tooltip props tooltip with delay show 1`] = `
<div>
<span
aria-describedby="example-delay-show"
data-tooltip-id="example-delay-show"
>
Lorem Ipsum
Expand All @@ -103,6 +107,7 @@ exports[`tooltip props tooltip with delay show 1`] = `
exports[`tooltip props tooltip with disableTooltip return false 1`] = `
<div>
<span
aria-describedby="example-disableTooltip-false"
data-tooltip-id="example-disableTooltip-false"
>
Lorem Ipsum
Expand All @@ -125,6 +130,7 @@ exports[`tooltip props tooltip with disableTooltip return false 1`] = `
exports[`tooltip props tooltip with float 1`] = `
<div>
<span
aria-describedby="example-float"
data-tooltip-id="example-float"
>
Lorem Ipsum
Expand All @@ -147,6 +153,7 @@ exports[`tooltip props tooltip with float 1`] = `
exports[`tooltip props tooltip with html 1`] = `
<div>
<span
aria-describedby="example-html"
data-tooltip-id="example-html"
>
Lorem Ipsum
Expand Down Expand Up @@ -174,6 +181,7 @@ exports[`tooltip props tooltip with html 1`] = `
exports[`tooltip props tooltip with place 1`] = `
<div>
<span
aria-describedby="example-place"
data-tooltip-id="example-place"
>
Lorem Ipsum
Expand Down