-
Notifications
You must be signed in to change notification settings - Fork 810
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
base: master
Are you sure you want to change the base?
Conversation
LEGIT. Is there any accessibility tool or documentation where we chan check this behavior? |
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 |
@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? 🙇 |
Yep, please. |
Co-authored-by: Bruno Dias <[email protected]>
Co-authored-by: Bruno Dias <[email protected]>
Is this what you mean? |
@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. |
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. |
@diasbruno anything else needed to get this merged? |
@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? |
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. |
Ok. I'll have a look on it later. |
Co-authored-by: Bruno Dias <[email protected]>
@diasbruno Forgot about this, but any chance you could take a look at it again? |
@loganscharen @diasbruno This issue still exists. Any reasons why y'all have held off on merging this PR? |
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:
Fixes #636 .