fix: prevent calling focus on elements within template elements

[LFDanLu]

Closes

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

Navigate to the S2 Picker storybook docs story. Check your console and you shouldn't see "Error: Could not determine window of node. Node was [object HTMLElement]"

Go to S2 Picker docs. Type any description in the first example's description control, it should apply without crashing. Also try going to another page and coming back to the Picker page and adding a description.

🧢 Your Project:

RSP

[rspbot]

Build successful! 🎉

• View the storybook
• View the S2 storybook
• View the S2 docs
• View the React Aria docs

[snowystinger]

Go to S2 Picker docs. Type any description in the first example's description control, it should apply without crashing. Also try going to another page and coming back to the Picker page and adding a description.

I'm unable to reproduce this one on main locally, nor on the latest published main build.

Navigate to the S2 Picker docs story. Check your console and you shouldn't see "Error: Could not determine window of node. Node was [object HTMLElement]"

I can't verify this one locally because i cannot get it to reproduce locally for me, only on the latest published main build. Any other clues?

UPDATE, I got the second one to reproduce in the ContextualHelp Picker story

The error is coming from https://github.com/testing-library/user-event/blob/ebab6c6e81e7022af7afa5aacd07d01626895eb8/src/utils/misc/getWindow.ts#L8

[snowystinger]

Stream of consciousness:
It looks like some sort of issue with testing-library which patches the focus method, that’s where the error comes from. Though we’re calling focus on an element within the template it would appear, specifically the contextual help’s dialog is doing it

I think it’s due to a ref being shared/assigned to both the real and the hidden document

On story load, the hidden copy of the Dialog (from contextual help, a child of RACSelect->FieldLabel->Dialog) is mounted inside the template for collection building.
That mount triggers useDialog’s focus effect.

I’m not sure why this hasn’t been broken for forever… maybe the update to storybook inadvertently updated the user-event plugin which brought in this new patch to focus

I think instead what we should be doing is hiding the FieldLabel from the CollectionBuilder. Rather than making every form field have a dependency on the collection code though, we could wrap it just in the components which need it.

function PickerFieldLabel(props: FieldLabelProps) {
  let isHidden = useIsHidden();
  if (isHidden) {
    return null;
  }
  return <FieldLabel {...props} />;
}

then use that as the child of the AriaSelect

If people add helptext or descriptions with dialogs in them, that could also be an issue, so might need to do it for HelpText as well

Looks like ComboBox needs the same fix

[nwidynski]

@snowystinger Wouldn't it be possible to just add a ref.current instanceof Element check in useDialog?

[snowystinger]

Sure, but it's a little weird, then useDialog suddenly knows it can be rendered into a fake dom which is an implementation detail of our Collections. Also, it's just more things for the CollectionBuilder to go through. Seems better to exclude it. The other concern is if there is any logic on mount in there, then it'll technically run twice, once in the fake dom and once in the real. So again, seems better to only do once.

[LFDanLu]

@snowystinger

Go to S2 Picker docs. Type any description in the first example's description control, it should apply without crashing. Also try going to another page and coming back to the Picker page and adding a description.

I'm unable to reproduce this one on main locally, nor on the latest published main build.

That is correct, you should not be able to reproduce in the main build because #9703 (and #9385) address that issue so you'd have to use a older build. Try https://d1pzu54gtk2aed.cloudfront.net/pr/86324ada420c80fad2393ab6e11e491b60e14140/Picker and lemme know if it reproduces for you.

Navigate to the S2 Picker docs story. Check your console and you shouldn't see "Error: Could not determine window of node. Node was [object HTMLElement]"

Are you navigating to the S2 Picker docs storybook story? I'll make that more clear in the description. Feel free to try https://reactspectrum.blob.core.windows.net/reactspectrum/a9fccbde0f124853d58ba59d35f411130a2bbd2a/storybook-s2/index.html?path=/docs/picker--docs and see if it reproduces

[snowystinger]

approving so we can continue testing with chromatic again while we actually resolve this

[LFDanLu]

This returns false if the element is in a template element as well so perhaps more robust of a check

[rspbot]

Build successful! 🎉

• View the storybook
• View the S2 storybook
• View the S2 docs
• View the React Aria docs

[rspbot]

## API Changes

react-aria-components

/react-aria-components:GridListItemRenderProps

 GridListItemRenderProps {
   allowsDragging?: boolean
-  id?: Key
   isDisabled: boolean
   isDragging?: boolean
   isDropTarget?: boolean
   isFocusVisible: boolean
   isFocused: boolean
   isHovered: boolean
   isPressed: boolean
   isSelected: boolean
   selectionBehavior: SelectionBehavior
   selectionMode: SelectionMode
-  state: ListState<unknown>
 }

---

@react-spectrum/s2

/@react-spectrum/s2:ListView

-ListView <T extends {}> {
-  UNSAFE_className?: UnsafeClassName
-  UNSAFE_style?: CSSProperties
-  aria-describedby?: string
-  aria-details?: string
-  aria-label?: string
-  aria-labelledby?: string
-  autoFocus?: boolean | FocusStrategy
-  children: ReactNode | ({}) => ReactNode
-  defaultSelectedKeys?: 'all' | Iterable<Key>
-  dependencies?: ReadonlyArray<any>
-  disabledBehavior?: DisabledBehavior = "all"
-  disabledKeys?: Iterable<Key>
-  disallowEmptySelection?: boolean
-  disallowTypeAhead?: boolean = false
-  escapeKeyBehavior?: 'clearSelection' | 'none' = 'clearSelection'
-  hideLinkOutIcon?: boolean
-  id?: string
-  isQuiet?: boolean
-  items?: Iterable<T>
-  loadingState?: LoadingState
-  onAction?: (Key) => void
-  onLoadMore?: () => void
-  onSelectionChange?: (Selection) => void
-  overflowMode?: 'wrap' | 'truncate' = 'truncate'
-  renderActionBar?: ('all' | Set<Key>) => ReactElement
-  renderEmptyState?: (GridListRenderProps) => ReactNode
-  selectedKeys?: 'all' | Iterable<Key>
-  selectionMode?: SelectionMode
-  selectionStyle?: 'highlight' | 'checkbox' = 'checkbox'
-  shouldSelectOnPressUp?: boolean
-  slot?: string | null
-  styles?: StylesPropWithHeight
-}

/@react-spectrum/s2:ListViewContext

-ListViewContext {
-  UNTYPED
-}

/@react-spectrum/s2:ListViewItem

-ListViewItem {
-  children: ReactNode
-  download?: boolean | string
-  hasChildItems?: boolean
-  href?: Href
-  hrefLang?: string
-  id?: Key
-  isDisabled?: boolean
-  onAction?: () => void
-  onHoverChange?: (boolean) => void
-  onHoverEnd?: (HoverEvent) => void
-  onHoverStart?: (HoverEvent) => void
-  onPress?: (PressEvent) => void
-  onPressChange?: (boolean) => void
-  onPressEnd?: (PressEvent) => void
-  onPressStart?: (PressEvent) => void
-  onPressUp?: (PressEvent) => void
-  ping?: string
-  referrerPolicy?: HTMLAttributeReferrerPolicy
-  rel?: string
-  routerOptions?: RouterOptions
-  target?: HTMLAttributeAnchorTarget
-  textValue?: string
-  value?: T
-}

/@react-spectrum/s2:ListViewProps

-ListViewProps <T> {
-  UNSAFE_className?: UnsafeClassName
-  UNSAFE_style?: CSSProperties
-  aria-describedby?: string
-  aria-details?: string
-  aria-label?: string
-  aria-labelledby?: string
-  autoFocus?: boolean | FocusStrategy
-  children: ReactNode | (T) => ReactNode
-  defaultSelectedKeys?: 'all' | Iterable<Key>
-  dependencies?: ReadonlyArray<any>
-  disabledBehavior?: DisabledBehavior = "all"
-  disabledKeys?: Iterable<Key>
-  disallowEmptySelection?: boolean
-  disallowTypeAhead?: boolean = false
-  escapeKeyBehavior?: 'clearSelection' | 'none' = 'clearSelection'
-  hideLinkOutIcon?: boolean
-  id?: string
-  isQuiet?: boolean
-  items?: Iterable<T>
-  loadingState?: LoadingState
-  onAction?: (Key) => void
-  onLoadMore?: () => void
-  onSelectionChange?: (Selection) => void
-  overflowMode?: 'wrap' | 'truncate' = 'truncate'
-  renderActionBar?: ('all' | Set<Key>) => ReactElement
-  renderEmptyState?: (GridListRenderProps) => ReactNode
-  selectedKeys?: 'all' | Iterable<Key>
-  selectionMode?: SelectionMode
-  selectionStyle?: 'highlight' | 'checkbox' = 'checkbox'
-  shouldSelectOnPressUp?: boolean
-  slot?: string | null
-  styles?: StylesPropWithHeight
-}

/@react-spectrum/s2:ListViewItemProps

-ListViewItemProps {
-  children: ReactNode
-  download?: boolean | string
-  hasChildItems?: boolean
-  href?: Href
-  hrefLang?: string
-  id?: Key
-  isDisabled?: boolean
-  onAction?: () => void
-  onHoverChange?: (boolean) => void
-  onHoverEnd?: (HoverEvent) => void
-  onHoverStart?: (HoverEvent) => void
-  onPress?: (PressEvent) => void
-  onPressChange?: (boolean) => void
-  onPressEnd?: (PressEvent) => void
-  onPressStart?: (PressEvent) => void
-  onPressUp?: (PressEvent) => void
-  ping?: string
-  referrerPolicy?: HTMLAttributeReferrerPolicy
-  rel?: string
-  routerOptions?: RouterOptions
-  target?: HTMLAttributeAnchorTarget
-  textValue?: string
-  value?: T
-}

---

[LFDanLu]

note this is using useId now because the Popover in a Menu actually renders null when not open since it doesn't benefit from Hidden always rendering the children. We were previously actually relying on RACDialog setting the id for the heading