Skip to content

Make searchbar more keyboard-friendly #1752

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Oct 3, 2025
Merged

Make searchbar more keyboard-friendly #1752

merged 10 commits into from
Oct 3, 2025

Conversation

manuhabitela
Copy link
Collaborator

@manuhabitela manuhabitela commented Aug 6, 2025 •
edited
Loading

Context

The document search bar in the top panel has a few issues:

  • we can't navigate after the search button when using the tab key using Ctrl+O. The navigation is stopped when arriving on the search input and it blocks users from accessing focusable elements appearing after the search bar, in the top panel (for example the share button)
  • we can't focus the search input when using the tab key after using Ctrl+O shortcut to navigate in the top panel. Arriving on the input with Tab instantly closes the input. Works great with Ctrl+F though so it's not such a blocking issue here, but still ;)
  • we can't use the Tab key to focus the search bar items (the "all pages" checkbox, the arrow items, the close button)

Proposed solution

The idea behind this PR is:

  • navigation is not blocked anymore, we can access items from the top panel that are after the search bar when using Ctrl+O to focus the top panel then Tab
  • navigation with Ctrl+O then Tab is now also working as expected to focus the search input
  • tabbing through items in the search bar works. We can toggle the "all pages" checkbox with keyboard only and focus the arrow and close buttons.
  • pressing escape sets back the focus correctly on the current widget if not coming from Ctrl+O nav ; otherwise it just focuses back the search button, allowing the user to continue their navigation in the top panel

Has this been tested?

I did not add any specific test ; the few current tests making searches still pass. Not sure taking time to carefully test behavior is a must add here, I can add some if you think we must :)

  • 👍 yes, I added tests to the test suite
  • 💭 no, because this PR is a draft and still needs work
  • 🙅 no, because this is not relevant here (maybe?)
  • 🙋 no, because I need help

Screenshots / Screencasts

Before:

searchbar.before.mp4

After:

search.mp4

@manuhabitela manuhabitela force-pushed the searchbar-a11y branch 3 times, most recently from aa0072c to 2f62f04 Compare August 12, 2025 14:59
@manuhabitela manuhabitela marked this pull request as ready for review August 12, 2025 14:59
@manuhabitela manuhabitela mentioned this pull request Aug 12, 2025
Merged
@manuhabitela manuhabitela self-assigned this Aug 12, 2025
@georgegevoian georgegevoian self-requested a review August 12, 2025 18:21
@manuhabitela manuhabitela force-pushed the searchbar-a11y branch 2 times, most recently from c5a3079 to ae81a21 Compare August 13, 2025 08:34
@manuhabitela manuhabitela moved this from In Progress to Needs Internal Feedback in French administration Board Aug 13, 2025
@manuhabitela
Copy link
Collaborator Author

This is ready to review on my side :) @fflorent @hexaltation if you wanna see

@manuhabitela manuhabitela added the preview Launch preview deployment of this PR label Aug 13, 2025
@manuhabitela manuhabitela removed their assignment Aug 13, 2025
@github-actions GitHub Actions
Copy link
Contributor

Deployed commit ae81a21b7fcbb2cb2a80f65733e1b2017c283808 as https://grist-manuhabitela-grist-core-searchbar-a11y.fly.dev (until 2025-09-12T09:30:39.004Z)

hexaltation
hexaltation reviewed Aug 14, 2025
View reviewed changes
app/client/ui2018/search.ts
)
),
dom.onKeyDown({
Escape$: () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is there a $ here?
Does it act like a regex or is it a typo for the key code Escape?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a grainjs feature:

By default, registered keyboard events are stopped from bubbling with stopPropagation() and preventDefault(). If, however, you register a key with a "$" suffix (i.e. Enter$ instead of Enter), then the event is allowed to bubble normally.

Copy link
Collaborator Author

@manuhabitela manuhabitela Aug 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

But your question shows I should add a comment here explaining why we don't want to stopPropagation here, you are right it's not obvious 👀

edit: added a comment to clarify

@github-actions GitHub Actions
Copy link
Contributor

Deployed commit 022983ba413ad2f69cfd892e1859634860a036bb as https://grist-manuhabitela-grist-core-searchbar-a11y.fly.dev (until 2025-09-18T12:35:47.903Z)

@manuhabitela manuhabitela moved this from Needs Internal Feedback to Needs feedback in French administration Board Aug 19, 2025
@georgegevoian
Copy link
Contributor

Thanks for working on this @manuhabitela.

The Mod+g/Mod+Shift+G shortcuts no longer work for me while the search input is expanded.

@manuhabitela
Copy link
Collaborator Author

The Mod+g/Mod+Shift+G shortcuts no longer work for me while the search input is expanded.

Woops, good catch thanks. I made a tiny change to make sure they work.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 2, 2025

Deployed commit c22ce574a3daf940aad9dfb6acbf058e5fc71fc3 as https://grist-manuhabitela-grist-core-searchbar-a11y.fly.dev (until 2025-10-02T12:41:33.929Z)

@georgegevoian
Copy link
Contributor

Thanks for the quick fix.

One other thing is that clicking the previous/next buttons or the search all pages checkbox no longer focuses the input. We have some tests not yet in this repo. Let me see about moving them here so you have more context.

@manuhabitela
Copy link
Collaborator Author

One other thing is that clicking the previous/next buttons or the search all pages checkbox no longer focuses the input. We have some tests not yet in this repo. Let me see about moving them here so you have more context.

That one I kinda removed intentionally, felt like sticking with usual browser behavior was a good thing part of the cleanup.

But I added it back with latest commit. This only applies when using the mouse in order to not mess up with keyboard nav where this behavior would be a bit weird :)