-
Notifications
You must be signed in to change notification settings - Fork 252
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
fix: pressing arrow keys behavior on Radio buttons #1138
base: main
Are you sure you want to change the base?
fix: pressing arrow keys behavior on Radio buttons #1138
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 5e9fd2b:
|
<input type="radio" name="group" value="e"/> | ||
<input type="radio" name="group" value="f"/> |
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.
There were only two available radio buttons actually (the values are "a"
and "d"
).
I've added some other elements to make the behavior easier to understand.
@@ -28,7 +28,7 @@ const keydownBehavior: { | |||
ArrowDown: (event, target, instance) => { | |||
/* istanbul ignore else */ | |||
if (isElementType(target, 'input', {type: 'radio'} as const)) { | |||
return () => walkRadio(instance, target, -1) | |||
return () => walkRadio(instance, target, 1) |
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.
As the W3C specification about radio group describes, pressing the Down Arrow key behaves the same way as the Right Arrow key. "Moves focus to and checks the next radio button in the group" when the Down/Right arrow is pressed, so the direction
of walkRadio
should be 1.
The same is true for the Up and Left Arrow keys, so the direction
should be -1.
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.
I think the previous implementation is a bit too hard to understand. I have rewritten it to make it easier to understand what each process does.
- find the starting element
- get the next element according on the direction. If it's over the last / first element, go to the other end.
- if the element is...
3.1. disabled, skip it and go to the next one.
3.2. avaiable, and if it is equal to the starting element, do nothing. Otherwise, check it.
@ph-fritsche @Gpx @kentcdodds can we merge this PR? If I can help something to merge, I will gladly cooperate with you. |
What:
Fix a bug in the behavior of the keyboard operations on the radio buttons.
Why:
closes #1048
How:
Checklist:
FYI
I know there is already another PR #1049 for this bug. However, it has past several months since it stop its activity.