Skip to content

feat: migrate more to useKeyboard and bubble phase#10187

Open
snowystinger wants to merge 7 commits into
mainfrom
new-event-leaks
Open

feat: migrate more to useKeyboard and bubble phase#10187
snowystinger wants to merge 7 commits into
mainfrom
new-event-leaks

Conversation

@snowystinger

Copy link
Copy Markdown
Member

Closes

This one is a little more controversial. I solved the useToolbar in an interesting way, I would like opinions.
My main concern right now is potentially how many places would need to prevent that default, and it's not obvious because of the way React components compose. So you'd kinda need to anticipate that someone might pass a toolbar into your component where you want to handle the arrow right/left.
This only works with our own components. Other users would have to implement this themselves for their custom components.

I also contemplated keeping a capturing keydown and attaching a special value to the event like "preferred key handler" so that the toolbar could look for that and if it was there, only handle the non-wrapping case, and when it would wrap, yield instead to the preferred key handler.
This wouldn't require a useEffect and custom event, but it's weird to just attach data to an event. Not very obvious for debugging.

In the past I also solved it by looking to see if a role=toolbar was above my current one. This worked for nested toolbars, but only if it was one of ours. We'd have to expand it to gridlist and possibly others.

I thought maybe we could defer running the event handler to see if something else handled it (or preventedDefault), but why would toolbar do that but gridlist wouldn't...

We could turn off wrapping by default and make people do it explicitly, in which case it would just take precedence and be broken in a GridList, but again, it'd work by default, so I think this might be ok.
I think this is my favourite option. But I've left it as the first one so people can see what it looks like.

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

🧢 Your Project:

'aria-label': ariaLabel,
'aria-labelledby': ariaLabel == null ? ariaLabelledBy : undefined,
onKeyDownCapture: !isInToolbar ? onKeyDown : undefined,
onKeyDown: !isInToolbar ? keyboardProps.onKeyDown : undefined,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm unable to let children toolbars manage themselves at the moment, this is because in a nested case, tab/shift+tab needs to go to the last focus element across everything in the nested toolbars

Arrow keys would all work, it's just the overall management. I could remove tab/shift tab from useKeyboard and just gate that part on this logic

Comment thread packages/react-aria-components/test/Table.test.js Outdated
@rspbot

rspbot commented Jun 11, 2026

Copy link
Copy Markdown

@rspbot

rspbot commented Jun 11, 2026

Copy link
Copy Markdown

@rspbot

rspbot commented Jun 11, 2026

Copy link
Copy Markdown
## API Changes

@react-aria/focus

/@react-aria/focus:FocusManagerOptions

 FocusManagerOptions {
   accept?: (Element) => boolean
+  action?: string
   from?: Element
   tabbable?: boolean
   wrap?: boolean
 }

@react-spectrum/ai

/@react-spectrum/ai:HorizontalCard

 HorizontalCard {
   children: ReactNode | (CardRenderProps) => ReactNode
   density?: 'compact' | 'regular' | 'spacious' = 'regular'
   download?: boolean | string
   href?: Href
   hrefLang?: string
   id?: Key
   isDisabled?: boolean
   onAction?: () => void
   onPress?: (PressEvent) => void
   onPressChange?: (boolean) => void
   onPressEnd?: (PressEvent) => void
   onPressStart?: (PressEvent) => void
   onPressUp?: (PressEvent) => void
   ping?: string
   referrerPolicy?: HTMLAttributeReferrerPolicy
   rel?: string
   routerOptions?: RouterOptions
   size?: 'XS' | 'S' | 'M' | 'L' | 'XL' = 'M'
   styles?: StyleString
   target?: HTMLAttributeAnchorTarget
   textValue?: string
   value?: T
-  variant?: 'primary' | 'secondary' | 'tertiary' = 'primary'
+  variant?: 'primary' | 'secondary' | 'tertiary' | 'quiet' = 'primary'
 }

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