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

Fixing issue with tabbing when a radio is the last element of the modal #1045

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

Conversation

loganscharen
Copy link

The issue here was that the code was checking if the activeElement was the last element, but for radio groups, you can tab out without it being the last element. This checks if the tail and activeElement are part of the same radio group and if so treats it as if it were the last element

Acceptance Checklist:

  • Tests
  • Documentation and examples (if needed)

Fixes #636 .

src/helpers/scopeTab.js Outdated Show resolved Hide resolved
@diasbruno
Copy link
Collaborator

LEGIT.

Is there any accessibility tool or documentation where we chan check this behavior?

@loganscharen
Copy link
Author

Moved that to a function and updated the basic form example to have the radios at the end to be able to see the functionality

src/helpers/scopeTab.js Outdated Show resolved Hide resolved
src/helpers/scopeTab.js Outdated Show resolved Hide resolved
@diasbruno
Copy link
Collaborator

@loganscharen @doeg Is there any accessibility tool or documentation where we chan check this behavior?

@doeg
Copy link

doeg commented Mar 11, 2024

@loganscharen @doeg Is there any accessibility tool or documentation where we chan check this behavior?

@diasbruno ah, not sure I follow... Do you mean checking whether the new behaviour is compliant with W3 standards? Or do you mean automated checks in addition to the tests to verify it works in all cases? 🙇

@diasbruno
Copy link
Collaborator

Do you mean checking whether the new behaviour is compliant with W3 standards?

Yep, please.

@loganscharen
Copy link
Author

@diasbruno
Copy link
Collaborator

@loganscharen Yas, but for the case of radio groups. If we are going to intervening on default browser behavior, we need to do it right.

@loganscharen
Copy link
Author

https://www.w3.org/WAI/ARIA/apg/patterns/radio/#keyboardinteraction

So this? That tab is used to go into and out of the radio group and not between the elements of the group. So the radio group is the last tabbable element, not each radio button separately.

@loganscharen
Copy link
Author

@diasbruno anything else needed to get this merged?

@diasbruno
Copy link
Collaborator

@loganscharen Thanks. That's what I was looking for.

There are some rules on that specification. Should we add tests to check if the behavior is correct (arrows, space)?

Also, long time ago there was this issue https://github.com/reactjs/react-modal/blob/master/src/helpers/scopeTab.js#L48, but I think I've implemented not respecting the radio specification. Should this cause this bug?

@loganscharen
Copy link
Author

I don't see why we would need to test that space and arrow are working correctly, we're not touching that functionality at all.

The safari bug doesn't have any impact on this, this is just occurring because we're currently looking at the last element, but from a radio group, the last element isn't always the last tabbable element.

@diasbruno
Copy link
Collaborator

Ok. I'll have a look on it later.

src/helpers/scopeTab.js Outdated Show resolved Hide resolved
@loganscharen
Copy link
Author

@diasbruno Forgot about this, but any chance you could take a look at it again?

@Divya1k5
Copy link

@loganscharen @diasbruno This issue still exists. Any reasons why y'all have held off on merging this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Focus escapes modal when radio buttons in modal
4 participants