From f4137ce16652b6f4a442fcb3f1a32a7e610de134 Mon Sep 17 00:00:00 2001 From: Ryan Cogswell <287804+ryancogswell@users.noreply.github.com> Date: Tue, 5 Mar 2019 21:40:53 -0600 Subject: [PATCH 1/3] [MenuList] Remove focus method and test dependencies on instance methods (#14756) * Remove test dependencies on focus, setTabIndex, and resetTabIndex * Remove focus method from MenuList * Increase MenuList branch coverage from 94.64% to 100% --- packages/material-ui/src/MenuList/MenuList.js | 14 --- .../test/integration/MenuList.test.js | 105 +++++++++++++----- 2 files changed, 78 insertions(+), 41 deletions(-) diff --git a/packages/material-ui/src/MenuList/MenuList.js b/packages/material-ui/src/MenuList/MenuList.js index ac8e154d1e96b1..8af064bd1067ad 100644 --- a/packages/material-ui/src/MenuList/MenuList.js +++ b/packages/material-ui/src/MenuList/MenuList.js @@ -93,20 +93,6 @@ class MenuList extends React.Component { } }; - focus() { - const { currentTabIndex } = this.state; - const list = this.listRef; - if (!list || !list.children || !list.firstChild) { - return; - } - - if (currentTabIndex && currentTabIndex >= 0) { - list.children[currentTabIndex].focus(); - } else { - list.firstChild.focus(); - } - } - resetTabIndex() { const list = this.listRef; const currentFocus = ownerDocument(list).activeElement; diff --git a/packages/material-ui/test/integration/MenuList.test.js b/packages/material-ui/test/integration/MenuList.test.js index c2db32cac92b55..c6e0d6b3aaf63b 100644 --- a/packages/material-ui/test/integration/MenuList.test.js +++ b/packages/material-ui/test/integration/MenuList.test.js @@ -1,10 +1,19 @@ -import React from 'react'; +import React, {useRef, useLayoutEffect} from 'react'; import { assert } from 'chai'; import { spy } from 'sinon'; import MenuList from 'packages/material-ui/src/MenuList'; import MenuItem from 'packages/material-ui/src/MenuItem'; +import RootRef from 'packages/material-ui/src/RootRef'; import { createMount } from 'packages/material-ui/src/test-utils'; +function FocusOnMountMenuItem(props) { + const listItemRef = useRef(); + useLayoutEffect(()=> { + listItemRef.current.focus(); + }, []); + return ; +} + function assertMenuItemTabIndexed(wrapper, tabIndexed) { const items = wrapper.find('li[role="menuitem"]'); assert.strictEqual(items.length, 4); @@ -33,6 +42,10 @@ function assertMenuItemFocused(wrapper, tabIndexed) { }); } +function initializeFocus(wrapper) { + wrapper.find('[tabIndex=0]').first().getDOMNode().focus(); +} + describe(' integration', () => { let mount; @@ -52,7 +65,7 @@ describe(' integration', () => { Menu Item 1 Menu Item 2 - Menu Item 2 + Menu Item 3 Menu Item 4 , ); @@ -65,7 +78,7 @@ describe(' integration', () => { }); it('should select/focus the first item 1', () => { - wrapper.instance().focus(); + initializeFocus(wrapper); assertMenuItemTabIndexed(wrapper, 0); assertMenuItemFocused(wrapper, 0); }); @@ -87,7 +100,7 @@ describe(' integration', () => { }); it('should focus the second item 1', () => { - wrapper.instance().focus(); + initializeFocus(wrapper); wrapper.simulate('keyDown', { key: 'ArrowDown' }); assertMenuItemTabIndexed(wrapper, 1); assertMenuItemFocused(wrapper, 1); @@ -110,20 +123,8 @@ describe(' integration', () => { }, 60); }); - it('should reset the tabIndex to the focused element when calling resetTabIndex', () => { - wrapper.instance().focus(); - wrapper.simulate('keyDown', { key: 'ArrowDown' }); - wrapper.instance().setTabIndex(2); - wrapper.instance().resetTabIndex(); - - assertMenuItemTabIndexed(wrapper, 1); - assertMenuItemFocused(wrapper, 1); - - resetWrapper(); - }); - it('should select/focus the first item 2', () => { - wrapper.instance().focus(); + initializeFocus(wrapper); assertMenuItemTabIndexed(wrapper, 0); assertMenuItemFocused(wrapper, 0); }); @@ -161,7 +162,7 @@ describe(' integration', () => { Menu Item 1 Menu Item 2 - Menu Item 2 + Menu Item 3 Menu Item 4 , ); @@ -174,13 +175,13 @@ describe(' integration', () => { }); it('should select/focus the second item', () => { - wrapper.instance().focus(); + initializeFocus(wrapper); assertMenuItemTabIndexed(wrapper, 1); assertMenuItemFocused(wrapper, 1); }); it('should focus the third item', () => { - wrapper.instance().focus(); + initializeFocus(wrapper); wrapper.simulate('keyDown', { key: 'ArrowDown' }); assertMenuItemTabIndexed(wrapper, 2); assertMenuItemFocused(wrapper, 2); @@ -199,14 +200,64 @@ describe(' integration', () => { }); }); - it('should not crash and burn when calling focus() on an empty MenuList', () => { - const wrapper = mount(); - wrapper.instance().focus(); + describe('MenuItem with focus on mount', () => { + let wrapper; + + const resetWrapper = () => { + wrapper = mount( + + Menu Item 1 + Menu Item 2 + Menu Item 3 + Menu Item 4 + , + ); + }; + + before(resetWrapper); + + it('should have the 3nd item tabIndexed and focused', () => { + assertMenuItemTabIndexed(wrapper, 2); + assertMenuItemFocused(wrapper, 2); + }); + }); - it('should not crash and burn when calling focus() on an unmounted MenuList', () => { - const wrapper = mount(); - delete wrapper.instance().list; - wrapper.instance().focus(); + describe('MenuList with disableListWrap', () => { + let wrapper; + + const resetWrapper = () => { + wrapper = mount( + + Menu Item 1 + Menu Item 2 + Menu Item 3 + Menu Item 4 + , + ); + }; + + before(resetWrapper); + + it('should not wrap focus with ArrowUp from first', () => { + initializeFocus(wrapper); + wrapper.simulate('keyDown', { key: 'ArrowUp' }); + assertMenuItemTabIndexed(wrapper, 0); + assertMenuItemFocused(wrapper, 0); + }); + it('should not wrap focus with ArrowDown from last', () => { + initializeFocus(wrapper); + wrapper.simulate('keyDown', { key: 'ArrowDown' }); + wrapper.simulate('keyDown', { key: 'ArrowDown' }); + wrapper.simulate('keyDown', { key: 'ArrowDown' }); + assertMenuItemTabIndexed(wrapper, 3); + assertMenuItemFocused(wrapper, 3); + + wrapper.simulate('keyDown', { key: 'ArrowDown' }); + assertMenuItemTabIndexed(wrapper, 3); + assertMenuItemFocused(wrapper, 3); + }); + }); + }); From 233d21a85d443d73a84a17c6db825665ac41e140 Mon Sep 17 00:00:00 2001 From: Ryan Cogswell <287804+ryancogswell@users.noreply.github.com> Date: Tue, 5 Mar 2019 21:45:21 -0600 Subject: [PATCH 2/3] [test] Formatting fixes (yarn prettier) --- .../test/integration/MenuList.test.js | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/packages/material-ui/test/integration/MenuList.test.js b/packages/material-ui/test/integration/MenuList.test.js index c6e0d6b3aaf63b..7b49bda1ef92f0 100644 --- a/packages/material-ui/test/integration/MenuList.test.js +++ b/packages/material-ui/test/integration/MenuList.test.js @@ -1,4 +1,4 @@ -import React, {useRef, useLayoutEffect} from 'react'; +import React, { useRef, useLayoutEffect } from 'react'; import { assert } from 'chai'; import { spy } from 'sinon'; import MenuList from 'packages/material-ui/src/MenuList'; @@ -8,10 +8,14 @@ import { createMount } from 'packages/material-ui/src/test-utils'; function FocusOnMountMenuItem(props) { const listItemRef = useRef(); - useLayoutEffect(()=> { + useLayoutEffect(() => { listItemRef.current.focus(); }, []); - return ; + return ( + + + + ); } function assertMenuItemTabIndexed(wrapper, tabIndexed) { @@ -43,7 +47,11 @@ function assertMenuItemFocused(wrapper, tabIndexed) { } function initializeFocus(wrapper) { - wrapper.find('[tabIndex=0]').first().getDOMNode().focus(); + wrapper + .find('[tabIndex=0]') + .first() + .getDOMNode() + .focus(); } describe(' integration', () => { @@ -220,7 +228,6 @@ describe(' integration', () => { assertMenuItemTabIndexed(wrapper, 2); assertMenuItemFocused(wrapper, 2); }); - }); describe('MenuList with disableListWrap', () => { @@ -257,7 +264,5 @@ describe(' integration', () => { assertMenuItemTabIndexed(wrapper, 3); assertMenuItemFocused(wrapper, 3); }); - }); - }); From 8db8c28dcda43f9cf42e7cabdc383519411495a9 Mon Sep 17 00:00:00 2001 From: Ryan Cogswell <287804+ryancogswell@users.noreply.github.com> Date: Wed, 6 Mar 2019 09:11:42 -0600 Subject: [PATCH 3/3] [test] Code consistency fixes --- packages/material-ui/test/integration/MenuList.test.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/material-ui/test/integration/MenuList.test.js b/packages/material-ui/test/integration/MenuList.test.js index 7b49bda1ef92f0..95f492ba06772e 100644 --- a/packages/material-ui/test/integration/MenuList.test.js +++ b/packages/material-ui/test/integration/MenuList.test.js @@ -1,4 +1,4 @@ -import React, { useRef, useLayoutEffect } from 'react'; +import React from 'react'; import { assert } from 'chai'; import { spy } from 'sinon'; import MenuList from 'packages/material-ui/src/MenuList'; @@ -7,8 +7,8 @@ import RootRef from 'packages/material-ui/src/RootRef'; import { createMount } from 'packages/material-ui/src/test-utils'; function FocusOnMountMenuItem(props) { - const listItemRef = useRef(); - useLayoutEffect(() => { + const listItemRef = React.useRef(); + React.useLayoutEffect(() => { listItemRef.current.focus(); }, []); return ( @@ -252,6 +252,7 @@ describe(' integration', () => { assertMenuItemTabIndexed(wrapper, 0); assertMenuItemFocused(wrapper, 0); }); + it('should not wrap focus with ArrowDown from last', () => { initializeFocus(wrapper); wrapper.simulate('keyDown', { key: 'ArrowDown' });