Skip to content

Conversation

LFDanLu
Copy link
Member

@LFDanLu LFDanLu commented Sep 12, 2025

Also attempts to replace Dialog in S2 Popover so users trying to recreate a Combobox/Autocomplete like experience can simply use Popover

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Autocomplete behavior should largely be the same as it already is on main, simply sanity check that aria-activedescendant only appears when the wrapped collection support virtual focus. For S2 components that used to use Popover (ComboBox, ContextualHelp, DatePicker, Menu, Picker, TabsPicker) check that their styling and behavior haven't changed with the refactor.

🧢 Your Project:

RSP

// moving focus back to the subtriggers
let isMobileScreenReader = getInteractionModality() === 'virtual' && (isIOS() || isAndroid());
let shouldUseVirtualFocus = !isMobileScreenReader && !disableVirtualFocus;
let [shouldUseVirtualFocus, setShouldUseVirtualFocus] = useState(!isMobileScreenReader && !disableVirtualFocus);
Copy link
Member Author

Choose a reason for hiding this comment

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

We still want to keep disableVirtualFocus as a prop so a user can opt out of the virtual focus behavior based on their use case

Comment on lines 656 to 659
// TODO: Unfortunately can't override via styles prop
// need to unset the overflow otherwise we get two scroll bars
padding: 0,
overflow: 'unset'
Copy link
Member Author

@LFDanLu LFDanLu Sep 12, 2025

Choose a reason for hiding this comment

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

A bit of gross, but needed if we want to completely migrate away from PopoverBase and replace it with Popover now that it directly uses the dialog rendered by RAC Popover. Also a bit annoying that we need to remember to override these...

Comment on lines +106 to +107
// TODO: Unfortunately, we can't pass these styles via the styles prop
// since we don't want to actually allow modifying width and padding for the popover...
Copy link
Member Author

Choose a reason for hiding this comment

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

Since Popover shouldn't freely allow a user to modify its width and padding, there isn't a great way to go about setting those overrides for components using PopoverBase like ContextualHelp...

Copy link
Member Author

Choose a reason for hiding this comment

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

For context, the Popover used to render a Dialog for the user, but this meant Dialog specific behavior like auto focusing the Dialog would make it impossible for a user to use the S2 Popover for a custom Combobox. We've opted to instead rely on RAC Popover applying a "dialog" role instead since users don't have access to PopoverBase

let contextProps;
[contextProps] = useContextProps({}, null, SelectableCollectionContext);
let {filter, ...collectionProps} = contextProps;
[props, ref] = useContextProps(props, ref, SelectableCollectionContext);
Copy link
Member Author

Choose a reason for hiding this comment

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

we now use the ref from useAutocomplete so the Autocomplete input knows it has been connected to a collection element it can filter

@LFDanLu LFDanLu marked this pull request as ready for review September 16, 2025 18:29
@rspbot
Copy link

rspbot commented Sep 16, 2025

@rspbot
Copy link

rspbot commented Sep 16, 2025

});

let turnOffVirtualFocus = useCallback(() => {
setShouldUseVirtualFocus(false);
Copy link
Member

Choose a reason for hiding this comment

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

This probably needs to stopPropagation so that nested collections don't accidentally tell one farther up as well

@rspbot
Copy link

rspbot commented Sep 23, 2025

Comment on lines 259 to 267
// TODO: this moves the ref to the dialog, but the styles all go on the inner div since that is what needs overflow: auto to avoid hiding the popover arrow
// Kinda weird since we usually put everything on the top level element
// TODO: the combobox many items story is odd, it doesn't shift itself over to be properly aligned with the trigger like it does on main
<PopoverBase {...otherProps} ref={domRef}>
{composeRenderProps(props.children, (children) => (
<div
style={UNSAFE_style}
className={(UNSAFE_className || '') + mergeStyles(innerDivStyle, styles)}>
{/* Reset OverlayTriggerStateContext so the buttons inside the dialog don't retain their hover state. */}
Copy link
Member Author

Choose a reason for hiding this comment

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

ugh this isn't quite right either...

  • the TabsPicker want to sent a negative margin left to offset the popover's position (aka style provided via props wants to go to popover base NOT the inner div)
  • Popover wants the inner div to have the padding and overflow set so that it handles scrolling (aka these styles need to be on the inner div). Also needs the overflow set on inner div so the popover arrow isn't hidden
  • Combobox/components with listbox within their Popovers want the padding/overflow on the inner div to be unset since the ListBox already has padding/overflow handling out of the box (aka these styles need to go to the inner div)

@rspbot
Copy link

rspbot commented Sep 23, 2025

@rspbot
Copy link

rspbot commented Sep 23, 2025

@rspbot
Copy link

rspbot commented Sep 23, 2025

## API Changes

react-aria-components

/react-aria-components:SelectionIndicator

-SelectionIndicator {
-  children?: ChildrenOrFunction<SharedElementRenderProps>
-  className?: ClassNameOrFunction<SharedElementRenderProps>
-  isSelected?: boolean
-  style?: StyleOrFunction<SharedElementRenderProps>
-}

/react-aria-components:SelectionIndicatorContext

-SelectionIndicatorContext {
-  UNTYPED
-}

/react-aria-components:SharedElementTransition

-SharedElementTransition {
-  children: ReactNode
-}

/react-aria-components:SharedElement

-SharedElement {
-  children?: ChildrenOrFunction<SharedElementRenderProps>
-  className?: ClassNameOrFunction<SharedElementRenderProps>
-  isVisible?: boolean
-  name: string
-  style?: StyleOrFunction<SharedElementRenderProps>
-}

/react-aria-components:SelectionIndicatorProps

-SelectionIndicatorProps {
-  children?: ChildrenOrFunction<SharedElementRenderProps>
-  className?: ClassNameOrFunction<SharedElementRenderProps>
-  isSelected?: boolean
-  style?: StyleOrFunction<SharedElementRenderProps>
-}

/react-aria-components:SharedElementTransitionProps

-SharedElementTransitionProps {
-  children: ReactNode
-}

/react-aria-components:SharedElementProps

-SharedElementProps {
-  children?: ChildrenOrFunction<SharedElementRenderProps>
-  className?: ClassNameOrFunction<SharedElementRenderProps>
-  isVisible?: boolean
-  name: string
-  style?: StyleOrFunction<SharedElementRenderProps>
-}

/react-aria-components:SharedElementRenderProps

-SharedElementRenderProps {
-  isEntering: boolean
-  isExiting: boolean
-}

/react-aria-components:SelectableCollectionContext

+SelectableCollectionContext {
+  UNTYPED
+}

/react-aria-components:FieldInputContext

+FieldInputContext {
+  UNTYPED
+}

/react-aria-components:SelectableCollectionContextValue

+SelectableCollectionContextValue <T> {
+  aria-describedby?: string
+  aria-details?: string
+  aria-label?: string
+  aria-labelledby?: string
+  disallowTypeAhead?: boolean
+  filter?: (string, Node<T>) => boolean
+  id?: string
+  shouldUseVirtualFocus?: boolean
+}

@react-aria/disclosure

/@react-aria/disclosure:useDisclosure

 useDisclosure {
   props: AriaDisclosureProps
   state: DisclosureState
-  ref: RefObject<HTMLElement | null>
+  ref: RefObject<Element | null>
   returnVal: undefined
 }

@react-aria/utils

/@react-aria/utils:willOpenKeyboard

-willOpenKeyboard {
-  target: Element
-  returnVal: undefined
-}

/@react-aria/utils:DISALLOW_VIRTUAL_FOCUS

+DISALLOW_VIRTUAL_FOCUS {
+  UNTYPED
+}

@react-spectrum/s2

/@react-spectrum/s2:Dialog

 Dialog {
   UNSAFE_className?: UnsafeClassName
   UNSAFE_style?: CSSProperties
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
   children?: ReactNode | (DialogRenderProps) => ReactNode
   id?: string
   isDismissible?: boolean
   isKeyboardDismissDisabled?: boolean
   role?: 'dialog' | 'alertdialog' = 'dialog'
-  size?: 'S' | 'M' | 'L' | 'XL' = 'M'
+  size?: 'S' | 'M' | 'L' = 'M'
   slot?: string | null
   styles?: StylesProp
 }

/@react-spectrum/s2:Popover

 Popover {
   UNSAFE_className?: UnsafeClassName
   UNSAFE_style?: CSSProperties
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
-  children?: ReactNode | (DialogRenderProps) => ReactNode
+  children?: ChildrenOrFunction<PopoverRenderProps>
   containerPadding?: number = 12
   crossOffset?: number = 0
+  disableInnerDiv?: boolean
   hideArrow?: boolean = false
   id?: string
   isOpen?: boolean
   offset?: number = 8
   placement?: Placement = 'bottom'
   role?: 'dialog' | 'alertdialog' = 'dialog'
   shouldFlip?: boolean = true
   size?: 'S' | 'M' | 'L'
   slot?: string | null
   styles?: StylesProp
   triggerRef?: RefObject<Element | null>
 }

/@react-spectrum/s2:DialogProps

 DialogProps {
   UNSAFE_className?: UnsafeClassName
   UNSAFE_style?: CSSProperties
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
   children?: ReactNode | (DialogRenderProps) => ReactNode
   id?: string
   isDismissible?: boolean
   isKeyboardDismissDisabled?: boolean
   role?: 'dialog' | 'alertdialog' = 'dialog'
-  size?: 'S' | 'M' | 'L' | 'XL' = 'M'
+  size?: 'S' | 'M' | 'L' = 'M'
   slot?: string | null
   styles?: StylesProp
 }


let {
children,
disableInnerDiv,
Copy link
Member Author

Choose a reason for hiding this comment

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

super gross, maybe we just export PopoverBase as something like CustomPopover to mirror CustomDialog

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

Successfully merging this pull request may close these issues.

3 participants