From b878ccba9aa79c17eb8f7757e8bc41b68db0e67c Mon Sep 17 00:00:00 2001 From: Silviu Avram Date: Wed, 4 Dec 2019 16:11:41 +0100 Subject: [PATCH 1/5] refactored split and menu buttons --- .../SplitButton/splitButtonBehavior.ts | 16 +++---- .../behaviors/splitButtonBehavior-test.tsx | 2 +- .../src/components/MenuButton/MenuButton.tsx | 19 ++++---- packages/react/src/components/Popup/Popup.tsx | 15 +++++- .../SplitButton/SplitButton-test.tsx | 47 +++++++++++++++---- 5 files changed, 71 insertions(+), 28 deletions(-) diff --git a/packages/accessibility/src/behaviors/SplitButton/splitButtonBehavior.ts b/packages/accessibility/src/behaviors/SplitButton/splitButtonBehavior.ts index d78270bb6..10d155c43 100644 --- a/packages/accessibility/src/behaviors/SplitButton/splitButtonBehavior.ts +++ b/packages/accessibility/src/behaviors/SplitButton/splitButtonBehavior.ts @@ -20,27 +20,27 @@ import menuButtonBehavior from '../MenuButton/menuButtonBehavior' * Adds attribute 'aria-haspopup=true' to 'toggleButton' slot. */ const splitButtonBehavior: Accessibility = props => { - const splitButtonMenuButtonBehavior = props => { + const splitButtonMenuButtonBehavior = () => { const menuButtonBehaviorData = menuButtonBehavior(props) menuButtonBehaviorData.attributes.trigger['aria-haspopup'] = undefined return _.merge(menuButtonBehaviorData, { keyActions: { - trigger: { - open: { - keyCombinations: [{ keyCode: keyboardKey.ArrowDown, altKey: true }], - }, - }, popup: { closeAndFocusTrigger: { keyCombinations: [ { keyCode: keyboardKey.Escape }, { keyCode: keyboardKey.ArrowUp, altKey: true }, - { keyCode: keyboardKey.Tab, shiftKey: false }, - { keyCode: keyboardKey.Tab, shiftKey: true }, ], }, }, + root: { + ...(!props.open && { + openAndFocusFirst: { + keyCombinations: [{ keyCode: keyboardKey.ArrowDown, altKey: true }], + }, + }), + }, }, }) } diff --git a/packages/accessibility/test/behaviors/splitButtonBehavior-test.tsx b/packages/accessibility/test/behaviors/splitButtonBehavior-test.tsx index 652ece909..3a915ae66 100644 --- a/packages/accessibility/test/behaviors/splitButtonBehavior-test.tsx +++ b/packages/accessibility/test/behaviors/splitButtonBehavior-test.tsx @@ -37,7 +37,7 @@ describe('SplitButtonBehavior.ts', () => { const property = {} const supportedKeys = [{ altKey: true, keyCode: keyboardKey.ArrowDown }] const keysFromBehavior = splitButtonBehavior(property)['childBehaviors']['menuButton'](property) - .keyActions.trigger.open.keyCombinations + .keyActions.root.openAndFocusFirst.keyCombinations verifyKeys(supportedKeys, keysFromBehavior) }) }) diff --git a/packages/react/src/components/MenuButton/MenuButton.tsx b/packages/react/src/components/MenuButton/MenuButton.tsx index c43e804e0..4982e74f2 100644 --- a/packages/react/src/components/MenuButton/MenuButton.tsx +++ b/packages/react/src/components/MenuButton/MenuButton.tsx @@ -169,24 +169,23 @@ export default class MenuButton extends AutoControlledComponent() actionHandlers = { - closeMenu: () => this.closeMenu(), + closeMenu: e => this.closeMenu(e), openAndFocusFirst: e => this.openAndFocus(e, 'first'), openAndFocusLast: e => this.openAndFocus(e, 'last'), } - closeMenu() { - this.setState({ open: false }) + closeMenu(e: React.KeyboardEvent) { + this.handleOpenChange(e, false) } openAndFocus(e: React.KeyboardEvent, which: 'first' | 'last') { - const renderCallback = () => focusMenuItem(this.menuRef.current, which) - this.setState({ open: true }, renderCallback) e.preventDefault() + this.handleOpenChange(e, true, () => focusMenuItem(this.menuRef.current, which)) } - handleOpenChange = (e, { open }: PopupProps) => { + handleOpenChange = (e: React.SyntheticEvent, open: boolean, callback?: () => void) => { _.invoke(this.props, 'onOpenChange', e, { ...this.props, open }) - this.setState({ open }) + this.setState({ open }, callback) } handleMenuOverrides = (predefinedProps?: MenuProps) => ({ @@ -195,7 +194,7 @@ export default class MenuButton extends AutoControlledComponent accessibility, open: this.state.open, - onOpenChange: this.handleOpenChange, + onOpenChange: (e, { open }) => { + this.handleOpenChange(e, open) + }, content: { styles: styles.popupContent, content: content && {content}, diff --git a/packages/react/src/components/Popup/Popup.tsx b/packages/react/src/components/Popup/Popup.tsx index d6dda2552..38c9a751c 100644 --- a/packages/react/src/components/Popup/Popup.tsx +++ b/packages/react/src/components/Popup/Popup.tsx @@ -195,6 +195,7 @@ export default class Popup extends AutoControlledComponent { + e.preventDefault() this.close(e, () => _.invoke(this.triggerFocusableDomElement, 'focus')) }, close: e => { @@ -214,7 +215,12 @@ export default class Popup extends AutoControlledComponent { const toggleButton = getToggleButton(wrapper) toggleButton.simulate('click') - expect(getMenuItems(wrapper).length).toBe(mockMenu.items.length) + expect(getMenuItems(wrapper)).toHaveLength(mockMenu.items.length) toggleButton.simulate('click') - expect(getMenuItems(wrapper).length).toBe(0) + expect(getMenuItems(wrapper)).toHaveLength(0) }) test('is false when clicking menu item', () => { @@ -41,31 +41,60 @@ describe('SplitButton', () => { getMenuItems(wrapper) .at(0) .simulate('click') - expect(getMenuItems(wrapper).length).toBe(0) + expect(getMenuItems(wrapper)).toHaveLength(0) }) - test('is open when Alt+ArrowDown is sent to the main button', () => { + test('is true when Alt+ArrowDown is sent to the main button', () => { const wrapper = mountWithProvider() getMainButton(wrapper).simulate('keydown', { keyCode: keyboardKey.ArrowDown, altKey: true }) - expect(getMenuItems(wrapper).length).toBe(mockMenu.items.length) + expect(getMenuItems(wrapper)).toHaveLength(mockMenu.items.length) }) - test('is open when Alt+ArrowUp is sent to the menu', () => { + test('is false when Alt+ArrowUp is sent to the menu', () => { const wrapper = mountWithProvider() getMenu(wrapper).simulate('keydown', { keyCode: keyboardKey.ArrowUp, altKey: true }) - expect(getMenuItems(wrapper).length).toBe(0) + expect(getMenuItems(wrapper)).toHaveLength(0) + expect(document.activeElement).toBe(getMainButton(wrapper).getDOMNode()) }) - test('is open when Escape is sent to the menu', () => { + test('is false when Escape is sent to the menu', () => { const wrapper = mountWithProvider() getMenu(wrapper).simulate('keydown', { keyCode: keyboardKey.Escape }) - expect(getMenuItems(wrapper).length).toBe(0) + expect(getMenuItems(wrapper)).toHaveLength(0) + expect(document.activeElement).toBe(getMainButton(wrapper).getDOMNode()) + }) + + test('is false when Tab is sent to the menu', () => { + const wrapper = mountWithProvider() + + getMenu(wrapper).simulate('keydown', { keyCode: keyboardKey.Tab, shiftKey: false }) + + expect(getMenuItems(wrapper)).toHaveLength(0) + }) + + test('is false when Enter is sent to the item', () => { + const wrapper = mountWithProvider() + + getMenu(wrapper).simulate('keydown', { keyCode: keyboardKey.Enter }) + getMenuItems(wrapper) + .at(0) + .simulate('click') + + expect(getMenuItems(wrapper)).toHaveLength(0) + }) + + test('is false when Shift+Tab is sent to the menu', () => { + const wrapper = mountWithProvider() + + getMenu(wrapper).simulate('keydown', { keyCode: keyboardKey.Tab, shiftKey: true }) + + expect(getMenuItems(wrapper)).toHaveLength(0) }) }) From 5ddd4e3721f6555f883b1fe594ee66442bc1880f Mon Sep 17 00:00:00 2001 From: Silviu Avram Date: Wed, 4 Dec 2019 17:04:13 +0100 Subject: [PATCH 2/5] test description fix --- .../test/specs/components/SplitButton/SplitButton-test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/test/specs/components/SplitButton/SplitButton-test.tsx b/packages/react/test/specs/components/SplitButton/SplitButton-test.tsx index d8f646e95..ff49d6bdd 100644 --- a/packages/react/test/specs/components/SplitButton/SplitButton-test.tsx +++ b/packages/react/test/specs/components/SplitButton/SplitButton-test.tsx @@ -78,7 +78,7 @@ describe('SplitButton', () => { expect(getMenuItems(wrapper)).toHaveLength(0) }) - test('is false when Enter is sent to the item', () => { + test('is false when Enter is sent to the menu', () => { const wrapper = mountWithProvider() getMenu(wrapper).simulate('keydown', { keyCode: keyboardKey.Enter }) From 5f0e1519ba4fddd4bad40207b99c9d2310939ca2 Mon Sep 17 00:00:00 2001 From: Silviu Avram Date: Thu, 5 Dec 2019 09:46:42 +0100 Subject: [PATCH 3/5] update behavior test description --- .../src/behaviors/SplitButton/splitButtonBehavior.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/accessibility/src/behaviors/SplitButton/splitButtonBehavior.ts b/packages/accessibility/src/behaviors/SplitButton/splitButtonBehavior.ts index 10d155c43..a5e1d5733 100644 --- a/packages/accessibility/src/behaviors/SplitButton/splitButtonBehavior.ts +++ b/packages/accessibility/src/behaviors/SplitButton/splitButtonBehavior.ts @@ -13,7 +13,7 @@ import menuButtonBehavior from '../MenuButton/menuButtonBehavior' * Adds attribute 'id=menu-id' based on the property 'menuId' to 'menu' slot. * Adds attribute 'aria-labelledby=trigger-id' based on the property 'triggerId' to 'menu' slot. * Triggers 'closeAndFocusTrigger' action with 'Escape' or 'altKey'+'ArrowUp' or 'Tab' or 'Shift'+'Tab' on 'popup' slot. - * Triggers 'open' action with 'altKey'+'ArrowDown' on 'trigger' slot. + * Triggers 'openAndFocusFirst' action with 'altKey'+'ArrowDown' on 'root' slot. * * @specification * Adds attribute 'tabIndex=-1' to 'toggleButton' slot. From 2bb9b3c82b36d88366d0ba4562971b016184e4eb Mon Sep 17 00:00:00 2001 From: Silviu Avram Date: Thu, 5 Dec 2019 10:09:41 +0100 Subject: [PATCH 4/5] update another description --- .../src/behaviors/SplitButton/splitButtonBehavior.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/accessibility/src/behaviors/SplitButton/splitButtonBehavior.ts b/packages/accessibility/src/behaviors/SplitButton/splitButtonBehavior.ts index a5e1d5733..b1573d169 100644 --- a/packages/accessibility/src/behaviors/SplitButton/splitButtonBehavior.ts +++ b/packages/accessibility/src/behaviors/SplitButton/splitButtonBehavior.ts @@ -12,7 +12,7 @@ import menuButtonBehavior from '../MenuButton/menuButtonBehavior' * Adds attribute 'id=trigger-id' based on the property 'triggerId' to 'trigger' slot. * Adds attribute 'id=menu-id' based on the property 'menuId' to 'menu' slot. * Adds attribute 'aria-labelledby=trigger-id' based on the property 'triggerId' to 'menu' slot. - * Triggers 'closeAndFocusTrigger' action with 'Escape' or 'altKey'+'ArrowUp' or 'Tab' or 'Shift'+'Tab' on 'popup' slot. + * Triggers 'closeAndFocusTrigger' action with 'Escape' or 'altKey'+'ArrowUp'. * Triggers 'openAndFocusFirst' action with 'altKey'+'ArrowDown' on 'root' slot. * * @specification From 51f7bb2a8252a554e1811b4f56598aafada384b2 Mon Sep 17 00:00:00 2001 From: Silviu Avram Date: Thu, 5 Dec 2019 16:29:32 +0100 Subject: [PATCH 5/5] add changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ea05d4131..6fa9f2823 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Accessibility `splitButton` & `menuButton` - screen reader fixes @kolaps33 ([#2090](https://github.com/microsoft/fluent-ui-react/pull/2090)) - Accessibility `menuButton` add aria-controls attribute based on `open` prop @kolaps33 ([#2107](https://github.com/microsoft/fluent-ui-react/pull/2107)) - Fix the 500 color variant for `steelLight` in category color scheme @natashamayurshah ([#2089](https://github.com/microsoft/fluent-ui-react/pull/2089)) +- Fix focus handling cases in `MenuButton` and `SplitButton` @silviuavram ([#2145](https://github.com/microsoft/fluent-ui-react/pull/2145)) ### Features - Add `Table` component base implementation @pompomon ([#2099](https://github.com/microsoft/fluent-ui-react/pull/2099))