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.

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

fix: pressing arrow keys behavior on Radio buttons #1138

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: pressing arrow keys behavior on Radio buttons #1138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
48892d0
add more radio buttons for test
tnyo43 May 25, 2023
563f8f8
fix the test casess as they should be
tnyo43 May 25, 2023
24539b4
fix the logic of walkRadio function
tnyo43 May 25, 2023
5e9fd2b
fix direction
tnyo43 May 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/event/behavior/keydown.ts
View file
Open in desktop
Expand Up @@ -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)
Copy link
Author

@tnyo43 tnyo43 May 25, 2023 •
edited

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.

}
},
ArrowLeft: (event, target, instance) => {
Expand All @@ -46,7 +46,7 @@ const keydownBehavior: {
ArrowUp: (event, target, instance) => {
/* istanbul ignore else */
if (isElementType(target, 'input', {type: 'radio'} as const)) {
return () => walkRadio(instance, target, 1)
return () => walkRadio(instance, target, -1)
}
},
Backspace: (event, target, instance) => {
Expand Down
28 changes: 18 additions & 10 deletions src/event/radio.ts
View file
Open in desktop
Copy link
Author

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.

  1. find the starting element
  2. get the next element according on the direction. If it's over the last / first element, go to the other end.
  3. 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.

Expand Up @@ -15,18 +15,26 @@ export function walkRadio(
: `input[type="radio"][name=""], input[type="radio"]:not([name])`,
),
)
for (let i = group.findIndex(e => e === el) + direction; ; i += direction) {
if (!group[i]) {
i = direction > 0 ? 0 : group.length - 1
}
if (group[i] === el) {
return

let indexOfRadiogroup = group.findIndex(e => e === el)
do {
// move to the next element
indexOfRadiogroup += direction
if (!group[indexOfRadiogroup]) {
indexOfRadiogroup = direction > 0 ? 0 : group.length - 1
}
if (isDisabled(group[i])) {
const element = group[indexOfRadiogroup]

if (isDisabled(element)) {
continue
}

focusElement(group[i])
instance.dispatchUIEvent(group[i], 'click')
}
if (element === el) {
// If there is only one available radiobutton element, do nothing
} else {
focusElement(element)
instance.dispatchUIEvent(element, 'click')
}
break
} while (true)
}
6 changes: 4 additions & 2 deletions tests/event/behavior/keydown.ts
View file
Open in desktop
Expand Up @@ -317,6 +317,8 @@ cases(
<input type="radio" name="" value="nameless2"/>
<input type="radio" name="group" value="c" disabled/>
<input type="radio" name="group" value="d"/>
<input type="radio" name="group" value="e"/>
<input type="radio" name="group" value="f"/>
Comment on lines +320 to +321
Copy link
Author

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.

<input type="radio" name="foo"/>
<input type="text" name="group"/>
`,
Expand Down Expand Up @@ -360,14 +362,14 @@ cases(
expectedTarget: '//input[@value="a"]',
},
'forward around the corner': {
focus: '//input[@value="d"]',
focus: '//input[@value="f"]',
key: 'ArrowRight',
expectedTarget: '//input[@value="a"]',
},
'backward around the corner': {
focus: '//input[@value="a"]',
key: 'ArrowUp',
expectedTarget: '//input[@value="d"]',
expectedTarget: '//input[@value="f"]',
},
'do nothing on single radio': {
focus: '//input[@name="solo"]',
Expand Down