Review 5419

#5419 — fix(HoverCard): expose rich cards as dialog popups, not descriptions

gonzoblasco (contributor, fork) · OPEN · view on GitHub

Verdict: request changes

Head reviewed: 9647b756f0d178ebd6ef77a79685ffdc8c5b7011. No prior reviews; the only prior discussion is on #5049, where this author proposed the contract himself and no maintainer has ruled.

Problem

A screen-reader user focuses a HoverCard trigger with a labelled card and is told the card's name as a description — "Jane Doe, button, Profile actions" — and never told a popup exists or that it opened. Measured on main: description "Profile actions", no hasPopup, no expanded.

Solution

With label, the trigger advertises the popup instead of describing itself with it: aria-haspopup="dialog" + aria-controls + aria-expanded. useHoverCard() gains id and isOpen; describedBy stays as a deprecated alias. The unlabelled role="group" path is deliberately unchanged.

Impact

Only consumers who pass label — and nothing in this repo passes it, not one of the eight stories, not the CLI templates. So the effect on merge is nil, and the case the issue reproduces as content-flattening still describes the trigger with the card's contents. Labelled and unlabelled hover cards now disagree about how they expose themselves.

API

+ HoverCardReturn.id: string        (public, core barrel via HoverCard/index.ts)
+ HoverCardReturn.isOpen: boolean   (public, same)
~ HoverCardReturn.describedBy       (public; documented deprecated, no @deprecated tag)

Theme targets

No new theme targets.

Ossification

The class is landed, not speculative: usePopover already returns isOpen (usePopover.tsx:255) and id (:260), and DateInput/DateRangeInput/DateTimeInput consume exactly that pair. Same names, same job — the new surface is fine and is not what carries the verdict.

Breaking

• API — no, additive; describedBy retained.
• Visual — no; attributes and a hook return only, frames identical.
• Theme — no.

Performance & resources

Three extra setAttribute calls per render on a labelled card. The layout effect already re-ran every render — useLayer returns a fresh object literal (useLayer.tsx:880) — which is pre-existing and not this author's, but worth its own issue.

Risk class

Needs judgement — new public surface on a hook return, and an a11y contract change.

Judgement

request changes

1. The hook block template puts aria-haspopup="dialog" on a trigger whose card has
   no label
   → in Chrome the button announces "has dialog popup" and opens an unnamed
     role="group" — the exact mismatch the PR's own description rules out
                        · HoverCardHookUsage.tsx:23

2. The labelled branch OVERWRITES the trigger's own aria-haspopup, aria-controls
   and aria-expanded, and removes them on cleanup
   → wrap a menu button in a labelled HoverCard and its menu semantics are gone
     with nothing saying so. Measured: aria-haspopup="menu" → dialog,
     aria-controls="my-menu" → the card. The no-label branch fifteen lines down
     merges and restores instead                        · HoverCard.tsx:267

3. [not blocking] aria-controls is not gated on open
   → while closed it names an id that is not in the document — the dangling
     reference #5049 asked to remove, moved from describedby to controls.
     DateInput already gates it on popover.isOpen        · HoverCard.tsx:268

The review, as posted

Thanks for this — the popup contract is right, and it lines up with what DateInput and ComplexSelector already do.

Two things first.

The hook block template puts aria-haspopup="dialog" on a trigger whose card has no label, so the card renders role="group". In Chrome the button announces "has dialog popup" and opens an unnamed group — the mismatch your own description rules out. useHoverCard({label: 'Alex Morgan'}) closes it.

The labelled branch also overwrites the trigger's own aria-haspopup, aria-controls and aria-expanded, and removes them on cleanup. Wrap a menu button in a labelled HoverCard and its menu semantics are gone with nothing saying so; the no-label branch fifteen lines down merges and restores instead.

Can you pass a label in the template and merge rather than overwrite on the trigger?

What changed before posting

Posted as drafted.

Not verified

Chromium only. #5049 asks for a VoiceOver/Safari and NVDA pass on the chosen relationship; the author offered to do it.