fix/bug useTablist #5996

[mdhmaiti]

Closes #5996

✅ Pull Request Checklist:

• Issue "useTabList" hook keyboard navigation with arrow keys not considering orientation #5996
• Added/updated unit tests and storybook for this change (for new code or code which already has tests).
• Filled out test instructions.
• Updated documentation (if it already exists for this component).
• Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

1. run locally and navigate to the useTabList in the storybook.
2. try to move between the tabs using arrow keys.
3. Here I added restrictions to the upper arrow key and lower arrow key to solve the issue .

🧢 Your Project:

Added the horizontal orientations in the TabsKeyboardDeligate to prevent moving of tabs using upper and lower arrow keys

[snowystinger]

Thanks for the PR! To answer some of your questions from the Issue, it should be your Github login for the CLA.

It would also appear that you have prettier or some auto formatted enabled because some files changed which I don't think you meant to change. It would be good to revert all the unrelated style changes so we can focus on the actual changes for the PR.

You can see how each of the ci steps are run by looking in the circle config file, package.json, and makefile. As for getting them to pass, if you look at the test that failed, you'll see that we're testing for the bad behavior

react-spectrum/packages/@react-spectrum/tabs/test/Tabs.test.js

Line 122 in a8ec966

/** Changes selection regardless if it's horizontal tabs. */

so you should be able to just assert that up/down doesn't move focus for horizontal tabs

[mdhmaiti]

Thanks for helping me. I will fix my code

[LFDanLu]

GET_BUILD

[LFDanLu]

Thanks for the PR! I'm double checking the behavior we want here via #5996 (comment) so we'll be holding off on any further changes until we can confirm that.

As an aside, if this is indeed the behavior we want, we'll also need to modify useSelectableCollection to only conditionally prevent default behavior on various keydown events. As it is now, the changes in this PR actually aren't sufficient to allow ArrowUp/Down to scroll the page when the TabList's orientation is 'horizontal'

[rspbot]

Build successful! 🎉

• View the storybook
• View the storybook-17
• View the storybook-16
• View the documentation

[rspbot]

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

[mdhmaiti]

getKeyBelow(key: Key) { if (this.tabDirection) { window.scrollBy(0, window.innerHeight); return null; } return this.getNextKey(key); }
I thought of adding window.scrollBy(0, window.innerHeight); to provide vertical scroll but the useTablist story book only has the horizontal scroll so I was not sure window.scrollby was working or not, so I skipped this code

[snowystinger]

getKeyBelow(key: Key) { if (this.tabDirection) { window.scrollBy(0, window.innerHeight); return null; } return this.getNextKey(key); } I thought of adding window.scrollBy(0, window.innerHeight); to provide vertical scroll but the useTablist story book only has the horizontal scroll so I was not sure window.scrollby was working or not, so I skipped this code

Better to just not preventDefault when we aren't actively handling that key and let the browser do its thing. It may not be the window which needs to scroll. It could be any ancestor.

react-spectrum/packages/@react-aria/selection/src/useSelectableCollection.ts

Line 126 in 89fa370

e.preventDefault();

[LFDanLu]

since this is in the if (e.altKey && e.key === 'Tab') { statement, this won't actually have the desired effect of allowing default behavior if the the delegate returns null from getKeyAbove/getKeyBelow. You'll want to look at the cases in the switch statement like here:

react-spectrum/packages/@react-aria/selection/src/useSelectableCollection.ts

Line 166 in 449bc07

e.preventDefault();

and make those e.preventDefault not be called if nextKey is null I imagine.

[yihuiliao]

I just wanted to pop in and let you know that the team might be slow to review as we work on some other priorities. We appreciate your understanding and apologize for the wait.

[mdhmaiti]

No no its okk , I also had my exams and I am still learning, I am willing to wait

[snowystinger]

Update on comment #6023 (review) from above here: #5996 (comment)

[LFDanLu]

Verified that ArrowUp/Down still have browser default behavior when navigating a horizontally oriented TabList but ArrowUp/Down/Left/Right all still shift the Tab selection for vertical tabs. Thanks for the contribution!