react-modal icon indicating copy to clipboard operation
react-modal copied to clipboard

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

Open loganscharen opened this issue 4 months ago • 13 comments

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:

  • [X] Tests
  • [ ] Documentation and examples (if needed)

Fixes #636 .

loganscharen avatar Mar 08 '24 16:03 loganscharen

LEGIT.

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

diasbruno avatar Mar 08 '24 17:03 diasbruno

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 avatar Mar 08 '24 17:03 loganscharen

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

diasbruno avatar Mar 11 '24 14:03 diasbruno

@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? 🙇

doeg avatar Mar 11 '24 20:03 doeg

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

Yep, please.

diasbruno avatar Mar 12 '24 13:03 diasbruno

Is this what you mean? https://www.w3.org/WAI/ARIA/apg/patterns/dialog-modal/#keyboardinteraction

loganscharen avatar Mar 13 '24 14:03 loganscharen

@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.

diasbruno avatar Mar 13 '24 17:03 diasbruno

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 avatar Mar 13 '24 17:03 loganscharen

@diasbruno anything else needed to get this merged?

loganscharen avatar Mar 14 '24 15:03 loganscharen

@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?

diasbruno avatar Mar 14 '24 16:03 diasbruno

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.

loganscharen avatar Mar 14 '24 16:03 loganscharen

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

diasbruno avatar Mar 14 '24 16:03 diasbruno

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

loganscharen avatar Apr 19 '24 14:04 loganscharen