Skip to content
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

Attach keydown listeners to searchbar #9845

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

schu96
Copy link
Contributor

@schu96 schu96 commented Sep 5, 2024

Closes #7916
I have implemented the suggested behaviors from #7916 but would like to know if there are additional navigation features/more preferred navigation flows for this component.

I wasn't entirely sure what the intended behavior should be when a user presses Tab while focused on the Form input field and left it as the default behavior.

Fix

Technical

Implements the suggested keyboard functionality by attaching jQuery .on event handlers to listen for ArrowUp, ArrowDown, Escape, Tab, and Shift + Tab keycodes. tabindex has been attached to each li element to allow users to navigate the search bar results with their keyboard.

Testing

Go to localhost:8080 and use a search term that returns at least 3 titles.
The expected behaviors are:
Form input <ArrowUp/ArrowDown> - Goes to the first or last item in the results list
Form input <Escape> - Removes focus from the search bar input, clears search results

Result li element <ArrowUp/Shift + Tab> - Navigates to the previous list sibling if exists, otherwise will focus on the Form input
Result li element <ArrowDown/Tab> - Navigates to the next list sibling if exists, otherwise will loop focus back to the Form input
Result li element - Redirects the user to the title
Result li element - Blurs focus from input[type="text"] and clears the results list.

Search facet <Shift + Tab> - Clears the results list since focus is no longer within the relevant search bar component

Screenshot

This video demos all but the Result li element keyboard behaviors.
https://github.com/user-attachments/assets/aa4c8261-b888-43b6-806a-5b4e28ff7424

Stakeholders

@RayBB

@codecov-commenter
Copy link

codecov-commenter commented Sep 5, 2024

Codecov Report

Attention: Patch coverage is 17.07317% with 34 lines in your changes missing coverage. Please review.

Project coverage is 16.43%. Comparing base (ce16a79) to head (cc68ea4).
Report is 409 commits behind head on master.

Files with missing lines Patch % Lines
openlibrary/plugins/openlibrary/js/SearchBar.js 17.07% 22 Missing and 12 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9845      +/-   ##
==========================================
+ Coverage   16.06%   16.43%   +0.37%     
==========================================
  Files          90       92       +2     
  Lines        4769     4947     +178     
  Branches      832      870      +38     
==========================================
+ Hits          766      813      +47     
- Misses       3480     3587     +107     
- Partials      523      547      +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@RayBB
Copy link
Collaborator

RayBB commented Sep 5, 2024

@bick-jp do you have any opinions on this solution?

I'm really not so familiar with accessibility changes so I'll have to get some second opinions for sure.

@scottbarnes do you know what the current thoughts are on using jQuery for changes like this? I know in some cases we prefer vanilla JavaScript but I don't know if we are totally avoiding jQuery for new code like this.

@RayBB
Copy link
Collaborator

RayBB commented Sep 10, 2024

@schu96 small update from the weekly meeting.

@cdrini said the jquery probably should be :)
Also Drini will checkout this PR as well for accessibility.

Just to double check @schu96 is this PR ready for review? If so, please mark it as such, but it looks like it is so I'll assume so.

@schu96 schu96 marked this pull request as ready for review September 10, 2024 18:13
@schu96
Copy link
Contributor Author

schu96 commented Sep 10, 2024

@RayBB thanks for the update! I initially had it as a draft in case the navigation behaviors needed some tweaking. Does this mean that all existing jQuery code is ok to update like this?

@RayBB RayBB added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Sep 11, 2024
@cdrini
Copy link
Collaborator

cdrini commented Sep 11, 2024

Nice work @schu96 ! Gave this a test, using wikipedia's search box autocomplete as our "gold standard" expected results, and here's what I noticed:

  • ❌ tabbing past the search box should close the autocomplete suggestions and move to the next item. Tabbing should not move through the autocomplete results (see wikipedia searchbox)
  • ❌ escape should close the autocomplete results and return focus to the input field
  • ✅ up down work perfectly

@cdrini cdrini added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Sep 11, 2024
@github-actions github-actions bot removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The accessibility of keyboard navigation of the search bar can be improved
4 participants