feat(react-dialog): supports 1st rule of ARIA by bsunderhus · Pull Request #24525 · microsoft/fluentui

bsunderhus · 2022-08-25T12:51:49Z

Current Behavior

Currently Dialog doesn't support 1st rule of ARIA

New Behavior

Supports as="dialog" at DialogSurface by default with possibility to opt-out of semantic element with as="div"
Removes document listeners since they're not required to control Escape 🚀
::backdrop support for native element

fabricteam · 2022-08-25T12:59:30Z

📊 Bundle size report

Package & Exports	Baseline (minified/GZIP)	PR	Change
`react-dialog` Dialog (including children components)	`80.608 kB` `24.053 kB`	`81.048 kB` `24.208 kB`	`440 B` `155 B`

Unchanged fixtures

Package & Exports	Size (minified/GZIP)
`react-components` react-components: Accordion, Button, FluentProvider, Image, Menu, Popover	`187.656 kB` `51.96 kB`
`react-components` react-components: FluentProvider & webLightTheme	`33.359 kB` `11.004 kB`
`react-portal-compat` PortalCompatProvider	`5.851 kB` `1.964 kB`

_{🤖 This report was generated against 9e604a18ef914090301b4d0cd0b602f646f5719c}

size-auditor · 2022-08-25T13:05:47Z

Asset size changes

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

Baseline commit: 9e604a18ef914090301b4d0cd0b602f646f5719c (build)

fabricteam · 2022-08-25T13:08:27Z

Perf Analysis (`@fluentui/react-components`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
Avatar	mount	1329	1299	5000
Button	mount	943	963	5000
FluentProvider	mount	1571	1553	5000
FluentProviderWithTheme	mount	624	627	10
FluentProviderWithTheme	virtual-rerender	587	585	10
FluentProviderWithTheme	virtual-rerender-with-unmount	625	632	10
MakeStyles	mount	1913	1903	50000
SpinButton	mount	2498	2525	5000

codesandbox-ci · 2022-08-26T09:57:49Z

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 e698797:

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

Hotell · 2022-08-29T11:15:29Z

not sure if ::backdrop is polyfillable - check our browser support matrix https://github.com/microsoft/fluentui/pull/24283/files

bsunderhus · 2022-08-29T11:16:35Z

not sure if ::backdrop is polyfillable - check our browser support matrix https://github.com/microsoft/fluentui/pull/24283/files

according to our support matrix, we support native ::backdrop, not sure about mobile browsers though. We have as="div" to support non native dialog on those cases 😁,

Hotell · 2022-08-29T12:37:08Z

not sure if ::backdrop is polyfillable - check our browser support matrix https://github.com/microsoft/fluentui/pull/24283/files

according to our support matrix, we support native ::backdrop, not sure about mobile browsers though. We have as="div" to support non native dialog on those cases 😁,

I'm concerned for desktop safari 13 in particular

👀

bsunderhus · 2022-08-29T12:39:32Z

I hear you @Hotell, the most straight solution would be to use our own backdrop slot on those scenarios. I'll come up with a way to use our backdrop slot even when the native element is being used!

smhigley · 2022-08-31T17:34:13Z

Is there a benefit to using the native dialog element? I'd actually lean against it, since behaviors are still being ironed out. In particular, support for modeless dialogs and initial focus behavior are still being discussed (see https://github.com/whatwg/html/wiki/dialog--initial-focus,-a-proposal and whatwg/html#7707)

There's also the issue that only Safari 15.4+ supports <dialog>, and while we could polyfill parts of it, I'm not sure what the benefit is. The 1st rule of ARIA is a good rule of thumb, but its primary purpose is to encourage using tools that have broad browser support, and discourage fragile JS & ARIA solutions. In this case, custom ARIA & JS is actually more robust and better-supported than the native element, so I think we should stick with that.

Scott's dialog article has more or less the same recommendation (last updated March 2022): https://www.scottohara.me/blog/2019/03/05/open-dialog.html. He's been much more involved in the <dialog> discussions than I have though; I can ask him to give his current opinion if desired.

bsunderhus · 2022-08-31T19:03:54Z

Thanks for the comment @smhigley, that's definitely a valid question!

The initial focus behaviour wouldn't be an issue, as focus is handled internally no matter if a <dialog> is being rendered or a <div>, and this PR also includes e2e test to ensure that.

I'd love to hear more about it if you could get Scott's opinion on the usage of <dialog> nowadays, the support tables on caniuse indicated a more concise support.

I guess the biggest benefit would be to ensure best native behaviour in cases where a browser actually supports <dialog>, but if that support is not as conclusive as it seemed to be in support tables matrixes, I'll have to review this whole idea!

smhigley · 2022-08-31T23:05:58Z

After talking to Scott, I'm now more ambivalent 😅. Having <dialog> handle the aria-hidden/inert stuff would be really nice, since that was a giant pain in v8. I think my main points of concern are still:

I believe we still need a polyfill for modal behavior for Safari
I think the non-modal version should not default to <dialog> -- not sure how we type that or set up slots for having the element type be conditional based on a prop
Weirdly enough, this is relevant for our live region strategy. That's maybe a bigger topic and I need to do some testing, but maybe we should have a chat about it? Essentially, if we end up with some sort of Announced-like component, we may need to figure out whether <dialog> making everything else inert interferes with that approach, and if there are ways around that.

bsunderhus · 2022-09-01T07:33:16Z

Having handle the aria-hidden/inert stuff would be really nice, since that was a giant pain in v8

Well, the approach on v9 so far for inert behaviour is dealt by tabster, so I wouldn't say that this is a problem. But again, I would prefer to have this dealt by native behaviour

I believe we still need a polyfill for modal behavior for Safari

Fair enough, as @Hotell has already pointed for the ::backdrop also. I'll have to cover that on another PR though, this one is too big already.

I think the non-modal version should not default to <dialog> -- not sure how we type that or set up slots for having the element type be conditional based on a prop

Can you elaborate on why @smhigley? for the tests I've done so far (visually), on major browsers all of them work fine with non-modal native <dialog> using the HTMLDialogElement.show method. Nonetheless if this is also a must this can be easily done internally by the DialogSurface component, nothing to worry.

Weirdly enough, this is relevant for our live region strategy. That's maybe a bigger topic and I need to do some testing, but maybe we should have a chat about it? Essentially, if we end up with some sort of Announced-like component, we may need to figure out whether <dialog> making everything else inert interferes with that approach, and if there are ways around that.

Yeah, this definitely didn't cross my mind, I've already noticed some difficulties while trying to substitute ::backdrop on a modal native <dialog>. It's simply not possible due to inert. But definitely let's have a chat about that!

ling1726 · 2022-09-05T14:59:14Z

does safari <15 support <dialog> element without the inert and focus behaviours ? i.e. will it at least render like a normal div in the browser ?

bsunderhus · 2022-09-06T09:15:23Z

does safari <15 support <dialog> element without the inert and focus behaviours ? i.e. will it at least render like a normal div in the browser ?

Yes it does work @ling1726! Only problem right now is regarding ::backdrop support on safari v13

bsunderhus · 2022-09-06T09:15:42Z

This PR got a little bigger than I expected, I'll try to split it

bsunderhus · 2022-09-07T12:27:33Z

I'll keep this PR as draft and blocked meanwhile there are still one major discussion to be clarified regardingliveregion usage with native dialog.

bsunderhus · 2022-09-08T07:03:43Z

Ok, after some testing and a few discussions, there's one main scenario (and Safari) that is troubling right now with native dialogs:

`liveregion` narration

here's a simple reproduction of the problem. When a native dialog is opened every other element in the page works as if they were inert, causing changes to be ignored on liveregion outside the dialog.

I could spend a few paragraphs here explaining that this shouldn't be much of a problem because dialogs are modal content and therefore they should be used sparingly and they should present small content to be dismissed as soon as possible!

But of course... this is the real world and every once in a while you'll see something like this:

This is still a problem with `tabster` and non native dialog with `aria-hidden`

Here's the catch though: This is not a dialog problem itself, any inert (the same is valid for aria-hidden with tabster) element will not be narrated, the problem here is our lack of control over where the liveregion is located.

@smhigley proposed that we create a notification management system to ensure a liveregion is always available through the application for components that require it, such as Alert. That way no aria-hidden or inert attribute would be a problem.

ling1726 · 2022-09-12T15:33:02Z

packages/react-components/react-dialog/src/components/DialogSurface/useDialogSurface.ts

  const dialogBodyID = useDialogContext_unstable(ctx => ctx.dialogBodyID);
  const isNestedDialog = useDialogContext_unstable(ctx => ctx.isNestedDialog);

+  const handleNativeClick = useEventCallback((event: React.MouseEvent<DialogSurfaceElementIntersection>) => {


nit: you could probably return the native control hooks from another hook

layershifter

Supports as="dialog" at DialogSurface by default with possibility to opt-out of semantic element with as="div"

Sorry, but I don't understand this. Our current browser support matrix starts with Safari 14.1 while dialog works only in Safari 15.4 (according to MDN). What is expected from developers in a product if they support Safari 14.3? Go to every DialogSurface and specify as="div"?

layershifter · 2022-09-13T10:12:54Z

change/@fluentui-react-dialog-dc310d89-6297-481f-a1fa-2a25450bc15a.json

@@ -0,0 +1,7 @@
+{
+  "type": "prerelease",
+  "comment": "feat(react-dialog): 1st rule of ARIA for Dialog",


Suggested change

"comment": "feat(react-dialog): 1st rule of ARIA for Dialog",

"comment": "feat(react-dialog): use native dialog element",

I'll be addressing any Safari related issues on a separate PR, since some cases need investigation @layershifter, but in the worst case scenario of not getting native <dialog>to work out of of the box then I'll have to default to divand add dialog as an option

packages/react-components/react-dialog/src/components/Dialog/Dialog.types.ts

layershifter · 2022-09-13T10:31:28Z

packages/react-components/react-dialog/src/stories/Dialog/DialogNoFocusableElement.stories.tsx

          <DialogBody>
            <p>⛔️ A Dialog without focusable elements is not recommended!</p>
-            <p>⛔️ Escape key doesn't work here</p>
+            <p>⚠️ Escape key only works with native dialog</p>


Why do we want to have this limitation? Is not it better to align implementations?

Well, this limitation is due to the fact this a very specific edge case scenario that should not be implemented by the user.

Aligning implementations would require listening to events on the document itself and filtering out the case where Escape might do something else, like closing a Menu or a Popover. We could discuss adding this again for treating this very specific edge case, but this has already been removed on a previous PR actually #24668

If I correctly understand the goal: div & dialog should be replaceable. So it would be great to align behaviors. Otherwise we could get a different behaviors across browsers that might be confusing.

The argument here is... do we care for an incompatible behaviour on a very obscure edge case? is it worth adding a global listener that will not follow React's typing API to just resolve that very specific edge case?!

another alternative would be to stop support Escape key to work on native dialog for this scenario also. @layershifter

another alternative would be to stop support Escape key to work on native dialog for this scenario also. @layershifter

@bsunderhus It's also an option 👍 I think that we need to normalize behavior anyway, but it's your choice what to choose 😉

layershifter · 2022-09-13T10:35:41Z

packages/react-components/react-dialog/src/utils/isHTMLDialogElement.ts

+  /**
+   * The close event is fired on a <dialog> when it has been closed.
+   */
+  onClose?: (event: React.SyntheticEvent) => void;
+  /**
+   * The cancel event fires on a <dialog> when
+   * the user instructs the browser that they wish to dismiss the current open dialog.
+   * For example, the browser might fire this event when the user presses the Esc
+   * key.
+   */
+  onCancel?: (event: React.SyntheticEvent) => void;


I suggest to bump @types/react to 17.0.49 instead:

(may be in a follow up PR)

layershifter · 2022-09-13T10:36:51Z

packages/react-components/react-dialog/src/components/DialogSurface/useDialogSurfaceStyles.ts

    ...shorthands.borderStyle('none'),
    ...shorthands.overflow('unset'),
+    '&::backdrop': {
+      backgroundColor: 'rgba(0, 0, 0, 0.4)',


Should be this a token?

I'll reach design team and ask about it

layershifter · 2022-09-13T10:40:53Z

packages/react-components/react-dialog/src/components/DialogSurface/useDialogSurface.ts

  const dialogTitleID = useDialogContext_unstable(ctx => ctx.dialogTitleID);
  const dialogBodyID = useDialogContext_unstable(ctx => ctx.dialogBodyID);


Suggested change

const dialogTitleID = useDialogContext_unstable(ctx => ctx.dialogTitleID);

const dialogBodyID = useDialogContext_unstable(ctx => ctx.dialogBodyID);

const dialogTitleId = useDialogContext_unstable(ctx => ctx.dialogTitleId);

const dialogBodyId = useDialogContext_unstable(ctx => ctx.dialogBodyId);

nit: it's out of scope of this PR, but let's fix casing to be consistent

* master: (28 commits) Fix value font-weight inside heatmap chart (microsoft#24726) Fix legend overflow-indication-text role (microsoft#24756) Support custom locale in date axis (microsoft#24753) Cleanup env variables (microsoft#24739) ci(github): add GH Action to add issue labels based on new GH issue template (microsoft#24788) Update disallowedChangeTypes for newly created packages, to allow only 'prerelease' change types by default (microsoft#24763) feat(react-components): Adding missing AvatarGroup exports (microsoft#24770) remove unnecessary nohoist (microsoft#24760) feat(react-dialog): supports 1st rule of ARIA (microsoft#24525) BREAKING: TableCell layouts are handled by layout components (microsoft#24762) feat: Implement table cell layout components (microsoft#24773) applying package updates fix: remove readonly from DetailsList (microsoft#24615) chore: Cleaning up tokens in Button components so they better adhere to the design spec (microsoft#24732) fix: react-combobox listbox popup width matches trigger width (microsoft#24733) fix: react-combobox Option focus outline only shows with keyboard nav (microsoft#24700) feat: Publish react-field package, and export from react-components/unstable (microsoft#24235) fix: Replacing bottom border styles with text decoration underline in Link (microsoft#24734) docs(react-theme): Update readme (microsoft#24755) Add tests for hover states (microsoft#24390) ...

bsunderhus added Component: Dialog Fluent UI react-components (v9) labels Aug 25, 2022

bsunderhus self-assigned this Aug 25, 2022

github-actions bot added this to the July Project Cycle Q3 2022 milestone Aug 25, 2022

bsunderhus force-pushed the react-dialog-first-rule-of-aria branch from 6638c42 to 5afee67 Compare August 26, 2022 08:26

bsunderhus force-pushed the react-dialog-first-rule-of-aria branch 2 times, most recently from f7b63fd to b0338e1 Compare August 26, 2022 21:32

bsunderhus marked this pull request as ready for review August 26, 2022 21:33

bsunderhus requested review from a team as code owners August 26, 2022 21:33

bsunderhus force-pushed the react-dialog-first-rule-of-aria branch from b0338e1 to 2c82768 Compare August 31, 2022 16:40

bsunderhus force-pushed the react-dialog-first-rule-of-aria branch from 5a62f90 to ea19f45 Compare September 6, 2022 07:54

bsunderhus marked this pull request as draft September 6, 2022 09:15

bsunderhus mentioned this pull request Sep 6, 2022

feat(react-dialog): moves backdrop slot from Dialog to DialogSurface #24669

Merged

bsunderhus force-pushed the react-dialog-first-rule-of-aria branch from ea19f45 to 52468fd Compare September 7, 2022 11:25

bsunderhus marked this pull request as ready for review September 7, 2022 12:24

bsunderhus added the Status: Blocked Resolution blocked by another issue label Sep 7, 2022

bsunderhus marked this pull request as draft September 7, 2022 12:27

bsunderhus mentioned this pull request Sep 7, 2022

feat(react-utilities): add dialog properties to getNativeElementProps #24698

Merged

bsunderhus force-pushed the react-dialog-first-rule-of-aria branch 2 times, most recently from 6683da8 to cd7335b Compare September 8, 2022 06:39

bsunderhus marked this pull request as ready for review September 8, 2022 07:06

bsunderhus removed the Status: Blocked Resolution blocked by another issue label Sep 8, 2022

bsunderhus force-pushed the react-dialog-first-rule-of-aria branch from d83de7b to 871b334 Compare September 9, 2022 13:06

bsunderhus added 2 commits September 9, 2022 15:11

feat(react-dialog): 1st rule of ARIA for Dialog

a902c99

chore: delete useDialogProps

e698797

bsunderhus force-pushed the react-dialog-first-rule-of-aria branch from 871b334 to e698797 Compare September 9, 2022 15:11

ling1726 approved these changes Sep 12, 2022

View reviewed changes

ling1726 reviewed Sep 12, 2022

View reviewed changes

bsunderhus merged commit fd88d8b into microsoft:master Sep 13, 2022

bsunderhus deleted the react-dialog-first-rule-of-aria branch September 13, 2022 11:22

layershifter reviewed Sep 13, 2022

View reviewed changes

bsunderhus mentioned this pull request Sep 27, 2022

feat(react-dialog): removes DialogSurface native dialog support #24979

Merged

	"comment": "feat(react-dialog): 1st rule of ARIA for Dialog",
	"comment": "feat(react-dialog): use native dialog element",

		const dialogTitleID = useDialogContext_unstable(ctx => ctx.dialogTitleID);
		const dialogBodyID = useDialogContext_unstable(ctx => ctx.dialogBodyID);

Conversation

bsunderhus commented Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current Behavior

New Behavior

Uh oh!

fabricteam commented Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Bundle size report

Uh oh!

size-auditor bot commented Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Asset size changes

Uh oh!

fabricteam commented Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codesandbox-ci bot commented Aug 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hotell commented Aug 29, 2022

Uh oh!

bsunderhus commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hotell commented Aug 29, 2022

Uh oh!

bsunderhus commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smhigley commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsunderhus commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smhigley commented Aug 31, 2022

Uh oh!

bsunderhus commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ling1726 commented Sep 5, 2022

Uh oh!

bsunderhus commented Sep 6, 2022

Uh oh!

bsunderhus commented Sep 6, 2022

Uh oh!

bsunderhus commented Sep 7, 2022

Uh oh!

bsunderhus commented Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

liveregion narration

This is still a problem with tabster and non native dialog with aria-hidden

Uh oh!

Choose a reason for hiding this comment

Uh oh!

layershifter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsunderhus Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsunderhus commented Aug 25, 2022 •

edited

Loading

fabricteam commented Aug 25, 2022 •

edited

Loading

size-auditor bot commented Aug 25, 2022 •

edited

Loading

fabricteam commented Aug 25, 2022 •

edited

Loading

codesandbox-ci bot commented Aug 26, 2022 •

edited

Loading

bsunderhus commented Aug 29, 2022 •

edited

Loading

bsunderhus commented Aug 29, 2022 •

edited

Loading

smhigley commented Aug 31, 2022 •

edited

Loading

bsunderhus commented Aug 31, 2022 •

edited

Loading

bsunderhus commented Sep 1, 2022 •

edited

Loading

bsunderhus commented Sep 8, 2022 •

edited

Loading

`liveregion` narration

This is still a problem with `tabster` and non native dialog with `aria-hidden`

bsunderhus Sep 14, 2022 •

edited

Loading