fix(react-popover): Stop propagation when closing the PopoverSurface with Escape

[sopranopillow]

Previous Behavior

When Popover was used inside a Dialog component (note Popover has to be nested) and you press escape to close the Popover, the Dialog is closed along with PopoverSurface.

New Behavior

Stop Propagation to avoid closing the parent Dialog.

[size-auditor]

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 989ead3c07d6df61441f9c6ca8102634e40dc48d (build)

[fabricteam]

📊 Bundle size report

Package & Exports	Baseline (minified/GZIP)	PR	Change
react-components react-components: Accordion, Button, FluentProvider, Image, Menu, Popover	203.937 kB 57.086 kB	203.958 kB 57.089 kB	21 B 3 B
react-infobutton InfoButton	127.977 kB 39.012 kB	127.998 kB 39.025 kB	21 B 13 B
react-infobutton InfoLabel	131.26 kB 40 kB	131.468 kB 40.085 kB	208 B 85 B
react-popover Popover	114.969 kB 35.392 kB	114.99 kB 35.397 kB	21 B 5 B

Unchanged fixtures

Package & Exports	Size (minified/GZIP)
react-alert Alert	93.534 kB 22.497 kB
react-avatar Avatar	57.754 kB 15.041 kB
react-avatar AvatarGroup	15.632 kB 6.258 kB
react-avatar AvatarGroupItem	73.968 kB 19.552 kB
react-components react-components: Button, FluentProvider & webLightTheme	64.855 kB 17.852 kB
react-components react-components: FluentProvider & webLightTheme	36.086 kB 11.9 kB
react-datepicker-compat DatePicker Compat	220.303 kB 58.486 kB
react-persona Persona	64.675 kB 16.968 kB
react-portal-compat PortalCompatProvider	6.446 kB 2.186 kB
react-table DataGrid	147.583 kB 40.523 kB
react-table Table (Primitives only)	44.348 kB 12.347 kB
react-table Table as DataGrid	130.142 kB 33.083 kB
react-table Table (Selection only)	78.056 kB 19.084 kB
react-table Table (Sort only)	77.386 kB 18.896 kB
react-tags Tag	20.637 kB 7.332 kB

_{🤖 This report was generated against 989ead3c07d6df61441f9c6ca8102634e40dc48d}

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 001e09d:

Sandbox	Source
@fluentui/react 8 starter	Configuration
@fluentui/react-components 9 starter	Configuration

[fabricteam]

Perf Analysis (@fluentui/react-components)

Scenario	Render type	Master Ticks	PR Ticks	Iterations	Status
InfoButton	mount	21	22	5000	Possible regression

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations	Status
Avatar	mount	706	710	5000
Button	mount	381	392	5000
Field	mount	1305	1303	5000
FluentProvider	mount	946	944	5000
FluentProviderWithTheme	mount	121	130	10
FluentProviderWithTheme	virtual-rerender	105	112	10
FluentProviderWithTheme	virtual-rerender-with-unmount	113	113	10
InfoButton	mount	21	22	5000	Possible regression
MakeStyles	mount	1111	1145	50000
Persona	mount	2036	2044	5000
SpinButton	mount	1601	1602	5000

[fabricteam]

🕵 fluentuiv9 No visual regressions between this PR and main

[bsunderhus]

I'm not sure if stop propagation is ideal (although we're already doing exactly that everywhere apparently 🤷🏼‍♂️).

https://github.com/microsoft/fluentui/blob/master/packages/react-components/react-popover/src/components/PopoverTrigger/usePopoverTrigger.ts#L51-L53

This brings me the question if we shouldn't have a global context to indicate which component is currently responsible to react for such events 🤔. WDYT @ling1726?

[bsunderhus]

Seems to be related to having a focusable element inside the Popover actually (not necessarily related to nested popovers) here's an example:

https://codesandbox.io/s/reverent-booth-8l2w1j?file=/example.tsx

[bsunderhus]

Let's prevent default instead of stopping propagation as followed in the discussion: https://github.com/microsoft/fluentui/pull/27832/files#r1193611849

WDYT @sopranopillow ?!

[sopranopillow]

Hey, sorry to answer until now @bsunderhus @ling1726,

Let's prevent default instead of stopping propagation as followed in the discussion

sounds good, looks like it is the way to go, but this is a Dialog focused solution. I'll talk to Sarah about her thoughts on how we should handle propagation and such, but the data attribute sounds promising. I've created this issue to keep track of working on that.

As a side note: there's a bug with tabster where having the popover inside the dialog and closing it (with the preventDefault) refocuses to the Dialog's trigger and not the Popover's trigger. So in case you're testing this and see that happening, that's the reason why. It's due to tabster not keeping/not working correctly with a focus queue to go back to the previous trigger.

[msft-fluent-ui-bot]

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes
• Official NPM Release

[msft-fluent-ui-bot]

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes
• Official NPM Release