feat(react-dialog): allows user to opt-out of uncontrolled open changes

[bsunderhus]

Previous Behavior

At the moment if the user wants to prevent Escape to close a dialog, controlling the dialog open state is required

const [open, setOpen] = React.useState(false);
return (
  <Dialog
    open={open}
    onOpenChange={(event, data) => {
      if (data.type !== "escapeKeyDown") {
        setOpen(data.open);
      }
    }}
    modalType="alert"
  >
  {/* ... */}
  </Dialog>
);

https://codesandbox.io/s/sad-star-c7ytcs?file=/example.tsx

New Behavior

1. allows user to opt-out of the uncontrolled open state change by passing a boolean as the return of onOpenChange

To opt-out of Escape for example, this is doable:

<Dialog
  onOpenChange={(event, data) => {
    if (data.type === "escapeKeyDown") {
      return false // opting out of modifying the internal open state
    }
  }}
  modalType="alert"
>
  {/*...*/}
</Dialog>

[bsunderhus]

Follow up on #28276 (comment)

[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 6f318ca:

Sandbox	Source
@fluentui/react 8 starter	Configuration
@fluentui/react-components 9 starter	Configuration
autumn-dawn-4288dl	PR

[fabricteam]

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations	Status
Avatar	mount	601	613	5000
Button	mount	313	308	5000
Field	mount	1122	1124	5000
FluentProvider	mount	708	705	5000
FluentProviderWithTheme	mount	85	83	10
FluentProviderWithTheme	virtual-rerender	65	67	10
FluentProviderWithTheme	virtual-rerender-with-unmount	70	84	10
InfoButton	mount	17	8	5000
MakeStyles	mount	829	873	50000
Persona	mount	1742	1686	5000
SpinButton	mount	1369	1343	5000

[fabricteam]

🕵 fluentuiv9 No visual regressions between this PR and main

[fabricteam]

📊 Bundle size report

Package & Exports	Baseline (minified/GZIP)	PR	Change
react-dialog Dialog (including children components)	90.125 kB 27.485 kB	90.163 kB 27.5 kB	38 B 15 B

Unchanged fixtures

Package & Exports	Size (minified/GZIP)
react-components react-components: Button, FluentProvider & webLightTheme	69.572 kB 19.658 kB
react-components react-components: Accordion, Button, FluentProvider, Image, Menu, Popover	208.392 kB 59.385 kB
react-components react-components: FluentProvider & webLightTheme	40.966 kB 13.569 kB
react-portal-compat PortalCompatProvider	6.541 kB 2.227 kB

_{🤖 This report was generated against 81d629fbc7206dab2bc2dd4878487a079601f396}

[size-auditor]

Asset size changes

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

Baseline commit: 81d629fbc7206dab2bc2dd4878487a079601f396 (build)

[layershifter]

      onOpenChange={(event, data) => {
        console.log("onOpenChange(): data.type", data.type);
        if (data.type === "escapeKeyDown") {
          event.preventDefault();
        }
      }}

Why you can't just prevent default?

[bsunderhus]

@layershifter Why you can't just prevent default?

Default prevention can't be trusted on all use case scenarios, for example on navigation cases the prevention will be required to stop scrolling, and then there's no way you can tell if the prevention means "don't do nothing" or "do something but don't scroll", This problem is present in #29091

1. So in order to maintain a single way of doing things over the library I believe we shouldn't trust default prevention anywhere.
2. Also, preventing default on click events might cause some unexpected behaviours on applications that listen/capture event clicks globally

[layershifter]

To summarize:

• .preventDefault() works in current scenario
• you are proposing a new way to prevent library's behavior that should be consistent

Is it correct?

[bsunderhus]

To summarize:

• .preventDefault() works in current scenario
• you are proposing a new way to prevent library's behavior that should be consistent

Is it correct?

Correct, it works in current scenario but it has never been documented and it's not an official solution. We don't have an official solution for an opt-out mechanism, at the moment if you want to opt-out you have to control the state (in the majority of v9 components).

I'll convert this back into a draft, as I believe this mechanism should be better discussed.

[layershifter]

I'll convert this back into a draft, as I believe this mechanism should be better discussed.

Agree, would like to write RFC with proposed approaches?