fix(focusTrap): prevent blur if trap container is not in DOM #1258
Conversation
🦋 Changeset detectedLatest commit: 544280b 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 |
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/primer/primer-components/BQo3VUjQZzC23ioK4rMJa9GtGV7p |
| @@ -71,7 +71,7 @@ export function focusTrap( | |||
| // is not found, blur the recently-focused element so that focus doesn't leave the | |||
| // trap zone. | |||
| function ensureTrapZoneHasFocus(focusedElement: EventTarget | null) { | |||
| if (focusedElement instanceof HTMLElement) { | |||
| if (focusedElement instanceof HTMLElement && document.contains(container)) { | |||
There was a problem hiding this comment.
Is the condition below more robust (i.e. supports either Light DOM or Shadow DOM)?
| if (focusedElement instanceof HTMLElement && document.contains(container)) { | |
| if (focusedElement instanceof HTMLElement && focusedElement.getRootNode().contains(container)) { |
There was a problem hiding this comment.
Very interesting call out! I hadn't considered shadow DOM yet and I think that's worth exploring. Given my lack of shadow DOM experience, I can't say for certain that this change is the right solution, but I do think we should consider it in the future.
| @@ -170,6 +170,35 @@ it('Should should release the trap when the signal is aborted', async () => { | |||
| expect(document.activeElement).toEqual(durianButton) | |||
| }) | |||
|
|
|||
| it('Should should release the trap when the container is removed from the DOM', async () => { | |||
There was a problem hiding this comment.
Great test! Thanks for covering this case. ✨
There are some
AnchoredOverlayuse cases where the consumer moves focus to another element upon closing the overlay. Currently, it's common that they move focus after closing the overlay, but thefocusTraphas not yet been disabled because of the execution order of the effects. In these cases, thefocusTrapcontaineris not physically present in the DOM at the time of moving focus. This solution hasfocusTrapensure that it is still actually in the DOM before trying to trap focus.Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.