Fix: Enable tabbing to tag remove buttons #8002
Conversation
|
Thanks for the PR! You'll need to sign the CLA for it the PR to pass |
There was a problem hiding this comment.
Looks pretty good, I added a few more higher level tests to verify RAC and interactions between the TagGroup and other expected components continue to work
One interesting thing I found is that a Tooltip around a Tag with a removable button will still show the tooltip even if focus is on the remove button instead. I am unsure how we want to handle this one. Will require further team discussion. It's not something you have done though, it's just come up as a result of the work. I'm still approving.
| await user.tab({shift: true}); | ||
| expect(document.activeElement).toBe(tags[1]); | ||
|
|
||
| // TODO: Fix, this is going to the previous tag's remove button instead of to the gridlist item |
There was a problem hiding this comment.
Found a test case that's broken, however, I don't think it's due to any of your work, I think this just surfaced it. If you want to have a look go ahead.
…ing out of collection
| <Tag key="1">Tag 1<Button slot="remove">X</Button></Tag> | ||
| <Tag key="2">Tag 2<Button slot="remove">X</Button></Tag> | ||
| <Tag key="3">Tag 3<Button slot="remove">X</Button></Tag> |
There was a problem hiding this comment.
Is it weird that a user tabbing into the Gridlist's TagGroup needs to tab through each of the "X" buttons before being able to tab out of the GridList entirely? I think ideally hitting Tab when focused on the "X" button would move you out of the GridList then
There was a problem hiding this comment.
Ah looks like the same case as what @snowystinger mentioned https://github.com/adobe/react-spectrum/pull/8002/files#r2030387211, due to
react-spectrum/packages/@react-aria/gridlist/src/useGridListItem.ts
Lines 228 to 238 in e2f1c3b
Pushed up a fix
| // For some reason "await user.tab({shift: true});"" doesn't seem to follow the correct behavior when coercing focus to the collection before | ||
| // letting the browser handle the tab event so we simulate this | ||
| fireEvent.keyDown(document.activeElement, {key: 'Tab', shiftKey: true}); | ||
| let walker = getFocusableTreeWalker(document.body, {tabbable: true}); | ||
| walker.currentNode = document.activeElement; | ||
| act(() => {walker.previousNode().focus();}); | ||
| fireEvent.keyUp(document.activeElement, {key: 'Tab', shiftKey: true}); | ||
| expect(document.activeElement).toBe(items[0]); |
There was a problem hiding this comment.
weird that I had to do this, open to any ideas
There was a problem hiding this comment.
We've had to do that in a couple places before, I think it's ok, just a limitation of the libraries we can use. Might be better in a later jsdom
This PR fixes issue #7483 by improving keyboard accessibility for tag remove buttons.
Changes made:
keyboardNavigationBehavior: 'tab'to useTagGroupdelete rowProps.onKeyDownCapturefrom useTagexcludeFromTabOrder: truefrom useTagThese changes allow users to tab from a tag to its remove button, making the remove functionality accessible via keyboard. Arrow keys still work for navigating between tags.
Before this fix, keyboard users couldn't access the remove buttons via tab navigation, making it impossible to remove tags using only a keyboard.
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project:
Thomson Reuters - Contributing to improve React Spectrum accessibility