-
Notifications
You must be signed in to change notification settings - Fork 56
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
a11y: allow users to tab across the search button (and release: v2.8.6) #195
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review
✔️ Using onMouseDown
is definitely weird, but this is a symptom of the auto-expanding text field, and it is properly commented in the code.
Testing
Tested locally with InvenioRDM version 12.0.0b3.dev9
(and with the latest changes in this repo until v2.8.5
included).
Keyboard and search button:
- Going to: https://127.0.0.1:5000/search
- 3x tab to reach text field
- Typing some search text
- 1x tab to focus on submit button
- ✔️ The form is not submitted
- 1x tab to reach "Communities"
- 1x shift-tab to focus on the submit button
- Typing Enter
- ✔️ The form is submitted
Keyboard and text field:
- Going to: https://127.0.0.1:5000/search
- 3x tab to reach text field
- Typing some search text
- Typing Enter
- ✔️ The form is submitted
Mouse and search button:
- Going to: https://127.0.0.1:5000/search
- Clicking on the text field
- Typing some search text
- Clicking on the search button
- ✔️ The form is submitted
Previously as soon as you select the search button via tab it would execute the search. Now we only look for onMouseDown. We cannot use onClick because the button moves as soon as it is focused via mouse down. * remove handleOnMouseDown function * move event listener to button
@@ -158,7 +149,6 @@ export class MultipleOptionsSearchBarCmp extends Component { | |||
onResultSelect={this.onBtnSearchClick} | |||
onSearchChange={this.handleOnSearchChange} | |||
resultRenderer={(props) => resultRenderer(props, queryString)} | |||
onFocus={this.handleFocus} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we remove onFocus, can we still navigate to the searchbar by using tab? (I think it could matter for accessibility as well - screen readers, but correct me if I am wrong)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, one can reach the search bar with tab (see my keyboard tests above).
What this handler was doing was to submit the form when the button received focus, which is clearly not a behavior we want (it's probably due to the fact that the previous change was not tested with the keyboard and only by clicking with a mouse).
Pushed the tag and released on PyPI: https://pypi.org/project/invenio-search-ui/2.8.6/ |
❤️ Thank you for your contribution!
Close #198
Description
Fix https://github.com/zenodo/rdm-project/issues/634
Previously as soon as you select the search button via tab it would execute the search. Now we only look for onMouseDown.
We cannot use onClick because the button moves as soon as it is focused via mouse down.
The behaviour is now:
click
enter
orspacebar
enter
Video:
Screen.Recording.2024-01-12.at.5.05.43.pm.mov
Screen.Recording.2024-01-12.at.5.24.17.pm.mov
Checklist
Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:
Third-party code
If you've added third-party code (copy/pasted or new dependencies), please reach out to an architect.
Reminder
By using GitHub, you have already agreed to the GitHub’s Terms of Service including that: