Skip to content
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

Jump to bottom

feat(tabs): support home/end keyboard navigation #3258

Merged
merged 7 commits into from
Jul 9, 2019
Merged

feat(tabs): support home/end keyboard navigation #3258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4639b77
feat(tabs): add home/end key support
emyarod Jul 2, 2019
0a4af56
feat(Tab): support home/end keyboard navigation
emyarod Jul 2, 2019
3dce3e1
refactor(Tabs): use keyboard event handler helper function
emyarod Jul 2, 2019
2190ad4
test(Tabs): use key objects
emyarod Jul 8, 2019
813273e
Merge branch 'master' into 1495-tabs-keyboard-a11y
aledavila Jul 9, 2019
5731741
Merge branch 'master' into 1495-tabs-keyboard-a11y
joshblack Jul 9, 2019
bfdc52a
Merge branch 'master' into 1495-tabs-keyboard-a11y
asudoh Jul 9, 2019
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
29 changes: 24 additions & 5 deletions packages/components/src/components/tabs/tabs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,21 +114,40 @@ class Tab extends ContentSwitcher {
_handleKeyDown(event) {
const triggerNode = eventMatches(event, this.options.selectorTrigger);
if (triggerNode) {
// enter
if (event.which === 13) {
emyarod marked this conversation as resolved.
Show resolved Hide resolved
this._updateMenuState();
}
return;
}

const buttons = toArray(
this.element.querySelectorAll(this.options.selectorButtonEnabled)
);

// End or Home key handler
if (event.which === 35 || event.which === 36) {
emyarod marked this conversation as resolved.
Show resolved Hide resolved
event.preventDefault();
const activeIndex = {
35: buttons.length - 1,
36: 0,
}[event.which];
emyarod marked this conversation as resolved.
Show resolved Hide resolved
this.setActive(buttons[activeIndex], (error, item) => {
if (item) {
const link = item.querySelector(this.options.selectorLink);
if (link) {
link.focus();
}
}
});
}

const direction = {
37: this.constructor.NAVIGATE.BACKWARD,
39: this.constructor.NAVIGATE.FORWARD,
37: this.constructor.NAVIGATE.BACKWARD, // left arrow
39: this.constructor.NAVIGATE.FORWARD, // right arrow
}[event.which];

if (direction) {
const buttons = toArray(
this.element.querySelectorAll(this.options.selectorButtonEnabled)
);
const button = this.element.querySelector(
this.options.selectorButtonSelected
);
Expand Down
32 changes: 32 additions & 0 deletions packages/components/tests/spec/tabs_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,38 @@ describe('Test tabs', function() {
).toBe(true);
});

it('Should update active tab on end key', function() {
const defaultPrevented = element.dispatchEvent(
Object.assign(new CustomEvent('keydown'), { which: 35 })
);
expect(defaultPrevented).toBe(true);
expect(
buttonNodes[0].classList.contains('bx--tabs__nav-item--selected')
).toBe(false);
expect(
buttonNodes[1].classList.contains('bx--tabs__nav-item--selected')
).toBe(false);
expect(
buttonNodes[2].classList.contains('bx--tabs__nav-item--selected')
).toBe(true);
});

it('Should update active tab on home key', function() {
const defaultPrevented = element.dispatchEvent(
Object.assign(new CustomEvent('keydown'), { which: 36 })
);
expect(defaultPrevented).toBe(true);
expect(
buttonNodes[0].classList.contains('bx--tabs__nav-item--selected')
).toBe(true);
expect(
buttonNodes[1].classList.contains('bx--tabs__nav-item--selected')
).toBe(false);
expect(
buttonNodes[2].classList.contains('bx--tabs__nav-item--selected')
).toBe(false);
});

it('Should focus on the new active tab upon keyboard navigation', function() {
const link = document.createElement('a');
spyOn(link, 'focus');
Expand Down
11 changes: 3 additions & 8 deletions packages/react/src/components/Tab/Tab.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import React from 'react';
import classNames from 'classnames';
import { settings } from 'carbon-components';
import TabContent from '../TabContent';
import { keys, matches } from '../../internal/keyboard';

const { prefix } = settings;

Expand Down Expand Up @@ -109,14 +110,8 @@ export default class Tab extends React.Component {
};

setTabFocus(evt) {
const leftKey = 37;
const rightKey = 39;
if (evt.which === leftKey) {
this.props.handleTabAnchorFocus(this.props.index - 1);
} else if (evt.which === rightKey) {
this.props.handleTabAnchorFocus(this.props.index + 1);
} else {
return;
if (matches(evt, [keys.ArrowLeft, keys.ArrowRight, keys.End, keys.Home])) {
this.props.handleTabAnchorFocus(evt, { index: this.props.index });
}
}

Expand Down
39 changes: 27 additions & 12 deletions packages/react/src/components/Tabs/Tabs-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,19 @@

import React from 'react';
import { ChevronDownGlyph } from '@carbon/icons-react';
import { settings } from 'carbon-components';
import { shallow, mount } from 'enzyme';
import Tabs from '../Tabs';
import Tab from '../Tab';
import TabsSkeleton from '../Tabs/Tabs.Skeleton';
import { shallow, mount } from 'enzyme';
import { settings } from 'carbon-components';
import {
ArrowLeft,
ArrowRight,
End,
Enter,
Home,
Space,
} from '../../internal/keyboard/keys';

const { prefix } = settings;

Expand Down Expand Up @@ -197,33 +205,40 @@ describe('Tabs', () => {

const firstTab = wrapper.find('.firstTab').last();
const lastTab = wrapper.find('.lastTab').last();
const leftKey = 37;
const rightKey = 39;
const spaceKey = 32;
const enterKey = 13;

describe('state: selected', () => {
it('updates selected state when pressing arrow keys', () => {
firstTab.simulate('keydown', { which: rightKey });
firstTab.simulate('keydown', { ...ArrowRight });
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
firstTab.simulate('keydown', { ...ArrowRight });
firstTab.simulate('keydown', ArrowRight);

If you want! Although I'm sure you had a good reason for the spread 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

just to get all of the properties in the simulated event and not just which

Copy link
Contributor

@joshblack joshblack Jul 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@emyarod not sure I understand, it seems like you're calling out going from which to ArrorRight, I was just suggesting there was no need to spread the object as it works inline as well

expect(wrapper.state().selected).toEqual(1);
lastTab.simulate('keydown', { which: leftKey });
lastTab.simulate('keydown', { ...ArrowLeft });
expect(wrapper.state().selected).toEqual(0);
});

it('loops focus and selected state from lastTab to firstTab', () => {
lastTab.simulate('keydown', { which: rightKey });
lastTab.simulate('keydown', { ...ArrowRight });
expect(wrapper.state().selected).toEqual(0);
});

it('loops focus and selected state from firstTab to lastTab', () => {
firstTab.simulate('keydown', { which: leftKey });
firstTab.simulate('keydown', { ...ArrowLeft });
expect(wrapper.state().selected).toEqual(1);
});

it('updates selected state when pressing the End key', () => {
firstTab.simulate('keydown', { ...End });
expect(wrapper.state().selected).toEqual(1);
});

it('updates selected state when pressing the Home key', () => {
wrapper.setState({ selected: 1 });
lastTab.simulate('keydown', { ...Home });
expect(wrapper.state().selected).toEqual(0);
});

it('updates selected state when pressing space or enter key', () => {
firstTab.simulate('keydown', { which: spaceKey });
firstTab.simulate('keydown', { ...Space });
expect(wrapper.state().selected).toEqual(0);
lastTab.simulate('keydown', { which: enterKey });
lastTab.simulate('keydown', { ...Enter });
expect(wrapper.state().selected).toEqual(1);
});
});
Expand Down
78 changes: 47 additions & 31 deletions packages/react/src/components/Tabs/Tabs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import React from 'react';
import classNames from 'classnames';
import { ChevronDownGlyph } from '@carbon/icons-react';
import { settings } from 'carbon-components';
import { keys, matches } from '../../internal/keyboard';

const { prefix } = settings;

Expand Down Expand Up @@ -106,9 +107,10 @@ export default class Tabs extends React.Component {
};
}

getTabs() {
return React.Children.map(this.props.children, tab => tab);
}
getTabs = () =>
React.Children.map(this.props.children, (tab, index) =>
React.cloneElement(tab, { index })
);

getTabAt = (index, useFresh) => {
return (
Expand All @@ -133,38 +135,52 @@ export default class Tabs extends React.Component {
};
};

handleTabKeyDown = onSelectionChange => {
return (index, evt) => {
const key = evt.key || evt.which;

if (key === 'Enter' || key === 13 || key === ' ' || key === 32) {
this.selectTabAt(index, onSelectionChange);
this.setState({
dropdownHidden: true,
});
}
};
handleTabKeyDown = onSelectionChange => (index, evt) => {
if (matches(evt, [keys.Enter, keys.Space])) {
this.selectTabAt(index, onSelectionChange);
this.setState({
dropdownHidden: true,
});
}
};

handleTabAnchorFocus = onSelectionChange => {
return index => {
const tabCount = React.Children.count(this.props.children) - 1;
let tabIndex = index;
if (index < 0) {
tabIndex = tabCount;
} else if (index > tabCount) {
tabIndex = 0;
handleTabAnchorFocus = onSelectionChange => (evt, { index }) => {
const newIndex = (() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be wrapped in an IIFE?

Copy link
Member Author

Choose a reason for hiding this comment

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

not really, but we immediately execute the function and assign the return value to newIndex this way

Copy link
Contributor

Choose a reason for hiding this comment

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

@emyarod right, I'm just curious why we need to do this versus hoisting it to the parent scope?

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure I fully understand your question, but it references index so it needs to be within the scope of handleTabAnchorFocus

const validTabIndices = this.getTabs().reduce(
(acc, curr) =>
!curr.props.disabled ? [...acc, curr.props.index] : acc,
[]
);
// index of current tab in the array of valid tab indices
const currentValidTabIndex = validTabIndices.findIndex(
validIndex => validIndex === index
);
if (matches(evt, [keys.ArrowLeft])) {
return currentValidTabIndex - 1 < 0
? validTabIndices[validTabIndices.length - 1]
: validTabIndices[currentValidTabIndex - 1];
}

const tab = this.getTabAt(tabIndex);

if (tab) {
this.selectTabAt(tabIndex, onSelectionChange);
if (tab.tabAnchor) {
tab.tabAnchor.focus();
}
if (matches(evt, [keys.ArrowRight])) {
return currentValidTabIndex + 1 > validTabIndices.length - 1
? validTabIndices[0]
: validTabIndices[currentValidTabIndex + 1];
}
};
if (matches(evt, [keys.End])) {
return validTabIndices[validTabIndices.length - 1];
}
if (matches(evt, [keys.Home])) {
return validTabIndices[0];
}
return null;
})();
const tab = this.getTabAt(newIndex);

if (tab) {
this.selectTabAt(newIndex, onSelectionChange);
if (tab.tabAnchor) {
tab.tabAnchor.focus();
}
}
};

handleDropdownClick = () => {
Expand Down