Skip to content

Commit 11229e4

Browse files
petdudpetdudlayershifter
authored
fix: close Popover when focus escapes outside while trapFocus is enabled (#35995)
Co-authored-by: petdud <petrduda@microsoft.com> Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
1 parent 9a5e837 commit 11229e4

6 files changed

Lines changed: 324 additions & 2 deletions

File tree

‎change/@fluentui-react-popover-ea9e0418-25e8-45e1-8dda-ffbe8169164a.json‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"type": "patch",
3+
"comment": "fix: close Popover when focus escapes outside while trapFocus is enabled",
4+
"packageName": "@fluentui/react-popover",
5+
"email": "petrduda@microsoft.com",
6+
"dependentChangeType": "patch"
7+
}

‎packages/react-components/react-popover/library/etc/react-popover.api.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ export type OnOpenChangeData = {
3333
};
3434

3535
// @public
36-
export type OpenPopoverEvents = MouseEvent | TouchEvent | React_2.FocusEvent<HTMLElement> | React_2.KeyboardEvent<HTMLElement> | React_2.MouseEvent<HTMLElement>;
36+
export type OpenPopoverEvents = MouseEvent | TouchEvent | FocusEvent | React_2.FocusEvent<HTMLElement> | React_2.KeyboardEvent<HTMLElement> | React_2.MouseEvent<HTMLElement>;
3737

3838
// @public
3939
export const Popover: React_2.FC<PopoverProps>;

‎packages/react-components/react-popover/library/src/components/Popover/Popover.cy.tsx‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -453,6 +453,108 @@ describe('Popover', () => {
453453
cy.contains('Two').should('have.focus');
454454
});
455455
});
456+
457+
describe('close on focus escape', () => {
458+
it('should close when focus is programmatically moved outside', () => {
459+
mount(
460+
<>
461+
<button id="outside">Outside</button>
462+
<Popover trapFocus>
463+
<PopoverTrigger disableButtonEnhancement>
464+
<button>Popover trigger</button>
465+
</PopoverTrigger>
466+
<PopoverSurface>
467+
<button>Inside</button>
468+
</PopoverSurface>
469+
</Popover>
470+
</>,
471+
);
472+
473+
cy.get(popoverTriggerSelector).click();
474+
cy.get(popoverInteractiveContentSelector).should('be.visible');
475+
cy.get('#outside').focus();
476+
cy.get(popoverInteractiveContentSelector).should('not.exist');
477+
});
478+
479+
it('should close with inertTrapFocus when focus is programmatically moved outside', () => {
480+
mount(
481+
<>
482+
<button id="outside">Outside</button>
483+
<Popover trapFocus inertTrapFocus>
484+
<PopoverTrigger disableButtonEnhancement>
485+
<button>Popover trigger</button>
486+
</PopoverTrigger>
487+
<PopoverSurface>
488+
<button>Inside</button>
489+
</PopoverSurface>
490+
</Popover>
491+
</>,
492+
);
493+
494+
cy.get(popoverTriggerSelector).click();
495+
cy.get(popoverInteractiveContentSelector).should('be.visible');
496+
cy.get('#outside').focus();
497+
cy.get(popoverInteractiveContentSelector).should('not.exist');
498+
});
499+
500+
it('should not close without trapFocus when focus moves outside', () => {
501+
mount(
502+
<>
503+
<button id="outside">Outside</button>
504+
<Popover>
505+
<PopoverTrigger disableButtonEnhancement>
506+
<button>Popover trigger</button>
507+
</PopoverTrigger>
508+
<PopoverSurface>This is a popover</PopoverSurface>
509+
</Popover>
510+
</>,
511+
);
512+
513+
cy.get(popoverTriggerSelector).click();
514+
cy.get(popoverContentSelector).should('be.visible');
515+
cy.get('#outside').focus();
516+
cy.get(popoverContentSelector).should('be.visible');
517+
});
518+
519+
it('should not close when closeOnFocusOutside is false', () => {
520+
mount(
521+
<>
522+
<button id="outside">Outside</button>
523+
<Popover trapFocus {...({ closeOnFocusOutside: false } as unknown as PopoverProps)}>
524+
<PopoverTrigger disableButtonEnhancement>
525+
<button>Popover trigger</button>
526+
</PopoverTrigger>
527+
<PopoverSurface>
528+
<button>Inside</button>
529+
</PopoverSurface>
530+
</Popover>
531+
</>,
532+
);
533+
534+
cy.get(popoverTriggerSelector).click();
535+
cy.get(popoverInteractiveContentSelector).should('be.visible');
536+
cy.get('#outside').focus();
537+
cy.get(popoverInteractiveContentSelector).should('be.visible');
538+
});
539+
540+
it('should not close when focus moves to the trigger', () => {
541+
mount(
542+
<Popover trapFocus>
543+
<PopoverTrigger disableButtonEnhancement>
544+
<button>Popover trigger</button>
545+
</PopoverTrigger>
546+
<PopoverSurface>
547+
<button>Inside</button>
548+
</PopoverSurface>
549+
</Popover>,
550+
);
551+
552+
cy.get(popoverTriggerSelector).click();
553+
cy.get(popoverInteractiveContentSelector).should('be.visible');
554+
cy.get(popoverTriggerSelector).focus();
555+
cy.get(popoverInteractiveContentSelector).should('be.visible');
556+
});
557+
});
456558
});
457559

458560
describe('with Iframe', () => {

‎packages/react-components/react-popover/library/src/components/Popover/Popover.test.tsx‎

Lines changed: 180 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
import * as React from 'react';
22
import { Popover } from './Popover';
3-
import { renderHook } from '@testing-library/react-hooks';
3+
import { renderHook, act } from '@testing-library/react-hooks';
44
import { usePopover_unstable } from './usePopover';
55
import { isConformant } from '../../testing/isConformant';
6+
import type { PopoverProps } from './Popover.types';
67

78
describe('Popover', () => {
89
isConformant({
@@ -37,4 +38,182 @@ describe('Popover', () => {
3738
// Assert
3839
expect(result.current.withArrow).toBe(false);
3940
});
41+
42+
describe('close on focus outside', () => {
43+
it('should close when trapFocus is enabled and focus moves outside', () => {
44+
const onOpenChange = jest.fn();
45+
const outsideButton = document.createElement('button');
46+
const popoverContent = document.createElement('div');
47+
document.body.appendChild(outsideButton);
48+
document.body.appendChild(popoverContent);
49+
50+
const { result } = renderHook(
51+
({ open }) =>
52+
usePopover_unstable({
53+
open,
54+
trapFocus: true,
55+
onOpenChange,
56+
children: <div />,
57+
}),
58+
{ initialProps: { open: true } },
59+
);
60+
61+
// Set the contentRef to simulate mounted popover content
62+
act(() => {
63+
(result.current.contentRef as React.RefObject<HTMLElement | null>).current = popoverContent;
64+
});
65+
66+
act(() => {
67+
outsideButton.dispatchEvent(new FocusEvent('focusin', { bubbles: true }));
68+
});
69+
70+
expect(onOpenChange).toHaveBeenCalledWith(expect.anything(), { open: false });
71+
72+
document.body.removeChild(outsideButton);
73+
document.body.removeChild(popoverContent);
74+
});
75+
76+
it('should not close when trapFocus is not enabled and focus moves outside', () => {
77+
const onOpenChange = jest.fn();
78+
const outsideButton = document.createElement('button');
79+
document.body.appendChild(outsideButton);
80+
81+
renderHook(() =>
82+
usePopover_unstable({
83+
open: true,
84+
trapFocus: false,
85+
onOpenChange,
86+
children: <div />,
87+
}),
88+
);
89+
90+
act(() => {
91+
outsideButton.dispatchEvent(new FocusEvent('focusin', { bubbles: true }));
92+
});
93+
94+
expect(onOpenChange).not.toHaveBeenCalled();
95+
96+
document.body.removeChild(outsideButton);
97+
});
98+
99+
it('should not close when popover is not open', () => {
100+
const onOpenChange = jest.fn();
101+
const outsideButton = document.createElement('button');
102+
document.body.appendChild(outsideButton);
103+
104+
renderHook(() =>
105+
usePopover_unstable({
106+
open: false,
107+
trapFocus: true,
108+
onOpenChange,
109+
children: <div />,
110+
}),
111+
);
112+
113+
act(() => {
114+
outsideButton.dispatchEvent(new FocusEvent('focusin', { bubbles: true }));
115+
});
116+
117+
expect(onOpenChange).not.toHaveBeenCalled();
118+
119+
document.body.removeChild(outsideButton);
120+
});
121+
122+
it('should also close when inertTrapFocus is enabled and focus moves to a page element outside', () => {
123+
const onOpenChange = jest.fn();
124+
const outsideButton = document.createElement('button');
125+
const popoverContent = document.createElement('div');
126+
document.body.appendChild(outsideButton);
127+
document.body.appendChild(popoverContent);
128+
129+
const { result } = renderHook(() =>
130+
usePopover_unstable({
131+
open: true,
132+
trapFocus: true,
133+
inertTrapFocus: true,
134+
onOpenChange,
135+
children: <div />,
136+
}),
137+
);
138+
139+
act(() => {
140+
(result.current.contentRef as React.RefObject<HTMLElement | null>).current = popoverContent;
141+
});
142+
143+
act(() => {
144+
outsideButton.dispatchEvent(new FocusEvent('focusin', { bubbles: true }));
145+
});
146+
147+
expect(onOpenChange).toHaveBeenCalledWith(expect.anything(), { open: false });
148+
149+
document.body.removeChild(outsideButton);
150+
document.body.removeChild(popoverContent);
151+
});
152+
153+
it('should not close when closeOnFocusOutside is false', () => {
154+
const onOpenChange = jest.fn();
155+
const outsideButton = document.createElement('button');
156+
const popoverContent = document.createElement('div');
157+
document.body.appendChild(outsideButton);
158+
document.body.appendChild(popoverContent);
159+
160+
const { result } = renderHook(
161+
({ open }) =>
162+
usePopover_unstable({
163+
open,
164+
trapFocus: true,
165+
closeOnFocusOutside: false,
166+
onOpenChange,
167+
children: <div />,
168+
} as unknown as PopoverProps),
169+
{ initialProps: { open: true } },
170+
);
171+
172+
act(() => {
173+
(result.current.contentRef as React.RefObject<HTMLElement | null>).current = popoverContent;
174+
});
175+
176+
act(() => {
177+
outsideButton.dispatchEvent(new FocusEvent('focusin', { bubbles: true }));
178+
});
179+
180+
expect(onOpenChange).not.toHaveBeenCalled();
181+
182+
document.body.removeChild(outsideButton);
183+
document.body.removeChild(popoverContent);
184+
});
185+
186+
it('should not close when focus moves to the trigger element', () => {
187+
const onOpenChange = jest.fn();
188+
const triggerButton = document.createElement('button');
189+
const popoverContent = document.createElement('div');
190+
document.body.appendChild(triggerButton);
191+
document.body.appendChild(popoverContent);
192+
193+
const { result } = renderHook(
194+
({ open }) =>
195+
usePopover_unstable({
196+
open,
197+
trapFocus: true,
198+
onOpenChange,
199+
children: <div />,
200+
}),
201+
{ initialProps: { open: true } },
202+
);
203+
204+
act(() => {
205+
(result.current.contentRef as React.RefObject<HTMLElement | null>).current = popoverContent;
206+
(result.current.triggerRef as React.RefObject<HTMLElement | null>).current = triggerButton;
207+
});
208+
209+
act(() => {
210+
triggerButton.dispatchEvent(new FocusEvent('focusin', { bubbles: true }));
211+
});
212+
213+
expect(onOpenChange).not.toHaveBeenCalled();
214+
215+
document.body.removeChild(triggerButton);
216+
document.body.removeChild(popoverContent);
217+
});
218+
});
40219
});

‎packages/react-components/react-popover/library/src/components/Popover/Popover.types.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,7 @@ export type OnOpenChangeData = { open: boolean };
236236
export type OpenPopoverEvents =
237237
| MouseEvent
238238
| TouchEvent
239+
| FocusEvent
239240
| React.FocusEvent<HTMLElement>
240241
| React.KeyboardEvent<HTMLElement>
241242
| React.MouseEvent<HTMLElement>;

‎packages/react-components/react-popover/library/src/components/Popover/usePopover.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,39 @@ export const usePopoverBase_unstable = (props: PopoverBaseProps): PopoverBaseSta
168168
disabled: !open || !closeOnScroll,
169169
});
170170

171+
// When trapFocus is enabled, close the popover if focus is programmatically moved outside
172+
// (e.g. via element.focus()), which doesn't trigger click or scroll dismiss handlers.
173+
// Internal `closeOnFocusOutside` prop allows consumers to opt out during gradual rollout.
174+
const closeOnFocusOutside =
175+
(props as PopoverBaseProps & { closeOnFocusOutside?: boolean }).closeOnFocusOutside ?? true;
176+
177+
const closeOnFocusOutCallback = useEventCallback((ev: FocusEvent) => {
178+
const target = (ev.composedPath()[0] ?? ev.target) as HTMLElement;
179+
const contentElement = positioningRefs.contentRef.current;
180+
const triggerElement = positioningRefs.triggerRef.current ?? null;
181+
182+
if (!contentElement) {
183+
return;
184+
}
185+
186+
const isOutside = !elementContains(contentElement, target) && !elementContains(triggerElement, target);
187+
188+
if (isOutside) {
189+
setOpen(ev, false);
190+
}
191+
});
192+
193+
React.useEffect(() => {
194+
if (!open || !props.trapFocus || !closeOnFocusOutside) {
195+
return;
196+
}
197+
198+
targetDocument?.addEventListener('focusin', closeOnFocusOutCallback, true);
199+
return () => {
200+
targetDocument?.removeEventListener('focusin', closeOnFocusOutCallback, true);
201+
};
202+
}, [open, props.trapFocus, closeOnFocusOutside, targetDocument, closeOnFocusOutCallback]);
203+
171204
const { findFirstFocusable } = useFocusFinders();
172205
const activateModal = useActivateModal();
173206

0 commit comments

Comments
 (0)