test(Modal): make the reka-ui#1280 removal trigger itself - #458
Merged
Conversation
#159 tracks removing `useBlurOnOpen` once upstream fixes the aria-hidden focus bug. Checked both ways, and the answer is no: - unovue/reka-ui#1280 is still open — filed 2024-08-23, no assignee, no linked PR, no maintainer response; - reka-ui 2.10.3, the version pinned here, still has `dist/shared/useHideOthers.js` importing `hideOthers` from aria-hidden. So nothing is removed. What is fixed is the tracking itself. The issue tells the next reader to run `grep -rn 'reka-ui#1280' src/ test/` and says every site needing review carries that tag. It returned nothing: only the composable was annotated, and it spelled the reference as a URL. All six sites carry the tag now — the composable, the three components that wrap it, and the three specs that assert it. test/utils/blur-on-open-workaround.spec.ts asserts the condition rather than trusting anyone to re-read a tracking issue: it fails the day reka-ui's useHideOthers stops importing hideOthers, naming #159 and the removal steps. A failure there is good news. It also pins the tag list, so the documented grep cannot quietly go back to finding nothing. Not-found is deliberately not a pass — reka-ui moving that file is exactly the kind of change that would ship the fix alongside it. Refs #159 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012MsMuj8Fic9tjWVjyEyrxc
Review turned the answer to #159 into a defect report. The workaround itself was breaking focus restoration, and had been since it landed. reka-ui restores focus on close to `triggerElement`, which its `DialogTrigger` sets — but that slot is optional (`v-if="!!slots.default"`), so for a programmatic open, `useOverlay()` or a bare `v-model:open`, reka-ui falls back to capturing `document.activeElement` when the content mounts, and skips when it is `<body>`. This workaround blurs synchronously before that capture, so the fallback found `<body>`, `triggerElement` stayed null, and the `.focus()` on close was a silent no-op. Focus was stranded until the user tabbed back in — WCAG 2.4.3, on three shipped components. Confirmed by A/B rather than by reading: neutralising the blur restores focus, reinstating it strands focus again. The composable now remembers what it blurred and hands focus back when `open` goes false, after the overlay has finished unmounting, and only if nothing else claimed focus meanwhile — so the trigger-slot path, where reka-ui does this correctly on its own, is untouched. Also from review: - the component specs could not see `flush: 'sync'` at all. They drive `open` through `await setProps`, which flushes first, so a 'pre' watcher looked identical and dropping the flush left all 87 overlay tests green while the console warning came back. test/composables/useBlurOnOpen.spec.ts asserts the blur before any await, and covers the uncontrolled emits path that no spec reached; - the tripwire matched `hideOthers` as a substring, so `inertOthers as hideOthers` would have kept it quiet. It matches the import and the call now; - the tag list was an exact set, which would have failed a future component legitimately joining it with a message about upstream being fixed. Subset now; - the docblock claimed focus return was unaffected. It was the opposite. Documented the remaining trade-off on the Modal, Slideover and Drawer pages, where a consumer testing with VoiceOver can find it, and recorded the divergence in .sync/PORTING.md. Refs #159 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012MsMuj8Fic9tjWVjyEyrxc
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Linked issue
Refs #159 — which stays open, deliberately. See below.
Type of change
The answer to what #159 asks
Upstream is not fixed, so nothing is removed. Checked two independent ways rather than one:
reka-ui@2.10.3(the version pinned here, bumped in #428)dist/shared/useHideOthers.jsstill doesimport { hideOthers } from "aria-hidden"The second is the one that actually decides it: the issue could close for any reason, and what matters is the code we ship against.
Worth noting for whoever eventually does the upstream PR:
aria-hidden@1.2.6— already in the tree — exportsinertOthersandsuppressOthers, the latter picking betweeninertandaria-hiddenby feature detection. The upstream fix is a one-line swap, which matches #159's read that the blocker is attention rather than difficulty.What this PR does fix
#159 tells the next reader:
It returned nothing. Only
useBlurOnOpen.tswas annotated, and it spelled the reference as a URL rather than the documented token. So the removal instructions rest on a search that finds zero results — the tracking issue's own findability mechanism did not work.All six sites carry the tag now: the composable, the three components that wrap it (
Modal,Slideover,Drawer), and the three specs that assert it.And makes the check stop depending on memory
test/utils/blur-on-open-workaround.spec.tsasserts the condition instead of trusting someone to re-read a tracking issue at the right moment:useHideOthersstill importshideOthers, the workaround is needed and the spec passes;useBlurOnOpenworkaround after reka-ui#1280 is fixed #159 and pointing at the removal steps.A failure there is good news, not a regression, and the message says so. It also pins the tag list, so the documented grep cannot quietly go back to finding nothing.
useHideOthers.jsbeing missing is deliberately not treated as a pass: reka-ui restructuring that file is precisely the kind of change that would ship the fix alongside it, so it demands a look by hand.The issue suggested re-checking "once per reka-ui major bump". This makes it every bump, at no cost to anyone.
Why #159 stays open
It tracks the removal, and the removal has not happened. Closing it would lose the tracking; the only thing that changed is that it now trips on its own.
Verification
hideOthers→suppressOthersin the installed package)pnpm run typecheckeslinton every changed fileGenerated by Claude Code