Skip to content

Commit

Permalink
[MenuList] Remove currentTabIndex state (mui#10847)
Browse files Browse the repository at this point in the history 
* MenuList no longer changes item tabIndex values with focus changes

* Control of item tabIndex values is no longer handled by MenuList
  • Loading branch information
@ryancogswell
ryancogswell committed Mar 24, 2019
1 parent 178fa62 commit 4155115
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 72 deletions.
5 changes: 3 additions & 2 deletions packages/material-ui/src/MenuItem/MenuItem.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,13 @@ export const styles = theme => ({
});

const MenuItem = React.forwardRef(function MenuItem(props, ref) {
const { classes, className, component, disableGutters, role, selected, ...other } = props;
const { classes, className, component, disableGutters, role, selected, tabIndex, ...other } = props;

return (
<ListItem
button
role={role}
tabIndex={-1}
tabIndex={tabIndex}
component={component}
selected={selected}
disableGutters={disableGutters}
Expand Down Expand Up @@ -89,6 +89,7 @@ MenuItem.defaultProps = {
component: 'li',
disableGutters: false,
role: 'menuitem',
tabIndex: -1,
};

export default withStyles(styles, { name: 'MuiMenuItem' })(MenuItem);
60 changes: 2 additions & 58 deletions packages/material-ui/src/MenuList/MenuList.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,31 +9,8 @@ import List from '../List';
import getScrollbarSize from '../utils/getScrollbarSize';
import { setRef } from '../utils/reactHelpers';

function resetTabIndex(list, selectedItem, setCurrentTabIndex) {
const currentFocus = ownerDocument(list).activeElement;

const items = [];
for (let i = 0; i < list.children.length; i += 1) {
items.push(list.children[i]);
}

const currentFocusIndex = items.indexOf(currentFocus);

if (currentFocusIndex !== -1) {
return setCurrentTabIndex(currentFocusIndex);
}

if (selectedItem) {
return setCurrentTabIndex(items.indexOf(selectedItem));
}

return setCurrentTabIndex(0);
}

const MenuList = React.forwardRef(function MenuList(props, ref) {
const { actions, children, className, onBlur, onKeyDown, disableListWrap, ...other } = props;
const [currentTabIndex, setCurrentTabIndex] = React.useState(null);
const blurTimeoutIDRef = React.useRef();
const { actions, children, className, onKeyDown, disableListWrap, ...other } = props;
const listRef = React.useRef();
const selectedItemRef = React.useRef();

Expand Down Expand Up @@ -69,29 +46,6 @@ const MenuList = React.forwardRef(function MenuList(props, ref) {
},
}));

React.useEffect(() => {
resetTabIndex(listRef.current, selectedItemRef.current, setCurrentTabIndex);
return () => {
clearTimeout(blurTimeoutIDRef.current);
};
}, []);

const handleBlur = event => {
blurTimeoutIDRef.current = setTimeout(() => {
if (listRef.current) {
const list = listRef.current;
const currentFocus = ownerDocument(list).activeElement;
if (!list.contains(currentFocus)) {
resetTabIndex(list, selectedItemRef.current, setCurrentTabIndex);
}
}
}, 30);

if (onBlur) {
onBlur(event);
}
};

const handleKeyDown = event => {
const list = listRef.current;
const key = event.key;
Expand Down Expand Up @@ -134,15 +88,7 @@ const MenuList = React.forwardRef(function MenuList(props, ref) {
};

const handleItemFocus = event => {
const list = listRef.current;
if (list) {
for (let i = 0; i < list.children.length; i += 1) {
if (list.children[i] === event.currentTarget) {
setCurrentTabIndex(i);
break;
}
}
}
// no-op for now, but will use for a new purpose for #8191
};

return (
Expand All @@ -155,7 +101,6 @@ const MenuList = React.forwardRef(function MenuList(props, ref) {
}}
className={className}
onKeyDown={handleKeyDown}
onBlur={handleBlur}
{...other}
>
{React.Children.map(children, (child, index) => {
Expand All @@ -172,7 +117,6 @@ const MenuList = React.forwardRef(function MenuList(props, ref) {
);

return React.cloneElement(child, {
tabIndex: index === currentTabIndex ? 0 : -1,
ref: child.props.selected
? refArg => {
// not StrictMode ready
Expand Down
25 changes: 13 additions & 12 deletions packages/material-ui/test/integration/MenuList.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,12 @@ function assertMenuItemTabIndexed(wrapper, tabIndexed) {
});
}

function assertMenuItemFocused(wrapper, tabIndexed) {
function assertMenuItemFocused(wrapper, focusedIndex) {
const items = wrapper.find('li[role="menuitem"]');
assert.strictEqual(items.length, 4);

items.forEach((item, index) => {
if (index === tabIndexed) {
if (index === focusedIndex) {
assert.strictEqual(item.find('li').instance(), document.activeElement);
}
});
Expand All @@ -80,7 +80,7 @@ describe('<MenuList> integration', () => {
item4ActionsRef = React.createRef();
wrapper = mount(
<MenuList actions={menuListActionsRef}>
<MenuItem>Menu Item 1</MenuItem>
<MenuItem tabIndex={0}>Menu Item 1</MenuItem>
<MenuItem>Menu Item 2</MenuItem>
<MenuItem>Menu Item 3</MenuItem>
<TrackRenderCountMenuItem actions={item4ActionsRef}>Menu Item 4</TrackRenderCountMenuItem>
Expand Down Expand Up @@ -108,7 +108,8 @@ describe('<MenuList> integration', () => {

it('should select the last item when pressing up', () => {
wrapper.simulate('keyDown', { key: 'ArrowUp' });
assertMenuItemTabIndexed(wrapper, 3);
assertMenuItemTabIndexed(wrapper, 0);
assertMenuItemFocused(wrapper, 3);
assertItem4RenderCount(1);
});

Expand All @@ -128,7 +129,7 @@ describe('<MenuList> integration', () => {
it('should focus the second item 1', () => {
menuListActionsRef.current.focus();
wrapper.simulate('keyDown', { key: 'ArrowDown' });
assertMenuItemTabIndexed(wrapper, 1);
assertMenuItemTabIndexed(wrapper, 0);
assertMenuItemFocused(wrapper, 1);
assertItem4RenderCount(1);
});
Expand Down Expand Up @@ -158,13 +159,13 @@ describe('<MenuList> integration', () => {

it('should focus the second item 2', () => {
wrapper.simulate('keyDown', { key: 'ArrowDown' });
assertMenuItemTabIndexed(wrapper, 1);
assertMenuItemTabIndexed(wrapper, 0);
assertMenuItemFocused(wrapper, 1);
});

it('should focus the third item', () => {
wrapper.simulate('keyDown', { key: 'ArrowDown' });
assertMenuItemTabIndexed(wrapper, 2);
assertMenuItemTabIndexed(wrapper, 0);
assertMenuItemFocused(wrapper, 2);
});

Expand All @@ -190,7 +191,7 @@ describe('<MenuList> integration', () => {
wrapper = mount(
<MenuList actions={menuListActionsRef}>
<MenuItem>Menu Item 1</MenuItem>
<MenuItem selected>Menu Item 2</MenuItem>
<MenuItem selected tabIndex={0}>Menu Item 2</MenuItem>
<MenuItem>Menu Item 3</MenuItem>
<MenuItem>Menu Item 4</MenuItem>
</MenuList>,
Expand All @@ -212,7 +213,7 @@ describe('<MenuList> integration', () => {
it('should focus the third item', () => {
menuListActionsRef.current.focus();
wrapper.simulate('keyDown', { key: 'ArrowDown' });
assertMenuItemTabIndexed(wrapper, 2);
assertMenuItemTabIndexed(wrapper, 1);
assertMenuItemFocused(wrapper, 2);
});

Expand Down Expand Up @@ -259,7 +260,7 @@ describe('<MenuList> integration', () => {
menuListActionsRef = React.createRef();
wrapper = mount(
<MenuList disableListWrap actions={menuListActionsRef}>
<MenuItem>Menu Item 1</MenuItem>
<MenuItem tabIndex={0}>Menu Item 1</MenuItem>
<MenuItem>Menu Item 2</MenuItem>
<MenuItem>Menu Item 3</MenuItem>
<MenuItem>Menu Item 4</MenuItem>
Expand All @@ -281,11 +282,11 @@ describe('<MenuList> integration', () => {
wrapper.simulate('keyDown', { key: 'ArrowDown' });
wrapper.simulate('keyDown', { key: 'ArrowDown' });
wrapper.simulate('keyDown', { key: 'ArrowDown' });
assertMenuItemTabIndexed(wrapper, 3);
assertMenuItemTabIndexed(wrapper, 0);
assertMenuItemFocused(wrapper, 3);

wrapper.simulate('keyDown', { key: 'ArrowDown' });
assertMenuItemTabIndexed(wrapper, 3);
assertMenuItemTabIndexed(wrapper, 0);
assertMenuItemFocused(wrapper, 3);
});
});
Expand Down

0 comments on commit 4155115

Please sign in to comment.