From cda82a1620e9beca423487fe5e3b5e677b22bcc1 Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Thu, 19 Sep 2024 10:16:33 -0400 Subject: [PATCH 1/8] Add `role="dialog", focus trap to `Overlay` --- packages/react/src/Overlay/Overlay.tsx | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/react/src/Overlay/Overlay.tsx b/packages/react/src/Overlay/Overlay.tsx index 251ba0625ab..f94eedb6df1 100644 --- a/packages/react/src/Overlay/Overlay.tsx +++ b/packages/react/src/Overlay/Overlay.tsx @@ -13,6 +13,7 @@ import {useRefObjectAsForwardedRef} from '../hooks/useRefObjectAsForwardedRef' import type {AnchorSide} from '@primer/behaviors' import {useTheme} from '../ThemeProvider' import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' +import {useFocusTrap} from '../hooks/useFocusTrap' type StyledOverlayProps = { width?: keyof typeof widthMap @@ -138,7 +139,7 @@ const Overlay = React.forwardRef( ( { onClickOutside, - role = 'none', + role, initialFocusRef, returnFocusRef, ignoreClickRefs, @@ -200,12 +201,16 @@ const Overlay = React.forwardRef( // To be backwards compatible with the old Overlay, we need to set the left prop if x-position is not specified const leftPosition: React.CSSProperties = left === undefined && right === undefined ? {left: 0} : {left} + const {containerRef} = useFocusTrap({ + containerRef: overlayRef, // only if `role="dialog"`, `aria-modal="true"` is true + }) + return ( Date: Thu, 19 Sep 2024 10:36:18 -0400 Subject: [PATCH 2/8] Add conditional, new prop --- packages/react/src/Overlay/Overlay.tsx | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/react/src/Overlay/Overlay.tsx b/packages/react/src/Overlay/Overlay.tsx index f94eedb6df1..2982fabc873 100644 --- a/packages/react/src/Overlay/Overlay.tsx +++ b/packages/react/src/Overlay/Overlay.tsx @@ -110,6 +110,7 @@ type BaseOverlayProps = { portalContainerName?: string preventFocusOnOpen?: boolean role?: AriaRole + focusTrap?: boolean children?: React.ReactNode } @@ -134,6 +135,7 @@ type OwnOverlayProps = Merge * @param bottom Optional. Vertical bottom position of the overlay, relative to its closest positioned ancestor (often its `Portal`). * @param position Optional. Sets how an element is positioned in a document. Defaults to `absolute` positioning. * @param portalContainerName Optional. The name of the portal container to render the Overlay into. + * @param focusTrap Optional. Determines if the `Overlay` recieves a focus trap or not. Defaults to `true`. */ const Overlay = React.forwardRef( ( @@ -156,6 +158,7 @@ const Overlay = React.forwardRef( preventFocusOnOpen, position, style: styleFromProps = {}, + focusTrap = true, ...rest }, forwardedRef, @@ -201,8 +204,11 @@ const Overlay = React.forwardRef( // To be backwards compatible with the old Overlay, we need to set the left prop if x-position is not specified const leftPosition: React.CSSProperties = left === undefined && right === undefined ? {left: 0} : {left} - const {containerRef} = useFocusTrap({ + const dialog = role && role !== 'dialog' ? true : false + + useFocusTrap({ containerRef: overlayRef, // only if `role="dialog"`, `aria-modal="true"` is true + disabled: !focusTrap || dialog, }) return ( @@ -211,6 +217,7 @@ const Overlay = React.forwardRef( height={height} width={width} role={role || 'dialog'} + aria-modal={dialog ? undefined : 'true'} {...rest} ref={overlayRef} style={ From 3fa19049f86677abe2857afa51c9947a96545cb3 Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Thu, 19 Sep 2024 13:08:21 -0400 Subject: [PATCH 3/8] Add `role` test for `Overlay` --- packages/react/src/Overlay/Overlay.test.tsx | 25 +++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/packages/react/src/Overlay/Overlay.test.tsx b/packages/react/src/Overlay/Overlay.test.tsx index e83cabfb014..46614c819c7 100644 --- a/packages/react/src/Overlay/Overlay.test.tsx +++ b/packages/react/src/Overlay/Overlay.test.tsx @@ -1,4 +1,4 @@ -import React, {useRef, useState} from 'react' +import React, {useRef, useState, type AriaRole} from 'react' import {Overlay, Box, Text} from '..' import {ButtonDanger, Button} from '../deprecated' import {render, waitFor, fireEvent} from '@testing-library/react' @@ -12,8 +12,9 @@ import {NestedOverlays, MemexNestedOverlays, MemexIssueOverlay, PositionedOverla type TestComponentSettings = { initialFocus?: 'button' callback?: () => void + role?: AriaRole } -const TestComponent = ({initialFocus, callback}: TestComponentSettings) => { +const TestComponent = ({initialFocus, callback, role}: TestComponentSettings) => { const [isOpen, setIsOpen] = useState(false) const buttonRef = useRef(null) const confirmButtonRef = useRef(null) @@ -39,6 +40,7 @@ const TestComponent = ({initialFocus, callback}: TestComponentSettings) => { ignoreClickRefs={[buttonRef]} onEscape={closeOverlay} onClickOutside={closeOverlay} + role={role} width="small" > @@ -281,4 +283,23 @@ describe('Overlay', () => { // if stopPropagation worked, mockHandler would not have been called expect(mockHandler).toHaveBeenCalledTimes(0) }) + + it('should provide `role="dialog"` and `aria-modal="true"`, if role is not provided', async () => { + const user = userEvent.setup() + const {getByText, getByRole} = render() + + await user.click(getByText('open overlay')) + + expect(getByRole('dialog')).toBeInTheDocument() + }) + + it('should not provide `dialog` roles if role is already provided', async () => { + const user = userEvent.setup() + const {getByText, queryByRole} = render() + + await user.click(getByText('open overlay')) + + expect(queryByRole('dialog')).not.toBeInTheDocument() + expect(queryByRole('none')).toBeInTheDocument() + }) }) From 4d4f63bc5817b57ad41b4c713d1a6f34c69a1c8d Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Fri, 20 Sep 2024 14:05:21 -0400 Subject: [PATCH 4/8] Add `role="none"` to `Overlay` in autocomplete --- packages/react/src/Autocomplete/AutocompleteOverlay.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/react/src/Autocomplete/AutocompleteOverlay.tsx b/packages/react/src/Autocomplete/AutocompleteOverlay.tsx index 95d5e479c80..077c526387c 100644 --- a/packages/react/src/Autocomplete/AutocompleteOverlay.tsx +++ b/packages/react/src/Autocomplete/AutocompleteOverlay.tsx @@ -53,6 +53,7 @@ function AutocompleteOverlay({ return showMenu ? ( Date: Fri, 20 Sep 2024 16:40:06 -0400 Subject: [PATCH 5/8] Fix ts error --- packages/react/src/Overlay/Overlay.test.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/react/src/Overlay/Overlay.test.tsx b/packages/react/src/Overlay/Overlay.test.tsx index 46614c819c7..50715fae3e2 100644 --- a/packages/react/src/Overlay/Overlay.test.tsx +++ b/packages/react/src/Overlay/Overlay.test.tsx @@ -1,4 +1,5 @@ -import React, {useRef, useState, type AriaRole} from 'react' +import React, {useRef, useState} from 'react' +import type {AriaRole} from '../utils/types' import {Overlay, Box, Text} from '..' import {ButtonDanger, Button} from '../deprecated' import {render, waitFor, fireEvent} from '@testing-library/react' From d5b5ec9fcf0848f8e81f64ee6606f582f51eb0fb Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Fri, 20 Sep 2024 18:26:10 -0400 Subject: [PATCH 6/8] Add test --- packages/react/src/Overlay/Overlay.test.tsx | 9 +++++++++ packages/react/src/Overlay/Overlay.tsx | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/packages/react/src/Overlay/Overlay.test.tsx b/packages/react/src/Overlay/Overlay.test.tsx index 50715fae3e2..1125be5d866 100644 --- a/packages/react/src/Overlay/Overlay.test.tsx +++ b/packages/react/src/Overlay/Overlay.test.tsx @@ -303,4 +303,13 @@ describe('Overlay', () => { expect(queryByRole('dialog')).not.toBeInTheDocument() expect(queryByRole('none')).toBeInTheDocument() }) + + it('should add `aria-modal` if `role="dialog"` is present', async () => { + const user = userEvent.setup() + const {getByText, queryByRole} = render() + + await user.click(getByText('open overlay')) + + expect(queryByRole('dialog')).toHaveAttribute('aria-modal') + }) }) diff --git a/packages/react/src/Overlay/Overlay.tsx b/packages/react/src/Overlay/Overlay.tsx index 2982fabc873..d6f05a711fb 100644 --- a/packages/react/src/Overlay/Overlay.tsx +++ b/packages/react/src/Overlay/Overlay.tsx @@ -207,7 +207,7 @@ const Overlay = React.forwardRef( const dialog = role && role !== 'dialog' ? true : false useFocusTrap({ - containerRef: overlayRef, // only if `role="dialog"`, `aria-modal="true"` is true + containerRef: overlayRef, disabled: !focusTrap || dialog, }) From 67d1e547be699cd8fc7820e7e2ea363e4eb918bd Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Fri, 20 Sep 2024 18:37:34 -0400 Subject: [PATCH 7/8] Update docs --- packages/react/src/Overlay/Overlay.docs.json | 6 ++++++ packages/react/src/Overlay/Overlay.tsx | 6 +++--- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/react/src/Overlay/Overlay.docs.json b/packages/react/src/Overlay/Overlay.docs.json index 6377595594b..71de564937c 100644 --- a/packages/react/src/Overlay/Overlay.docs.json +++ b/packages/react/src/Overlay/Overlay.docs.json @@ -112,6 +112,12 @@ "defaultValue": "", "description": "If defined, Overlays will be rendered in the named portal. See also `Portal`." }, + { + "name": "focusTrap", + "type": "string", + "defaultValue": "true", + "description": "Determines if the `Overlay` recieves a focus trap or not" + }, { "name": "sx", "type": "SystemStyleObject" diff --git a/packages/react/src/Overlay/Overlay.tsx b/packages/react/src/Overlay/Overlay.tsx index d6f05a711fb..396dafa26c3 100644 --- a/packages/react/src/Overlay/Overlay.tsx +++ b/packages/react/src/Overlay/Overlay.tsx @@ -204,11 +204,11 @@ const Overlay = React.forwardRef( // To be backwards compatible with the old Overlay, we need to set the left prop if x-position is not specified const leftPosition: React.CSSProperties = left === undefined && right === undefined ? {left: 0} : {left} - const dialog = role && role !== 'dialog' ? true : false + const nonDialog = role && role !== 'dialog' ? true : false useFocusTrap({ containerRef: overlayRef, - disabled: !focusTrap || dialog, + disabled: !focusTrap || nonDialog, }) return ( @@ -217,7 +217,7 @@ const Overlay = React.forwardRef( height={height} width={width} role={role || 'dialog'} - aria-modal={dialog ? undefined : 'true'} + aria-modal={nonDialog ? undefined : 'true'} {...rest} ref={overlayRef} style={ From 5627e4f46fcca2f566788268b29c31df695da940 Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Fri, 20 Sep 2024 18:43:23 -0400 Subject: [PATCH 8/8] Add changeset --- .changeset/polite-taxis-sin.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/polite-taxis-sin.md diff --git a/.changeset/polite-taxis-sin.md b/.changeset/polite-taxis-sin.md new file mode 100644 index 00000000000..f40c21d3eb5 --- /dev/null +++ b/.changeset/polite-taxis-sin.md @@ -0,0 +1,7 @@ +--- +'@primer/react': minor +--- + +Overlay: Add `role="dialog"` and `aria-modal="true"` to the `Overlay` component, and implement a focus trap. + +