Skip to content

fix: add isFocusVisible useMenuItem and fix focusRing when typing in Autocomplete SearchField #7625

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

Merged
merged 3 commits into from
Jan 22, 2025

Conversation

LFDanLu
Copy link
Member

@LFDanLu LFDanLu commented Jan 16, 2025

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:

  1. Navigate to https://reactspectrum.blob.core.windows.net/reactspectrum/5500dd71c424604132f88afcb3df3eb8335d991e/storybook-s2/index.html?path=/story/popover--autocomplete-popover
  2. Open the Popover
  3. Keyboard navigate the Autocomplete. Verify that a focus ring is visible around the menu items when keyboard navigating, but the ring disappears when hovering via mouse. The menu item focus ring should reappear when typing or navigating via keyboard.
  4. Click into the SearchField and verify the focus ring on the SearchField disappears if previously present. Verify that typing in said field doesn't cause the SearchField's focus ring to come back.

Ignore the keyboard event leaks causing the storybook controls/bars from appearing/moving. This is known and will be handled when we make collections stop leaking events in general

🧢 Your Project:

RSP

@rspbot
Copy link

rspbot commented Jan 16, 2025

@LFDanLu LFDanLu marked this pull request as ready for review January 17, 2025 00:01
reidbarber
reidbarber previously approved these changes Jan 17, 2025
Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

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

Behavior looks good to me.

* for discussion

* fix searchfield focus ring appearing on keypress when wrapped in a autocomplete

* add test

* remove extranous controls
@LFDanLu LFDanLu changed the title fix: add isFocusVisible support to useMenuItem fix: add isFocusVisible useMenuItem and fix focusRing when typing in Autocomplete SearchField Jan 17, 2025
Comment on lines +293 to 295
// For keyboard events that occur on a non-input element that will move focus into input element (aka ArrowLeft going from Datepicker button to the main input group)
// we need to rely on the user passing isTextInput into here. This way we can skip toggling focus visiblity for said input element
isTextInput = isTextInput ||
Copy link
Member Author

Choose a reason for hiding this comment

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

So it turns out that we still need isTextInput since it allows elements to NOT get focusVisible styles when focus moves from a non-input element via some other method than Tab. This happens for Datepicker on main via the following steps:

  1. click into the datepicker date input
  2. using ArrowRight, move focus on to the Datepicker button. Note the focus ring appears for the button
  3. using ArrowLeft, move focus on to the Datepicker input again. Note the focus ring doesn't appear on the Datepicker group

A bit debatable if this is desired behavior perhaps, I would've liked to simply deprecate isTextInput but the removal of this behavior would be a breaking change for anyone who was relying on it previously

@rspbot
Copy link

rspbot commented Jan 17, 2025

@rspbot
Copy link

rspbot commented Jan 17, 2025

## API Changes

@react-aria/focus

/@react-aria/focus:isFocusable

 isFocusable {
-  element: Element
+  element: HTMLElement
   returnVal: undefined
 }

@react-aria/menu

/@react-aria/menu:MenuItemAria

 MenuItemAria {
   descriptionProps: DOMAttributes
   isDisabled: boolean
+  isFocusVisible: boolean
   isFocused: boolean
   isPressed: boolean
   isSelected: boolean
   keyboardShortcutProps: DOMAttributes
   menuItemProps: DOMAttributes
 }

@react-aria/test-utils

/@react-aria/test-utils:triggerLongPress

 triggerLongPress {
   opts: {
     element: HTMLElement
   advanceTimer: (number) => void | Promise<unknown>
-  pointerOpts?: Record<string, any>
+  pointerOpts?: {
+  
 }
+}
   returnVal: undefined
 }

@react-aria/utils

/@react-aria/utils:isFocusable

-isFocusable {
-  element: Element
-  returnVal: undefined
-}

/@react-aria/utils:isTabbable

-isTabbable {
-  element: Element
-  returnVal: undefined
-}

@react-spectrum/test-utils

/@react-spectrum/test-utils:triggerLongPress

 triggerLongPress {
   opts: {
     element: HTMLElement
   advanceTimer: (number) => void | Promise<unknown>
-  pointerOpts?: Record<string, any>
+  pointerOpts?: {
+  
 }
+}
   returnVal: undefined
 }

@snowystinger
Copy link
Member

snowystinger commented Jan 22, 2025

Looks like it still comes back?

test.focus.ring.mov

ignore the jump after typing "a", that's just storybook shortcuts, not sure why they are happening, but "a" will show and hide the addons panel

Copy link
Member

@yihuiliao yihuiliao left a comment

Choose a reason for hiding this comment

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

It might be good to add some chromatic tests for this using the play functions in S2

@LFDanLu
Copy link
Member Author

LFDanLu commented Jan 22, 2025

@yihuiliao good idea, I'll see about doing that as a followup

@LFDanLu LFDanLu added this pull request to the merge queue Jan 22, 2025
Merged via the queue into main with commit 9bb6ed9 Jan 22, 2025
33 checks passed
@LFDanLu LFDanLu deleted the autocomplete_focusvisible branch January 22, 2025 19:20
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.

5 participants