Skip to content

fix[RAC]: Update tab navigation for arrow keys #4821

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.

Sign up for GitHub

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

Merged
reidbarber merged 2 commits into from
Aug 15, 2023
Merged

fix[RAC]: Update tab navigation for arrow keys #4821

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ad85388
Update tab navigation for arrow keys
mariareedstrom Jul 22, 2023
30ab30c
Merge branch 'main' into 4140-tab-orientation-arrowkeys
LFDanLu Aug 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/@react-aria/tabs/docs/useTabList.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ tab selection.

### Orientation

By default, tabs are horizontally oriented. The `orientation` prop can be set to `vertical` to change this. This affects keyboard navigation. You are responsible for styling your tabs accordingly.
By default, tabs are horizontally oriented. The `orientation` prop can be set to `vertical` to change this. This does not affect keyboard navigation. You are responsible for styling your tabs accordingly.

```tsx example
<Tabs aria-label="Chat log orientation example" orientation="vertical">
Expand Down
28 changes: 6 additions & 22 deletions packages/@react-aria/tabs/src/TabsKeyboardDelegate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,49 +17,33 @@ export class TabsKeyboardDelegate<T> implements KeyboardDelegate {
private collection: Collection<T>;
private flipDirection: boolean;
private disabledKeys: Set<Key>;
private orientation: Orientation;

constructor(collection: Collection<T>, direction: Direction, orientation: Orientation, disabledKeys: Set<Key> = new Set()) {
this.collection = collection;
this.flipDirection = direction === 'rtl' && orientation === 'horizontal';
Copy link
Member

@LFDanLu LFDanLu Jul 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LFDanLu The orientation variable is no longer used in the code, unsure of plans for future use. Keeping it unused causes Typescript issues, which can be suppressed. What do you think?

I think we'll want to keep this.flipDirection = direction === 'rtl' && orientation === 'horizontal'; here to keep ArrowLeft/Right behavior in vertical Tabs in LTR/RTL locales in line with similar components (ActionGroup).

Copy link
Contributor Author

@mariareedstrom mariareedstrom Aug 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, I'll update it to match the behaviors of similar components. However, in RTL left is next for horizontal orientation, but the semantics flip when in vertical orientation, where left then means previous. This seems confusing to the end user. Would you suggest I open an RFC for this?

Copy link
Contributor Author

@mariareedstrom mariareedstrom Aug 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to match similar component behaviors. 🙂

Copy link
Member

@LFDanLu LFDanLu Aug 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, I'll update it to match the behaviors of similar components. However, in RTL left is next for horizontal orientation, but the semantics flip when in vertical orientation, where left then means previous. This seems confusing to the end user. Would you suggest I open an RFC for this?

Good point, this makes sense to me and got me revisiting the logic. Looks like we actually had this working here where ArrowLeft/ArrowUp always moved to the previous item and ArrowRight/ArrowDown always moved to the next item in RTL (can be observed here). However, looks like the behavior changed in this PR to be what it currently is, presumably as a fix for something separate. This feels like the wrong behavior as you pointed out, would you mind opening a separate issue for this?

For this PR, we'll keep it as is to avoid scope creep and so we can discuss with the rest of the team if the current behavior is incorrect.

this.orientation = orientation;
this.flipDirection = direction === 'rtl' && orientation === 'horizontal';
this.disabledKeys = disabledKeys;
}

getKeyLeftOf(key: Key) {
if (this.flipDirection) {
return this.getNextKey(key);
} else {
if (this.orientation === 'horizontal') {
return this.getPreviousKey(key);
}
return null;
}
return this.getPreviousKey(key);
}

getKeyRightOf(key: Key) {
if (this.flipDirection) {
return this.getPreviousKey(key);
} else {
if (this.orientation === 'horizontal') {
return this.getNextKey(key);
}
return null;
}
}
return this.getNextKey(key);
}

getKeyAbove(key: Key) {
if (this.orientation === 'vertical') {
return this.getPreviousKey(key);
}
return null;
return this.getPreviousKey(key);
}

getKeyBelow(key: Key) {
if (this.orientation === 'vertical') {
return this.getNextKey(key);
}
return null;
return this.getNextKey(key);
}

getFirstKey() {
Expand Down
2 changes: 2 additions & 0 deletions packages/@react-spectrum/tabs/docs/Tabs.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,8 @@ function Example() {
### Orientation
[View guidelines](https://spectrum.adobe.com/page/tabs/#Orientation)

By default, tabs are horizontally oriented. The `orientation` prop can be set to `vertical` to change this. This does not affect keyboard navigation.

```tsx example
<Tabs aria-label="Chat log orientation example" orientation="vertical">
<TabList>
Expand Down
25 changes: 13 additions & 12 deletions packages/@react-spectrum/tabs/test/Tabs.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,11 @@ describe('Tabs', function () {
expect(ref.current.UNSAFE_getDOMNode()).toBe(tablist.parentElement.parentElement);
});

it('allows user to change tab item select via left/right arrow keys with horizontal tabs', function () {
it('allows user to change tab item select via arrow keys with horizontal tabs', function () {
let container = renderComponent({orientation: 'horizontal'});
let tablist = container.getByRole('tablist');
let tabs = within(tablist).getAllByRole('tab');
let selectedItem = tabs[0];

expect(tablist).toHaveAttribute('aria-orientation', 'horizontal');

expect(selectedItem).toHaveAttribute('aria-selected', 'true');
Expand All @@ -117,14 +116,15 @@ describe('Tabs', function () {
fireEvent.keyDown(nextSelectedItem, {key: 'ArrowLeft', code: 37, charCode: 37});
expect(selectedItem).toHaveAttribute('aria-selected', 'true');

/** Doesn't change selection because it's horizontal tabs. */
/** Changes selection regardless if it's horizontal tabs. */
fireEvent.keyDown(selectedItem, {key: 'ArrowUp', code: 38, charCode: 38});
expect(selectedItem).toHaveAttribute('aria-selected', 'true');
nextSelectedItem = tabs[2];
expect(nextSelectedItem).toHaveAttribute('aria-selected', 'true');
fireEvent.keyDown(selectedItem, {key: 'ArrowDown', code: 40, charCode: 40});
expect(selectedItem).toHaveAttribute('aria-selected', 'true');
});

it('allows user to change tab item select via up/down arrow keys with vertical tabs', function () {
it('allows user to change tab item select via arrow keys with vertical tabs', function () {
let container = renderComponent({orientation: 'vertical'});
let tablist = container.getByRole('tablist');
let tabs = within(tablist).getAllByRole('tab');
Expand All @@ -133,18 +133,19 @@ describe('Tabs', function () {

expect(tablist).toHaveAttribute('aria-orientation', 'vertical');

/** Doesn't change selection because it's vertical tabs. */
expect(selectedItem).toHaveAttribute('aria-selected', 'true');
fireEvent.keyDown(selectedItem, {key: 'ArrowRight', code: 39, charCode: 39});
expect(selectedItem).toHaveAttribute('aria-selected', 'true');
fireEvent.keyDown(selectedItem, {key: 'ArrowLeft', code: 37, charCode: 37});
expect(selectedItem).toHaveAttribute('aria-selected', 'true');

let nextSelectedItem = tabs[1];
expect(selectedItem).toHaveAttribute('aria-selected', 'true');
fireEvent.keyDown(selectedItem, {key: 'ArrowDown', code: 40, charCode: 40});
expect(nextSelectedItem).toHaveAttribute('aria-selected', 'true');
fireEvent.keyDown(nextSelectedItem, {key: 'ArrowUp', code: 38, charCode: 38});
expect(selectedItem).toHaveAttribute('aria-selected', 'true');

/** Changes selection regardless if it's vertical tabs. */
fireEvent.keyDown(selectedItem, {key: 'ArrowLeft', code: 37, charCode: 37});
nextSelectedItem = tabs[2];
expect(nextSelectedItem).toHaveAttribute('aria-selected', 'true');
fireEvent.keyDown(selectedItem, {key: 'ArrowRight', code: 39, charCode: 39});
expect(selectedItem).toHaveAttribute('aria-selected', 'true');
});

it('wraps focus from first to last/last to first item', function () {
Expand Down
2 changes: 1 addition & 1 deletion packages/react-aria-components/docs/Tabs.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ tab selection.

### Orientation

By default, tabs are horizontally oriented. The `orientation` prop can be set to `vertical` to change this. This affects keyboard navigation. You are responsible for styling your tabs accordingly.
By default, tabs are horizontally oriented. The `orientation` prop can be set to `vertical` to change this. This does not affect keyboard navigation. You are responsible for styling your tabs accordingly.

```tsx example
<Tabs orientation="vertical">
Expand Down