Skip to content

Commit

Permalink
[MenuList] Skip dividers and disabled menu items during keyboard focu…
Browse files Browse the repository at this point in the history 
…s navigation (#13708 and #13464)

* [test] Improve maintainability of test scenario descriptions

* [ButtonBase] Use tabIndex of -2 for disabled

* [MenuItem] Use tabIndex of -2 for disabled

* [Divider] Use tabIndex of -2

* [MenuList] Skip tabIndex less than -1 in keyboard focus navigation
  • Loading branch information
@ryancogswell
ryancogswell committed Apr 18, 2019
1 parent 8445fb1 commit 7fa46a4
Show file tree
Hide file tree
Showing 7 changed files with 209 additions and 25 deletions.
3 changes: 2 additions & 1 deletion packages/material-ui/src/ButtonBase/ButtonBase.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ class ButtonBase extends React.Component {
buttonProps['aria-disabled'] = disabled;
}

// tabIndex < -1 for disabled button indicates to MenuList to skip it in keyboard focus navigation
return (
<ComponentProp
className={className}
Expand All @@ -309,7 +310,7 @@ class ButtonBase extends React.Component {
setRef(buttonRef, ref);
setRef(innerRef, ref);
}}
tabIndex={disabled ? '-1' : tabIndex}
tabIndex={disabled ? '-2' : tabIndex}
{...buttonProps}
{...other}
>
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/src/ButtonBase/ButtonBase.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ describe('<ButtonBase />', () => {
describe('prop: disabled', () => {
it('should not receive the focus', () => {
const wrapper = mount(<ButtonBase disabled>Hello</ButtonBase>);
assert.strictEqual(wrapper.find('button').props().tabIndex, '-1');
assert.strictEqual(wrapper.find('button').props().tabIndex, '-2');
});

it('should also apply it when using component', () => {
Expand Down
19 changes: 18 additions & 1 deletion packages/material-ui/src/Divider/Divider.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,23 @@ export const styles = theme => ({
});

const Divider = React.forwardRef(function Divider(props, ref) {
const { absolute, classes, className, component: Component, light, variant, ...other } = props;
const {
absolute,
classes,
className,
component: Component,
light,
tabIndex: tabIndexProp,
variant,
...other
} = props;

if (Component === 'li' && !other.role) {
other.role = 'separator';
}
// tabIndex defaults to -1 in the DOM. Using something lower to indicate to
// MenuList to skip it in keyboard focus navigation.
const tabIndex = tabIndexProp !== undefined ? tabIndexProp : -2;

return (
<Component
Expand All @@ -55,6 +67,7 @@ const Divider = React.forwardRef(function Divider(props, ref) {
className,
)}
ref={ref}
tabIndex={tabIndex}
{...other}
/>
);
Expand Down Expand Up @@ -83,6 +96,10 @@ Divider.propTypes = {
* If `true`, the divider will have a lighter color.
*/
light: PropTypes.bool,
/**
* @ignore
*/
tabIndex: PropTypes.oneOfType([PropTypes.number, PropTypes.string]),
/**
* The variant to use.
*/
Expand Down
4 changes: 3 additions & 1 deletion packages/material-ui/src/MenuItem/MenuItem.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ const MenuItem = React.forwardRef(function MenuItem(props, ref) {
} = props;

let tabIndex;
if (!disabled) {
if (disabled) {
tabIndex = -2;
} else {
tabIndex = tabIndexProp !== undefined ? tabIndexProp : -1;
}
return (
Expand Down
60 changes: 47 additions & 13 deletions packages/material-ui/src/MenuList/MenuList.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,48 @@ import List from '../List';
import getScrollbarSize from '../utils/getScrollbarSize';
import { useForkRef } from '../utils/reactHelpers';

function nextItem(list, item, disableListWrap) {
if (item && item.nextElementSibling) {
return item.nextElementSibling;
}
return disableListWrap ? null : list.firstChild;
}
function previousItem(list, item, disableListWrap) {
if (item && item.previousElementSibling) {
return item.previousElementSibling;
}
return disableListWrap ? null : list.lastChild;
}

function moveFocus(list, currentFocus, disableListWrap, traversalFunction) {
let nextFocus = traversalFunction(list, currentFocus, currentFocus ? disableListWrap : false);
while (nextFocus) {
if (nextFocus === currentFocus) {
return;
}
if (
nextFocus.tabIndex === undefined ||
nextFocus.tabIndex === null ||
nextFocus.tabIndex < -1
) {
nextFocus = traversalFunction(list, nextFocus, disableListWrap);
} else {
break;
}
}
if (nextFocus) {
nextFocus.focus();
}
}

function focusNextItem(list, currentFocus, disableListWrap) {
moveFocus(list, currentFocus, disableListWrap, nextItem);
}

function focusPreviousItem(list, currentFocus, disableListWrap) {
moveFocus(list, currentFocus, disableListWrap, previousItem);
}

const MenuList = React.forwardRef(function MenuList(props, ref) {
const { actions, autoFocus, children, className, onKeyDown, disableListWrap, ...other } = props;
const listRef = React.useRef();
Expand Down Expand Up @@ -47,27 +89,19 @@ const MenuList = React.forwardRef(function MenuList(props, ref) {
(key === 'ArrowUp' || key === 'ArrowDown') &&
(!currentFocus || (currentFocus && !list.contains(currentFocus)))
) {
list.firstChild.focus();
focusNextItem(list, null, disableListWrap);
} else if (key === 'ArrowDown') {
event.preventDefault();
if (currentFocus.nextElementSibling) {
currentFocus.nextElementSibling.focus();
} else if (!disableListWrap) {
list.firstChild.focus();
}
focusNextItem(list, currentFocus, disableListWrap);
} else if (key === 'ArrowUp') {
event.preventDefault();
if (currentFocus.previousElementSibling) {
currentFocus.previousElementSibling.focus();
} else if (!disableListWrap) {
list.lastChild.focus();
}
focusPreviousItem(list, currentFocus, disableListWrap);
} else if (key === 'Home') {
event.preventDefault();
list.firstChild.focus();
focusNextItem(list, null, disableListWrap);
} else if (key === 'End') {
event.preventDefault();
list.lastChild.focus();
focusPreviousItem(list, null, disableListWrap);
}

if (onKeyDown) {
Expand Down
28 changes: 20 additions & 8 deletions packages/material-ui/test/integration/Menu.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,8 @@ describe('<Menu> integration', () => {
if (instance && !instance.stubbed) {
if (instance.tabIndex === 0) {
tabIndexTracker[itemIndex] = true;
} else if (instance.tabIndex > 0) {
tabIndexTracker[itemIndex] = instance.tabIndex;
}
const offsetTop = instance.offsetTop;
stub(instance, 'offsetTop').get(() => {
Expand Down Expand Up @@ -205,6 +207,7 @@ describe('<Menu> integration', () => {
variant,
{
selectedIndex,
selectedTabIndex,
invalidIndex,
autoFocusIndex,
disabledIndex,
Expand Down Expand Up @@ -246,6 +249,7 @@ describe('<Menu> integration', () => {
disabled={itemIndex === disabledIndex ? true : undefined}
itemIndex={itemIndex}
selected={itemIndex === selectedIndex}
tabIndex={itemIndex === selectedIndex ? selectedTabIndex : undefined}
autoFocus={itemIndex === autoFocusIndex ? true : undefined}
>
Menu Item {itemIndex}
Expand All @@ -272,62 +276,70 @@ describe('<Menu> integration', () => {
});
});

it('[variant=menu] nothing selected, content anchor: first, focus: MenuList, tabIndex: first', () => {
it('[variant=menu] nothing selected', () => {
assert.deepStrictEqual(contentAnchorTracker, [true, false, false]);
assert.deepStrictEqual(focusTracker, [false, false, false]);
assert.strictEqual(menuListFocusTracker, true);
assert.deepStrictEqual(tabIndexTracker, [false, false, false]);
});

it('[variant=menu] nothing selected, autoFocus on third, content anchor: first, focus: third, tabIndex: third', () => {
it('[variant=menu] nothing selected, autoFocus on third', () => {
mountTrackingMenu('menu', { autoFocusIndex: 2 });
assert.deepStrictEqual(contentAnchorTracker, [true, false, false]);
assert.deepStrictEqual(focusTracker, [false, false, true]);
assert.strictEqual(menuListFocusTracker, true);
assert.deepStrictEqual(tabIndexTracker, [false, false, false]);
});

it('[variant=selectedMenu] nothing selected, content anchor: first, focus: MenuList, tabIndex: first', () => {
it('[variant=selectedMenu] nothing selected', () => {
mountTrackingMenu('selectedMenu');
assert.deepStrictEqual(contentAnchorTracker, [true, false, false]);
assert.deepStrictEqual(focusTracker, [false, false, false]);
assert.strictEqual(menuListFocusTracker, true);
assert.deepStrictEqual(tabIndexTracker, [false, false, false]);
});

it('[variant=selectedMenu] nothing selected, content anchor: first valid, focus: first valid, tabIndex: first valid', () => {
it('[variant=selectedMenu] nothing selected, first index invalid', () => {
mountTrackingMenu('selectedMenu', { invalidIndex: 0 });
assert.deepStrictEqual(contentAnchorTracker, [false, true, false]);
assert.deepStrictEqual(focusTracker, [false, false, false]);
assert.strictEqual(menuListFocusTracker, true);
assert.deepStrictEqual(tabIndexTracker, [false, false, false]);
});

it('[variant=menu] second item selected, content anchor: first, focus: MenuList, tabIndex: first', () => {
it('[variant=menu] second item selected', () => {
mountTrackingMenu('menu', { selectedIndex: 1 });
assert.deepStrictEqual(contentAnchorTracker, [true, false, false]);
assert.deepStrictEqual(focusTracker, [false, false, false]);
assert.strictEqual(menuListFocusTracker, true);
assert.deepStrictEqual(tabIndexTracker, [false, false, false]);
});

it('[variant=selectedMenu] second item selected, content anchor: second, focus: second, tabIndex: second', () => {
it('[variant=selectedMenu] second item selected, explicit tabIndex', () => {
mountTrackingMenu('selectedMenu', { selectedIndex: 1, selectedTabIndex: 2 });
assert.deepStrictEqual(contentAnchorTracker, [false, true, false]);
assert.deepStrictEqual(focusTracker, [false, true, false]);
assert.strictEqual(menuListFocusTracker, true);
assert.deepStrictEqual(tabIndexTracker, [false, 2, false]);
});

it('[variant=selectedMenu] second item selected', () => {
mountTrackingMenu('selectedMenu', { selectedIndex: 1 });
assert.deepStrictEqual(contentAnchorTracker, [false, true, false]);
assert.deepStrictEqual(focusTracker, [false, true, false]);
assert.strictEqual(menuListFocusTracker, true);
assert.deepStrictEqual(tabIndexTracker, [false, true, false]);
});

it('[variant=selectedMenu] second item selected and disabled, content anchor: second, focus: second, tabIndex: second', () => {
it('[variant=selectedMenu] second item selected and disabled', () => {
mountTrackingMenu('selectedMenu', { selectedIndex: 1, disabledIndex: 1 });
assert.deepStrictEqual(contentAnchorTracker, [true, false, false]);
assert.deepStrictEqual(focusTracker, [false, false, false]);
assert.strictEqual(menuListFocusTracker, true);
assert.deepStrictEqual(tabIndexTracker, [false, false, false]);
});

it('[variant=selectedMenu] second item selected, no autoFocus, content anchor: second, focus: none, tabIndex: second', () => {
it('[variant=selectedMenu] second item selected, no autoFocus', () => {
mountTrackingMenu('selectedMenu', { selectedIndex: 1, autoFocus: false });
assert.deepStrictEqual(contentAnchorTracker, [false, true, false]);
assert.deepStrictEqual(focusTracker, [false, false, false]);
Expand Down
Loading

0 comments on commit 7fa46a4

Please sign in to comment.