From 19d8aeb6b14060a0481fdbf4bd29987ba209eec3 Mon Sep 17 00:00:00 2001 From: Akira Sudoh Date: Fri, 27 Mar 2020 13:19:40 +0900 Subject: [PATCH] fix(Tabs): prevent update selecton upon arrow keys (#5659) This change introduces `selectionMode="manual"` prop to `` and `` that makes left/right arrow keys in those components change only the focused item, not the selection. User can use space/enter keys to update the selection. Fixes #4870. Fixes #5657. --- .../__snapshots__/PublicAPI-test.js.snap | 20 +++++ .../ContentSwitcher/ContentSwitcher-story.js | 12 ++- .../ContentSwitcher/ContentSwitcher.js | 38 +++++---- .../react/src/components/Tabs/Tabs-story.js | 18 ++++- .../react/src/components/Tabs/Tabs-test.js | 78 +++++++++++++------ packages/react/src/components/Tabs/Tabs.js | 10 ++- 6 files changed, 134 insertions(+), 42 deletions(-) diff --git a/packages/react/__tests__/__snapshots__/PublicAPI-test.js.snap b/packages/react/__tests__/__snapshots__/PublicAPI-test.js.snap index 0a760df50512..1e9014600bea 100644 --- a/packages/react/__tests__/__snapshots__/PublicAPI-test.js.snap +++ b/packages/react/__tests__/__snapshots__/PublicAPI-test.js.snap @@ -648,6 +648,7 @@ Map { "ContentSwitcher" => Object { "defaultProps": Object { "selectedIndex": 0, + "selectionMode": "automatic", }, "propTypes": Object { "children": Object { @@ -663,6 +664,15 @@ Map { "selectedIndex": Object { "type": "number", }, + "selectionMode": Object { + "args": Array [ + Array [ + "automatic", + "manual", + ], + ], + "type": "oneOf", + }, }, }, "Copy" => Object { @@ -4679,6 +4689,7 @@ Map { "iconDescription": "show menu options", "role": "navigation", "selected": 0, + "selectionMode": "automatic", "triggerHref": "#", "type": "default", }, @@ -4715,6 +4726,15 @@ Map { "selected": Object { "type": "number", }, + "selectionMode": Object { + "args": Array [ + Array [ + "automatic", + "manual", + ], + ], + "type": "oneOf", + }, "tabContentClassName": Object { "type": "string", }, diff --git a/packages/react/src/components/ContentSwitcher/ContentSwitcher-story.js b/packages/react/src/components/ContentSwitcher/ContentSwitcher-story.js index c2c54d2226bf..3fd2a44d1250 100644 --- a/packages/react/src/components/ContentSwitcher/ContentSwitcher-story.js +++ b/packages/react/src/components/ContentSwitcher/ContentSwitcher-story.js @@ -8,12 +8,22 @@ import React from 'react'; import { storiesOf } from '@storybook/react'; import { action } from '@storybook/addon-actions'; -import { withKnobs, boolean } from '@storybook/addon-knobs'; +import { withKnobs, boolean, select } from '@storybook/addon-knobs'; import ContentSwitcher from '../ContentSwitcher'; import Switch from '../Switch'; +const selectionModes = { + 'Change selection automatically upon focus (automatic)': 'automatic', + 'Change selection on explicit gesture (manual)': 'manual', +}; + const props = { contentSwitcher: () => ({ + selectionMode: select( + 'Selection mode (selectionMode)', + selectionModes, + 'automatic' + ), onChange: action('onChange'), }), switch: () => ({ diff --git a/packages/react/src/components/ContentSwitcher/ContentSwitcher.js b/packages/react/src/components/ContentSwitcher/ContentSwitcher.js index 20a8dd6058a0..4faea42eeacc 100644 --- a/packages/react/src/components/ContentSwitcher/ContentSwitcher.js +++ b/packages/react/src/components/ContentSwitcher/ContentSwitcher.js @@ -45,10 +45,16 @@ export default class ContentSwitcher extends React.Component { * Specify a selected index for the initially selected content */ selectedIndex: PropTypes.number, + + /** + * Choose whether or not to automatically change selection on focus + */ + selectionMode: PropTypes.oneOf(['automatic', 'manual']), }; static defaultProps = { selectedIndex: 0, + selectionMode: 'automatic', }; static getDerivedStateFromProps({ selectedIndex }, state) { @@ -66,6 +72,7 @@ export default class ContentSwitcher extends React.Component { }; handleChildChange = data => { + const { selectionMode } = this.props; // the currently selected child index const { selectedIndex } = this.state; // the newly selected child index @@ -73,21 +80,22 @@ export default class ContentSwitcher extends React.Component { const { key } = data; if (matches(data, [keys.ArrowRight, keys.ArrowLeft])) { - const nextIndex = getNextIndex( - key, - selectedIndex, - this.props.children.length - ); - this.setState( - { - selectedIndex: nextIndex, - }, - () => { - const switchRef = this._switchRefs[nextIndex]; - switchRef && switchRef.focus(); - this.props.onChange(data); - } - ); + const nextIndex = getNextIndex(key, index, this.props.children.length); + if (selectionMode === 'manual') { + const switchRef = this._switchRefs[nextIndex]; + switchRef && switchRef.focus(); + } else { + this.setState( + { + selectedIndex: nextIndex, + }, + () => { + const switchRef = this._switchRefs[nextIndex]; + switchRef && switchRef.focus(); + this.props.onChange(data); + } + ); + } } else { if (selectedIndex !== index) { this.setState({ selectedIndex: index }, () => { diff --git a/packages/react/src/components/Tabs/Tabs-story.js b/packages/react/src/components/Tabs/Tabs-story.js index 67a0b7f2c0ac..5aa3cbbb3917 100644 --- a/packages/react/src/components/Tabs/Tabs-story.js +++ b/packages/react/src/components/Tabs/Tabs-story.js @@ -8,7 +8,13 @@ import React from 'react'; import { storiesOf } from '@storybook/react'; import { action } from '@storybook/addon-actions'; -import { withKnobs, boolean, number, text } from '@storybook/addon-knobs'; +import { + withKnobs, + boolean, + number, + select, + text, +} from '@storybook/addon-knobs'; import { settings } from 'carbon-components'; import classNames from 'classnames'; import './Tabs-story.scss'; @@ -17,6 +23,11 @@ import Tabs from '../Tabs'; import Tab from '../Tab'; import TabsSkeleton from '../Tabs/Tabs.Skeleton'; +const selectionModes = { + 'Change selection automatically upon focus (automatic)': 'automatic', + 'Change selection on explicit gesture (manual)': 'manual', +}; + const { prefix } = settings; const props = { tabs: () => ({ @@ -39,6 +50,11 @@ const props = { 'The className for the child `` components', 'tab-content' ), + selectionMode: select( + 'Selection mode (selectionMode)', + selectionModes, + 'automatic' + ), }), tab: () => ({ disabled: boolean('Disabled (disabled in )', false), diff --git a/packages/react/src/components/Tabs/Tabs-test.js b/packages/react/src/components/Tabs/Tabs-test.js index 810d11be42a7..185732433f93 100644 --- a/packages/react/src/components/Tabs/Tabs-test.js +++ b/packages/react/src/components/Tabs/Tabs-test.js @@ -209,40 +209,56 @@ describe('Tabs', () => { }); describe('keydown', () => { - const wrapper = mount( - - - content - - - content - - - ); - - const firstTab = wrapper.find('.firstTab').last(); - const lastTab = wrapper.find('.lastTab').last(); const leftKey = 37; const rightKey = 39; const spaceKey = 32; const enterKey = 13; + let wrapper; + let firstTab; + let lastTab; + let anchorInFirstTab; + let anchorInLastTab; + let spyFocusAnchorInFirstTab; + let spyFocusAnchorInLastTab; + describe('state: selected', () => { + beforeEach(() => { + wrapper = mount( + + + content + + + content + + + ); + firstTab = wrapper.find('.firstTab').last(); + lastTab = wrapper.find('.lastTab').last(); + anchorInFirstTab = firstTab.find('a').getDOMNode(); + anchorInLastTab = lastTab.find('a').getDOMNode(); + }); + it('updates selected state when pressing arrow keys', () => { + spyFocusAnchorInFirstTab = jest.spyOn(anchorInFirstTab, 'focus'); + spyFocusAnchorInLastTab = jest.spyOn(anchorInLastTab, 'focus'); firstTab.simulate('keydown', { which: rightKey }); - expect(wrapper.state().selected).toEqual(1); + expect(spyFocusAnchorInLastTab).toHaveBeenCalled(); lastTab.simulate('keydown', { which: leftKey }); - expect(wrapper.state().selected).toEqual(0); + expect(spyFocusAnchorInFirstTab).toHaveBeenCalled(); }); it('loops focus and selected state from lastTab to firstTab', () => { + spyFocusAnchorInFirstTab = jest.spyOn(anchorInFirstTab, 'focus'); lastTab.simulate('keydown', { which: rightKey }); - expect(wrapper.state().selected).toEqual(0); + expect(spyFocusAnchorInFirstTab).toHaveBeenCalled(); }); it('loops focus and selected state from firstTab to lastTab', () => { + spyFocusAnchorInLastTab = jest.spyOn(anchorInLastTab, 'focus'); firstTab.simulate('keydown', { which: leftKey }); - expect(wrapper.state().selected).toEqual(1); + expect(spyFocusAnchorInLastTab).toHaveBeenCalled(); }); it('updates selected state when pressing space or enter key', () => { @@ -254,9 +270,6 @@ describe('Tabs', () => { }); describe('ignore disabled child tab', () => { - let wrapper; - let firstTab; - let lastTab; beforeEach(() => { wrapper = mount( @@ -273,23 +286,29 @@ describe('Tabs', () => { ); firstTab = wrapper.find('.firstTab').last(); lastTab = wrapper.find('.lastTab').last(); + anchorInFirstTab = firstTab.find('a').getDOMNode(); + anchorInLastTab = lastTab.find('a').getDOMNode(); }); it('updates selected state when pressing arrow keys', () => { + spyFocusAnchorInFirstTab = jest.spyOn(anchorInFirstTab, 'focus'); + spyFocusAnchorInLastTab = jest.spyOn(anchorInLastTab, 'focus'); firstTab.simulate('keydown', { which: rightKey }); - expect(wrapper.state().selected).toEqual(2); + expect(spyFocusAnchorInLastTab).toHaveBeenCalled(); lastTab.simulate('keydown', { which: leftKey }); - expect(wrapper.state().selected).toEqual(0); + expect(spyFocusAnchorInFirstTab).toHaveBeenCalled(); }); it('loops focus and selected state from lastTab to firstTab', () => { + spyFocusAnchorInFirstTab = jest.spyOn(anchorInFirstTab, 'focus'); wrapper.setState({ selected: 2 }); lastTab.simulate('keydown', { which: rightKey }); - expect(wrapper.state().selected).toEqual(0); + expect(spyFocusAnchorInFirstTab).toHaveBeenCalled(); }); it('loops focus and selected state from firstTab to lastTab', () => { + spyFocusAnchorInLastTab = jest.spyOn(anchorInLastTab, 'focus'); firstTab.simulate('keydown', { which: leftKey }); - expect(wrapper.state().selected).toEqual(2); + expect(spyFocusAnchorInLastTab).toHaveBeenCalled(); }); it('updates selected state when pressing space or enter key', () => { @@ -299,6 +318,17 @@ describe('Tabs', () => { expect(wrapper.state().selected).toEqual(2); }); }); + + afterEach(() => { + if (spyFocusAnchorInLastTab) { + spyFocusAnchorInLastTab.mockRestore(); + spyFocusAnchorInLastTab = null; + } + if (spyFocusAnchorInFirstTab) { + spyFocusAnchorInFirstTab.mockRestore(); + spyFocusAnchorInFirstTab = null; + } + }); }); }); diff --git a/packages/react/src/components/Tabs/Tabs.js b/packages/react/src/components/Tabs/Tabs.js index 96dafa96e7c9..c1ec3f30f962 100644 --- a/packages/react/src/components/Tabs/Tabs.js +++ b/packages/react/src/components/Tabs/Tabs.js @@ -89,6 +89,11 @@ export default class Tabs extends React.Component { * Provide a className that is applied to the components */ tabContentClassName: PropTypes.string, + + /** + * Choose whether or not to automatically change selection on focus + */ + selectionMode: PropTypes.oneOf(['automatic', 'manual']), }; static defaultProps = { @@ -98,6 +103,7 @@ export default class Tabs extends React.Component { triggerHref: '#', selected: 0, ariaLabel: 'listbox', + selectionMode: 'automatic', }; state = { @@ -184,7 +190,9 @@ export default class Tabs extends React.Component { const tab = this.getTabAt(nextIndex); if (tab && matches(evt, [keys.ArrowLeft, keys.ArrowRight])) { evt.preventDefault(); - this.selectTabAt(nextIndex, onSelectionChange); + if (this.props.selectionMode !== 'manual') { + this.selectTabAt(nextIndex, onSelectionChange); + } if (tab.tabAnchor) { tab.tabAnchor.focus(); }