Restore focus to the trigger when ColumnFilterMenu closes - #312
Merged
Merged
Conversation
Before: closing the filter popover moved focus to `document.body` instead of back to the trigger that opened it. The popover relies on React Aria's own FocusScope to notice focus fell to `document.body` and restore it, but that check is deferred to a `requestAnimationFrame` callback and only fires after focus has already visibly left the trigger's neighborhood. Both Escape and clicking outside the popover go through the same close path, so both lost focus the same way; Tab does not dismiss the popover at all, since React Aria already traps focus inside it while open. After: ColumnFilterMenu owns a ref to its own trigger button and, in a `useEffect` cleanup that fires exactly on the open-to-closed transition, focuses it directly. React Aria tears down the popover's focus-containment listeners in a layout effect during the same commit, before this passive effect runs, so the trigger is already free to receive focus again by the time it is called - no reliance on the timing-sensitive "did focus fall to body" heuristic, and no interference with React Aria's own restore logic (it no-ops once focus already isn't on `document.body`). This mirrors the ref+effect focus-management idiom DialogImplementation.tsx already uses for open-focus, applied here to the close transition instead, rather than adding a new hand-rolled mechanism. No rendered DOM or CSS changed; only a ref attachment and an effect were added. (#241)
The previous commit restored focus unconditionally: the effect cleanup that fires on the open-to-closed transition called `triggerRef.current .focus()` on every close, whatever had happened. That is wrong whenever the user closed the menu by going somewhere on purpose. With two filterable columns, opening column A's menu and then pressing column B's filter trigger dismisses A - and A's cleanup then yanked focus off B and back onto A's trigger, undoing the move the user had just made. Measured in jsdom before this change: after the press, focus is on B's trigger while the menu is still open, and after the click that closes A it is back on A's trigger. React Aria's FocusScope never had that bug, because it asks a question before it restores: at teardown it checks whether the active element is still inside the dying scope or has already fallen to `document.body`, and it leaves focus alone otherwise - it recovers focus that was dropped, never focus that was moved. What is wrong with it here is only the timing. It defers the actual restore to a `requestAnimationFrame` that re-tests `activeElement === body`, so the restore lands a frame after focus has visibly left the trigger's neighborhood, and silently does nothing at all if something else claims focus in between. So keep React Aria's question and drop its delay. The cleanup now evaluates the same predicate synchronously, in the commit that closed the menu, and focuses the trigger only if focus was dropped. There is nothing deferred left to lose a race with: the decision is made from state that is already final, since the user's focus move and the close both happened before this commit was scheduled. The subtle part is what "dropped" means at the moment the cleanup runs. The popover is still exiting and still in the document, so focus is usually still sitting on the popover's own value input - a drop in progress, not a destination. Reading React Aria's plain body check at this moment would therefore never fire; that is precisely why React Aria has to wait for a frame. A drop is consequently: nothing focused, the body, a node already detached from the document, or anything inside this menu's own popover. Anything else is a place the user went, and is left alone. Matching the popover needs a handle on it, so the popover's ref is captured stickily - React detaches that ref during the same commit that closes the menu, before this passive cleanup runs, so the last element seen is the only handle still available. Covered by a new spec that opens column A's menu, presses column B's trigger with the pointer sequence plus the focus a real `mousedown` performs as its default action (jsdom does not run that default action), and asserts focus settles on B's trigger and not A's. Reverting the guard fails exactly those two assertions on every run while the existing dismissal specs stay green; keeping only React Aria's literal body check, without the in-popover clause, instead fails both existing restore specs. The shared `a_filterable_table` helper grew an `additionalColumns` option and a `triggers` list to render a second filterable column. No rendered DOM and no CSS changed: serialized markup for the closed, open and reclosed states is byte-identical to the previous commit, and identical to pre-PR `main` except for React Aria's own `data-focused` and `data-focus-visible` attributes tracking the restored focus, which is the behavior this PR exists to produce. (#241)
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
1 participant
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.
Summary
ColumnFilterMenucomposes React Aria'sDialogTrigger+Popover+Dialogdirectly, with its own separate trigger button, rather than an integrated single-owner control likeComboBox/Dropdown. React Aria's built-in focus restoration for that shape only notices focus fell todocument.bodyon arequestAnimationFrame-deferred check, so any observer sampling focus right after a dismissal (a keyboard user, a screen reader, or an automated check) sees focus land on the document instead of the trigger. This happened identically whether the popover was dismissed with Escape or by clicking outside; Tab does not dismiss the popover at all, since React Aria already traps focus inside it while open.The restore is deliberately conditional. React Aria's own restore is guarded — at teardown it asks whether the active element is still inside the dying scope or has already fallen to
document.body, and leaves focus alone otherwise, so it only ever recovers focus that was dropped, never focus the user moved. Only the timing of that check is wrong for this shape, not the check itself.ColumnFilterMenutherefore keeps React Aria's question and drops its delay: the same predicate is evaluated synchronously in the effect cleanup that runs in the commit closing the menu, with nothing deferred left to lose a race with.Read at that moment, "dropped" cannot simply mean
document.body: the popover is still exiting and still in the document, so focus is usually still on the popover's own value input — a drop in progress, not a destination. A drop is therefore nothing focused, the body, a node already detached from the document, or anything inside this menu's own popover. Focus anywhere else is somewhere the user deliberately went, such as another column's filter trigger, and is left untouched.Fixed
ColumnFilterMenunow returns focus to its own trigger button when the filter popover closes having dropped focus, for both Escape and outside-click dismissal, instead of depending on React Aria's timing-sensitive automatic restore (ColumnFilterMenu Escape dismissal does not restore focus to its trigger #241)Behavior and compatibility
No rendered DOM and no CSS changed. Serialized markup for the closed, open and reclosed states is identical to pre-PR
mainapart from React Aria's owndata-focused/data-focus-visibleattributes following the restored focus, which is the behavior this PR exists to produce.