Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Automatically render popovers as dialogs #7813

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

devongovett
Copy link
Member

@devongovett devongovett commented Feb 21, 2025

Developers have found it confusing when they need to render a <Dialog> inside a <Popover>, and when they don't. This is becoming even more complicated with Autocomplete, which enables patterns like searchable menus, submenus, and selects. This PR makes RAC popovers automatically render with role=dialog when a <Dialog> is not rendered inside them.

Behavior changes

  • This always renders (modal) popovers as dialogs, so all menus, select list boxes, etc. are wrapped in a dialog. We need to test how this behaves with screen readers to ensure the announcements aren't too much more verbose or confusing.
  • Selects and Menus now contain focus. This means you can no longer tab out of them. That matches macOS native behavior.
  • Opening a submenu with the right arrow key will autofocus the first menu item within the dialog (if any) the same way it does with regular submenus. Is this a problem?
  • This removes SubDialogTrigger and merges it into SubmenuTrigger.

focusSafely(ref.current);
}
}, [isDialog, ref]);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to decide whether to do this in the hooks or keep the independent.

@rspbot
Copy link

rspbot commented Feb 21, 2025

@rspbot
Copy link

rspbot commented Feb 21, 2025

## API Changes

react-aria-components

/react-aria-components:UNSTABLE_SubDialogTrigger

-UNSTABLE_SubDialogTrigger {
-  children: Array<ReactElement>
-  delay?: number = 200
-}

/react-aria-components:Popover

 Popover {
   UNSTABLE_portalContainer?: Element = document.body
+  aria-describedby?: string
+  aria-details?: string
+  aria-label?: string
+  aria-labelledby?: string
   arrowBoundaryOffset?: number = 0
   boundaryElement?: Element = document.body
   children?: ReactNode | ((PopoverRenderProps & {
     defaultChildren: ReactNode | undefined
   className?: string | ((PopoverRenderProps & {
     defaultClassName: string | undefined
 })) => string
   containerPadding?: number = 12
   crossOffset?: number = 0
   defaultOpen?: boolean
   isEntering?: boolean
   isExiting?: boolean
   isKeyboardDismissDisabled?: boolean = false
   isNonModal?: boolean
   isOpen?: boolean
   maxHeight?: number
   offset?: number = 8
   onOpenChange?: (boolean) => void
   placement?: Placement = 'bottom'
   scrollRef?: RefObject<Element | null> = overlayRef
   shouldCloseOnInteractOutside?: (Element) => boolean
   shouldFlip?: boolean = true
   shouldUpdatePosition?: boolean = true
   slot?: string | null
   style?: CSSProperties | ((PopoverRenderProps & {
     defaultStyle: CSSProperties
 })) => CSSProperties | undefined
   trigger?: string
   triggerRef?: RefObject<Element | null>
 }

/react-aria-components:PopoverProps

 PopoverProps {
   UNSTABLE_portalContainer?: Element = document.body
+  aria-describedby?: string
+  aria-details?: string
+  aria-label?: string
+  aria-labelledby?: string
   arrowBoundaryOffset?: number = 0
   boundaryElement?: Element = document.body
   children?: ReactNode | ((PopoverRenderProps & {
     defaultChildren: ReactNode | undefined
   className?: string | ((PopoverRenderProps & {
     defaultClassName: string | undefined
 })) => string
   containerPadding?: number = 12
   crossOffset?: number = 0
   defaultOpen?: boolean
   isEntering?: boolean
   isExiting?: boolean
   isKeyboardDismissDisabled?: boolean = false
   isNonModal?: boolean
   isOpen?: boolean
   maxHeight?: number
   offset?: number = 8
   onOpenChange?: (boolean) => void
   placement?: Placement = 'bottom'
   scrollRef?: RefObject<Element | null> = overlayRef
   shouldCloseOnInteractOutside?: (Element) => boolean
   shouldFlip?: boolean = true
   shouldUpdatePosition?: boolean = true
   slot?: string | null
   style?: CSSProperties | ((PopoverRenderProps & {
     defaultStyle: CSSProperties
 })) => CSSProperties | undefined
   trigger?: string
   triggerRef?: RefObject<Element | null>
 }

@react-aria/overlays

/@react-aria/overlays:Overlay

 Overlay {
   children: ReactNode
   disableFocusManagement?: boolean
   isExiting?: boolean
   portalContainer?: Element = document.body
+  shouldContainFocus?: boolean
 }

/@react-aria/overlays:OverlayProps

 OverlayProps {
   children: ReactNode
   disableFocusManagement?: boolean
   isExiting?: boolean
   portalContainer?: Element = document.body
+  shouldContainFocus?: boolean
 }

@react-spectrum/s2

/@react-spectrum/s2:PopoverProps

 PopoverProps {
   UNSAFE_className?: string
   UNSAFE_style?: CSSProperties
   UNSTABLE_portalContainer?: Element = document.body
+  aria-describedby?: string
+  aria-details?: string
+  aria-label?: string
+  aria-labelledby?: string
   boundaryElement?: Element = document.body
   children?: ReactNode | ((PopoverRenderProps & {
     defaultChildren: ReactNode | undefined
 })) => ReactNode
     defaultClassName: string | undefined
 })) => string
   containerPadding?: number = 12
   crossOffset?: number = 0
   defaultOpen?: boolean
   hideArrow?: boolean = false
   isEntering?: boolean
   isExiting?: boolean
   isOpen?: boolean
   maxHeight?: number
   offset?: number = 8
   onOpenChange?: (boolean) => void
   placement?: Placement = 'bottom'
   scrollRef?: RefObject<Element | null> = overlayRef
   shouldFlip?: boolean = true
   size?: 'S' | 'M' | 'L'
   slot?: string | null
   style?: CSSProperties | ((PopoverRenderProps & {
     defaultStyle: CSSProperties
 })) => CSSProperties | undefined
   styles?: StyleString
   trigger?: string
   triggerRef?: RefObject<Element | null>
 }

@@ -877,7 +877,7 @@ export const AriaAutocompleteTests = ({renderers, setup, prefix, ariaPattern = '
let {getByRole, getAllByRole} = (renderers.subdialogs!)();
let menu = getByRole('menu');
let options = within(menu).getAllByRole('menuitem');
expect(options[1]).toHaveAttribute('aria-haspopup', 'dialog');
expect(options[1]).toHaveAttribute('aria-haspopup', 'menu');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now we don't know if the submenu will be a dialog or not, or rather, they are all dialogs? I didn't notice any announcement differences in real screen readers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants