va-modal: fix aria-hidden race condition from rAF timing#2045
Conversation
When React rapidly toggles visible=true then visible=false across two render passes, componentDidUpdate schedules setupModal via rAF on the first pass, then teardownModal runs synchronously on the second pass while the rAF is still pending. The stale rAF fires setupModal after teardown, calling applyAriaHidden() which permanently orphans aria-hidden="true" and data-aria-hidden="true" on DOM elements, causing aria-hidden-focus axe violations. Fix: store the rAF ID and cancel it in teardownModal and disconnectedCallback so a deferred setupModal never fires after teardown. Add e2e test verifying no orphaned aria-hidden attributes remain after an open/close cycle.
There was a problem hiding this comment.
Pull request overview
Fixes a timing race in va-modal where a pending requestAnimationFrame-scheduled setupModal() could run after teardownModal(), leaving orphaned aria-hidden / data-aria-hidden attributes on non-modal content (notably reproducible via rapid React prop toggles).
Changes:
- Track the
requestAnimationFrameid used to defersetupModal()and cancel it during teardown/disconnect. - Add an e2e regression test asserting
aria-hiddencleanup after a modal open/close cycle.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/web-components/src/components/va-modal/va-modal.tsx | Stores and cancels the pending rAF for setupModal() to prevent post-teardown aria-hidden application. |
| packages/web-components/src/components/va-modal/test/va-modal.e2e.ts | Adds an e2e test to verify aria-hidden / data-aria-hidden are cleaned up after closing. |
| @@ -187,13 +190,17 @@ export class VaModal { | |||
|
|
|||
| this.isVisibleDirty = false; | |||
| if (this.isVisible()) { | |||
| requestAnimationFrame(() => this.setupModal()); | |||
| this.setupModalRafId = requestAnimationFrame(() => this.setupModal()); | |||
| } else { | |||
There was a problem hiding this comment.
setupModalRafId is intended to represent a pending rAF callback, but it’s never cleared when the rAF actually runs. Consider clearing setupModalRafId inside the rAF callback (before calling setupModal) so later teardown/disconnect logic reflects the real state and avoids canceling an already-fired frame ID.
| if (this.setupModalRafId) { | ||
| cancelAnimationFrame(this.setupModalRafId); | ||
| this.setupModalRafId = undefined; | ||
| } |
There was a problem hiding this comment.
disconnectedCallback() now duplicates the rAF-cancellation logic that already exists in teardownModal(). To avoid divergence over time, consider extracting a small helper (e.g., cancelSetupModalRaf()) or relying solely on teardownModal() to perform the cancellation.
| if (this.setupModalRafId) { | |
| cancelAnimationFrame(this.setupModalRafId); | |
| this.setupModalRafId = undefined; | |
| } |
| // Additionally verify no elements in the document have orphaned attributes | ||
| const orphanedCount = await page.evaluate(() => { | ||
| return document.querySelectorAll( | ||
| '[aria-hidden="true"], [data-aria-hidden="true"]', | ||
| ).length; | ||
| }); |
There was a problem hiding this comment.
The comment says this check verifies no elements in the document have orphaned aria-hidden/data-aria-hidden, but document.querySelectorAll(...) won’t traverse into shadow roots. If the intent is to cover shadow DOM cases too, update the evaluation to walk shadow roots; otherwise consider tightening the wording so the test doesn’t imply broader coverage than it has.
jamigibbs
added
the
run chromatic workflow
Chromatic
https://fix-va-modal-aria-hidden-raf-race--65a6e2ed2314f7b8f98609d8.chromatic.com
Summary
Fixed orphaned
aria-hiddenattributes after modal close. Cancels pendingrequestAnimationFrameinteardownModalto prevent a race condition wheresetupModalfires after teardown, permanently leavingaria-hidden="true"/data-aria-hidden="true"on DOM elements.Description
When React rapidly toggles
visible=truethenvisible=falseacross two render passes (e.g., opening a modal with a checkbox click then immediately pressing Escape),componentDidUpdateschedulessetupModalviarequestAnimationFrameon the first pass, thenteardownModalruns synchronously on the second pass while the rAF is still pending.The stale rAF then fires
setupModalafter teardown, callingapplyAriaHidden()which resetsthis.undoAriaHidden = []and pushes new undo functions fromhideOthers(). SinceteardownModalalready ran, nobody will ever invoke those undo functions — leavingaria-hidden="true"anddata-aria-hidden="true"permanently on DOM elements outside the modal.This causes
aria-hidden-focusaxe violations on focusable elements likeva-link,va-accordion-item,va-text-input, etc.The fix:
requestAnimationFrameID insetupModalRafIdteardownModal()usingcancelAnimationFrame()disconnectedCallback()for safetyWhy this is React-specific: The React binding (
@stencil/react-output-target) usesattachPropswhich setsnode.visible = valueas a property (synchronous), triggering Stencil's@Watchimmediately. Two rapid React renders produce two separate Stencil render cycles with the rAF from the first still pending when the second fires teardown. Vanilla JS attribute changes get batched by Stencil into a single render.Root cause timeline:
requestAnimationFramewrapper incomponentDidLoad/componentDidUpdateto fix a TypeErrorundoAriaHiddenfrom a singleUndotoUndo[], making the race consequence more severeapplyAriaHidden()to cover shadow roots and light DOM containers, increasing the number of affected elementsRelated tickets and links
This fixes
aria-hidden-focusaxe violations seen in vets-website CI:Screenshots
N/A — no visual changes.
Testing and review
aria-hiddenattributes remain after open/close cycleApprovals
Applicable checklist: Component Fix
patchlabel