[#1660] Handle focus when focused node ancestor is removed#1861
Merged
Conversation
Fixes #1660 Detect focused-node removal when an ancestor subtree is removed, and only force refocus when at least one trap container remains connected. Add demo + Cypress regression coverage for the remove-parent path in the DOM-removal scenario.
🦋 Changeset detectedLatest commit: 4ac943a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR fixes a focus-management bug where removing an ancestor of the currently focused element inside a focus trap could cause focus to move to the document body. It updates DOM-removal detection and focus restoration logic, adjusts lifecycle callback signatures and return-focus timing, and extends demos/tests to cover ancestor removal. Ancestor-Removal Bug Fix
Sequence DiagramsequenceDiagram
participant User as User (click)
participant DOM as DOM
participant MO as MutationObserver
participant Trap as FocusTrap
participant Logic as checkDomRemoval / restore flow
User->>DOM: click remove-parent or remove element
DOM->>MO: mutation (removedNodes includes node/ancestor)
MO->>Trap: notify mutation
Trap->>Logic: checkDomRemoval()
Logic->>Logic: if focusedNode absent -> return
Logic->>Logic: detect removedNode === focusedNode OR removedNode.contains(focusedNode)
Logic->>Trap: if containers.some(isConnected) then updateTabbableNodes()
Logic->>Trap: compute getInitialFocusNode()
Logic->>Trap: tryFocus(initialFocusNode)
Trap-->>DOM: focus moves to initial focusable element
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes (Concise and slightly cheeky: the bug ran off to play hide-and-seek with focus; now it gets politely guided back.) 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cypress/e2e/focus-trap-demo.cy.js (1)
1849-1850: ⚡ Quick winTighten this assertion so it can’t pass on the “right text, wrong element.”
Line 1850 validates focused text, but not that focus stayed inside
#demo-dom-remove. Since"with"appears in multiple demos, this can mask an escape. Add a containment check on the focused node.Suggested tweak
cy.get('@testRoot').find('#dom-remove-parent-button').click(); -cy.focused().should('not.be.undefined').and('have.text', 'with'); +cy.focused() + .should('not.be.undefined') + .and('have.text', 'with') + .and(($el) => { + expect($el.closest('#demo-dom-remove').length).to.equal(1); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/focus-trap-demo.cy.js` around lines 1849 - 1850, The current assertion checks only the focused element's text and can pass if another element elsewhere contains "with"; modify the assertion so cy.focused() is also asserted to be inside the demo container (`#demo-dom-remove`). Update the chain after clicking '#dom-remove-parent-button' to assert both cy.focused().should('have.text', 'with') and that the focused node is contained within '#demo-dom-remove' (e.g., add an assertion like .and('be.descendantOf', '#demo-dom-remove') or equivalent) so focus is verified to remain inside the correct demo root referenced by '@testRoot' and '#demo-dom-remove'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.js`:
- Around line 999-1005: The recovery path uses getInitialFocusNode() before
refreshing tabbable state, so if the removed ancestor contained the trap’s first
tabbable or the configured initial focus the call can return a detached element;
update the logic in the block that checks isFocusedNodeRemoved and
state.containers (the code that then calls tryFocus(getInitialFocusNode())) to
recompute/refresh tabbables/tabbableGroups first (invoke whatever function
updates state.tabbableGroups or recomputeTabbables used elsewhere) and only then
call getInitialFocusNode() and tryFocus so the recovery target is always a
current, connected element.
---
Nitpick comments:
In `@cypress/e2e/focus-trap-demo.cy.js`:
- Around line 1849-1850: The current assertion checks only the focused element's
text and can pass if another element elsewhere contains "with"; modify the
assertion so cy.focused() is also asserted to be inside the demo container
(`#demo-dom-remove`). Update the chain after clicking '#dom-remove-parent-button'
to assert both cy.focused().should('have.text', 'with') and that the focused
node is contained within '#demo-dom-remove' (e.g., add an assertion like
.and('be.descendantOf', '#demo-dom-remove') or equivalent) so focus is verified
to remain inside the correct demo root referenced by '@testRoot' and
'#demo-dom-remove'.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af0435f7-1218-4202-a532-5e112a96cc35
⛔ Files ignored due to path filters (1)
docs/demo-bundle.js.mapis excluded by!**/*.map
📒 Files selected for processing (6)
.changeset/soft-socks-eat.mdcypress/e2e/focus-trap-demo.cy.jsdocs/demo-bundle.jsdocs/index.htmldocs/js/dom-remove.jsindex.js
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.
Fixes #1660
Detect focused-node removal when an ancestor subtree is removed, and only force refocus when at least one trap container remains connected.
Add demo + Cypress regression coverage for the remove-parent path in the DOM-removal scenario.
PR Checklist
Please leave this checklist in your PR.
npm run demo-bundlein your branch and include the/docs/demo-bundle.jsfile that gets generated in your PR).// TEST MANUALLYcomments here) that can't be fully tested in Cypress have been manually verified.typeof document/window !== 'undefined'before using it in code that gets executed on load.npm run changesetlocally to add one, and follow the prompts).Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Tests