Skip to content

refactor(global-search): keep browser focus but allow for keyboard nav#450

Merged
elmarburke merged 3 commits intomainfrom
global-search/keep-focus
Sep 24, 2025
Merged

refactor(global-search): keep browser focus but allow for keyboard nav#450
elmarburke merged 3 commits intomainfrom
global-search/keep-focus

Conversation

@elmarburke
Copy link
Copy Markdown
Contributor

@elmarburke elmarburke commented Sep 23, 2025

Description

Changing how items are focused during keyboard navigation. Previously, the native focus was on the results. That blocked users from typing ahead. Now, it uses a "virtual focus" so typing remains possible.

  • Removed useFocusHandlers and added integrated useSelection for "virtual" focus handling in GroupHeader, Match, and SearchInput components.
  • Updated ResultsList to manage virtual item IDs with useSelection.
  • Removed useFocus and react-aria

Testing

  • Type, navigate, type
    • search for something
    • use the arrow key to navigate to an item
    • type more
    • it works
  • Headers are not selected by up/down navigation
  • Clicking does set virtual focus
    • search for something, click the result with the mouse
    • the active state is looking correct
  • Same as above, but with the tab key

@elmarburke elmarburke force-pushed the global-search/keep-focus branch from ecb6ace to e45732a Compare September 23, 2025 10:35
@elmarburke elmarburke marked this pull request as ready for review September 23, 2025 10:37
Copy link
Copy Markdown
Collaborator

@kaloyanvi kaloyanvi left a comment

Choose a reason for hiding this comment

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

Nice one! 🔥

Comment thread plugins/global-search/src/components/Results.tsx Outdated
Comment thread plugins/global-search/src/utils/selection/context.ts Outdated
Comment thread plugins/global-search/src/utils/selection/context.ts Outdated
Comment thread plugins/global-search/src/utils/selection/SelectionProvider.tsx
Comment thread plugins/global-search/src/utils/selection/SelectionProvider.tsx Outdated
@kaloyanvi
Copy link
Copy Markdown
Collaborator

The specified QA steps are all ok but we shouldn't set the focus on the headers when clicking. See in the video

CleanShot.2025-09-23.at.14.37.24.mp4

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Comment @cursor review or bugbot run to trigger another review on this PR

aria-expanded={isExpanded}
{...focusProps}
{...props}
{...getFocusProps(headerId(entry.id))}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: Group Headers Incorrectly Activate on Click

Group headers become active when clicked, which isn't the intended behavior and creates an inconsistency with keyboard navigation that correctly skips them. The getFocusProps function in SelectionProvider sets any focused item as active, including headers.

Additional Locations (1)

Fix in Cursor Fix in Web

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's very much intended here!

@elmarburke
Copy link
Copy Markdown
Contributor Author

@kaloyanvi the header focus is now removed on navigation

@kaloyanvi
Copy link
Copy Markdown
Collaborator

@kaloyanvi the header focus is now removed on navigation

Yes thats fixed now! Theres still another issue with the focus when tabbing and shift-tabbing.

  1. I'd expect that the arrows focus and the tab focus are in sync. E.g going to the 3rd item with the arrows and pressing tab once should bring you back to the 4th item. Pressing shift-tab twice should bring you to the 2nd item.
  2. Headers are still focusable when tabbing
CleanShot 2025-09-23 at 15 46 06

@elmarburke
Copy link
Copy Markdown
Contributor Author

Headers are focusable on tab by design. Every item should be reachable with the tab key. That's why they are "partially" out of sync: With tab, you move the (real) browser focus. IMHO, we shouldn't block elements from being reached here.

@kaloyanvi
Copy link
Copy Markdown
Collaborator

IMHO, we shouldn't block elements from being reached here.

yeah thats fair! But now feels strange that arrow keys after tabbing continues correctly but tabbing after arrow keys brings you back to the start.

…vigation

- Removed useFocusHandlers and integrated useSelection for focus handling in GroupHeader, Match, and SearchInput components.
- Updated ResultsList to manage virtual item IDs with useSelection.
- Removed unused useFocus utility file.
- Adjusted SearchScene to use SelectionProvider for context management.
@elmarburke elmarburke force-pushed the global-search/keep-focus branch from 2c0ce55 to 7a4c8b6 Compare September 24, 2025 08:11
@kaloyanvi
Copy link
Copy Markdown
Collaborator

QA ✅

@elmarburke
Copy link
Copy Markdown
Contributor Author

As discussed offline: Split the tab and the arrow key navigation into two different things. Added an outline with focus-visbile to visually seperate the virtual and the real focus state.

@elmarburke elmarburke added this pull request to the merge queue Sep 24, 2025
Merged via the queue into main with commit dd769b1 Sep 24, 2025
7 of 9 checks passed
@elmarburke elmarburke deleted the global-search/keep-focus branch September 24, 2025 08:25
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