reactjs / react-modal

Accessible modal dialog component for React
http://reactcommunity.org/react-modal
MIT License
7.37k stars 808 forks source link

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

Open loganscharen opened 8 months ago

loganscharen commented 8 months ago

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 .

diasbruno commented 8 months ago

LEGIT.

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

loganscharen commented 8 months ago

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

diasbruno commented 8 months ago

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

doeg commented 8 months ago

@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 commented 8 months ago

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

Yep, please.

loganscharen commented 8 months ago

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

diasbruno commented 8 months ago

@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 commented 8 months ago

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 commented 8 months ago

@diasbruno anything else needed to get this merged?

diasbruno commented 8 months ago

@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 commented 8 months ago

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 commented 8 months ago

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

loganscharen commented 7 months ago

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

Divya1k5 commented 4 days ago

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