Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Menu] Add onFocusIndexChange property #5851

Merged
merged 4 commits into from
Jan 3, 2017
Merged

[Menu] Add onFocusIndexChange property #5851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
69d07ab
[Menu] Add onFocusIndexChange property
gabrieldeal Dec 14, 2016
518ff68
[Menu] Fix spec that was causing Node.js to hang
gabrieldeal Dec 29, 2016
8d63daa
[Menu] Change setFocusIndex argument order
gabrieldeal Jan 3, 2017
b627c27
[Menu] Add spec for hotkeys
gabrieldeal Jan 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
59 changes: 43 additions & 16 deletions src/Menu/Menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,19 @@ class Menu extends Component {
onItemTouchTap: PropTypes.func,
/** @ignore */
onKeyDown: PropTypes.func,
/**
* Callback function fired when the focus on a `MenuItem` is changed.
* There will be some "duplicate" changes reported if two different
* focusing event happen, for example if a `MenuItem` is focused via
* the keyboard and then it is clicked on.
*
* @param {object} event The event that triggered the focus change.
* The event can be null since the focus can be changed for non-event
* reasons such as prop changes.
* @param {number} newFocusIndex The index of the newly focused
* `MenuItem` or `-1` if focus was lost.
*/
onMenuItemFocusChange: PropTypes.func,
/**
* Override the inline-styles of selected menu items.
*/
Expand Down Expand Up @@ -164,8 +177,12 @@ class Menu extends Component {
const filteredChildren = this.getFilteredChildren(props.children);
const selectedIndex = this.getSelectedIndex(props, filteredChildren);

const newFocusIndex = props.disableAutoFocus ? -1 : selectedIndex >= 0 ? selectedIndex : 0;
if (newFocusIndex !== -1 && props.onMenuItemFocusChange) {
props.onMenuItemFocusChange(null, newFocusIndex);
}
this.state = {
focusIndex: props.disableAutoFocus ? -1 : selectedIndex >= 0 ? selectedIndex : 0,
focusIndex: newFocusIndex,
isKeyboardFocused: props.initiallyKeyboardFocused,
keyWidth: props.desktop ? 64 : 56,
};
Expand All @@ -184,8 +201,12 @@ class Menu extends Component {
const filteredChildren = this.getFilteredChildren(nextProps.children);
const selectedIndex = this.getSelectedIndex(nextProps, filteredChildren);

const newFocusIndex = nextProps.disableAutoFocus ? -1 : selectedIndex >= 0 ? selectedIndex : 0;
if (newFocusIndex !== this.state.focusIndex && this.props.onMenuItemFocusChange) {
this.props.onMenuItemFocusChange(null, newFocusIndex);
}
this.setState({
focusIndex: nextProps.disableAutoFocus ? -1 : selectedIndex >= 0 ? selectedIndex : 0,
focusIndex: newFocusIndex,
keyWidth: nextProps.desktop ? 64 : 56,
});
}
Expand All @@ -207,7 +228,7 @@ class Menu extends Component {
return;
}

this.setFocusIndex(-1, false);
this.setFocusIndex(event, -1, false);
};

// Do not use outside of this component, it will be removed once valueLink is deprecated
Expand Down Expand Up @@ -269,13 +290,13 @@ class Menu extends Component {
});
}

decrementKeyboardFocusIndex() {
decrementKeyboardFocusIndex(event) {
let index = this.state.focusIndex;

index--;
if (index < 0) index = 0;

this.setFocusIndex(index, true);
this.setFocusIndex(event, index, true);
}

getMenuItemCount(filteredChildren) {
Expand Down Expand Up @@ -308,35 +329,35 @@ class Menu extends Component {
switch (key) {
case 'down':
event.preventDefault();
this.incrementKeyboardFocusIndex(filteredChildren);
this.incrementKeyboardFocusIndex(event, filteredChildren);
break;
case 'esc':
this.props.onEscKeyDown(event);
break;
case 'tab':
event.preventDefault();
if (event.shiftKey) {
this.decrementKeyboardFocusIndex();
this.decrementKeyboardFocusIndex(event);
} else {
this.incrementKeyboardFocusIndex(filteredChildren);
this.incrementKeyboardFocusIndex(event, filteredChildren);
}
break;
case 'up':
event.preventDefault();
this.decrementKeyboardFocusIndex();
this.decrementKeyboardFocusIndex(event);
break;
default:
if (key && key.length === 1) {
const hotKeys = this.hotKeyHolder.append(key);
if (this.setFocusIndexStartsWith(hotKeys)) {
if (this.setFocusIndexStartsWith(event, hotKeys)) {
event.preventDefault();
}
}
}
this.props.onKeyDown(event);
};

setFocusIndexStartsWith(keys) {
setFocusIndexStartsWith(event, keys) {
let foundIndex = -1;
React.Children.forEach(this.props.children, (child, index) => {
if (foundIndex >= 0) {
Expand All @@ -348,7 +369,7 @@ class Menu extends Component {
}
});
if (foundIndex >= 0) {
this.setFocusIndex(foundIndex, true);
this.setFocusIndex(event, foundIndex, true);
return true;
}
return false;
Expand All @@ -362,7 +383,7 @@ class Menu extends Component {
const itemValue = item.props.value;
const focusIndex = React.isValidElement(children) ? 0 : children.indexOf(item);

this.setFocusIndex(focusIndex, false);
this.setFocusIndex(event, focusIndex, false);

if (multiple) {
const itemIndex = menuValue.indexOf(itemValue);
Expand All @@ -381,14 +402,14 @@ class Menu extends Component {
this.props.onItemTouchTap(event, item, index);
}

incrementKeyboardFocusIndex(filteredChildren) {
incrementKeyboardFocusIndex(event, filteredChildren) {
let index = this.state.focusIndex;
const maxIndex = this.getMenuItemCount(filteredChildren) - 1;

index++;
if (index > maxIndex) index = maxIndex;

this.setFocusIndex(index, true);
this.setFocusIndex(event, index, true);
}

isChildSelected(child, props) {
Expand All @@ -402,7 +423,12 @@ class Menu extends Component {
}
}

setFocusIndex(newIndex, isKeyboardFocused) {
setFocusIndex(event, newIndex, isKeyboardFocused) {
if (this.props.onMenuItemFocusChange) {
// Do this even if `newIndex === this.state.focusIndex` to allow users
// to detect up-arrow on the first MenuItem or down-arrow on the last.
this.props.onMenuItemFocusChange(event, newIndex);
}
this.setState({
focusIndex: newIndex,
isKeyboardFocused: isKeyboardFocused,
Expand Down Expand Up @@ -479,6 +505,7 @@ class Menu extends Component {
multiple, // eslint-disable-line no-unused-vars
onItemTouchTap, // eslint-disable-line no-unused-vars
onEscKeyDown, // eslint-disable-line no-unused-vars
onMenuItemFocusChange, // eslint-disable-line no-unused-vars
selectedMenuItemStyle, // eslint-disable-line no-unused-vars
menuItemStyle, // eslint-disable-line no-unused-vars
style,
Expand Down
129 changes: 128 additions & 1 deletion src/Menu/Menu.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,142 @@
/* eslint-env mocha */
import React from 'react';
import {shallow} from 'enzyme';
import {mount, shallow} from 'enzyme';
import {match, spy} from 'sinon';
import {assert} from 'chai';
import Menu from './Menu';
import MenuItem from '../MenuItem';
import Divider from '../Divider';
import getMuiTheme from '../styles/getMuiTheme';
import keycode from 'keycode';

describe('<Menu />', () => {
const muiTheme = getMuiTheme();
const shallowWithContext = (node) => shallow(node, {context: {muiTheme}});
const mountWithContext = (node) => mount(node, {context: {muiTheme}});
const keycodeEvent = (key) => ({keyCode: keycode(key)});

describe('onMenuItemFocusChange', () => {
function createMenu(props) {
return (
<Menu {...props}>
<MenuItem primaryText="item 1" />
<Divider />
<MenuItem primaryText="item 2" />
<MenuItem primaryText="item 3" />
</Menu>
);
}

it('is invoked when using the arrow key to go down to the bottom and back up to the top', () => {
const onMenuItemFocusChangeSpy = spy();
const menu = createMenu({
disableAutoFocus: false,
onMenuItemFocusChange: onMenuItemFocusChangeSpy,
});
const wrapper = mountWithContext(menu);

assert(onMenuItemFocusChangeSpy.calledWith(null, 0),
'initial focus should invoke callback with 0');
onMenuItemFocusChangeSpy.reset();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using the beforeEach / afterEach pattern to better isolate the tests?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am very interested in making sure we don't break what happens when the user enters a particular sequence of arrow key events. That would be difficult for me to test if I isolated each arrow key event inside its own it since I would lose all the Menu state along with the spy state.

Does that make sense? I'm not confident I interpreted your comment correctly.


wrapper.simulate('keydown', keycodeEvent('down'));
assert(onMenuItemFocusChangeSpy.calledWith(match.object, 1),
'down-arrow invokes callback with index 1');
onMenuItemFocusChangeSpy.reset();

wrapper.simulate('keydown', keycodeEvent('down'));
assert(onMenuItemFocusChangeSpy.calledWith(match.object, 2),
'down-arrow invokes callback with index 2');
onMenuItemFocusChangeSpy.reset();

wrapper.simulate('keydown', keycodeEvent('down'));
assert(onMenuItemFocusChangeSpy.calledWith(match.object, 2),
'down-arrow at end invokes callback with unchanged index');
onMenuItemFocusChangeSpy.reset();

wrapper.simulate('keydown', keycodeEvent('up'));
assert(onMenuItemFocusChangeSpy.calledWith(match.object, 1),
'up-arrow invokes callback with 1');
onMenuItemFocusChangeSpy.reset();

wrapper.simulate('keydown', keycodeEvent('up'));
assert(onMenuItemFocusChangeSpy.calledWith(match.object, 0),
'up-arrow invokes callback with 0');
onMenuItemFocusChangeSpy.reset();

wrapper.simulate('keydown', keycodeEvent('up'));
assert(onMenuItemFocusChangeSpy.calledWith(match.object, 0),
'up-arrow at top invokes callback with unchanged index');
onMenuItemFocusChangeSpy.reset();

wrapper.unmount(); // Otherwise the timer in FocusRipple keeps Node from exiting
});

it('is invoked when props change', () => {
const onMenuItemFocusChangeSpy = spy();
const menu = createMenu({
disableAutoFocus: true,
onMenuItemFocusChange: onMenuItemFocusChangeSpy,
});
const wrapper = mountWithContext(menu);
assert(onMenuItemFocusChangeSpy.notCalled,
'should not be called when creating with disableAutoFocus=true');
onMenuItemFocusChangeSpy.reset();

wrapper.setProps({disableAutoFocus: false});
assert(onMenuItemFocusChangeSpy.calledWith(null, 0),
'changing disableAutoFocus to false invokes callback');
onMenuItemFocusChangeSpy.reset();

wrapper.setProps({disableAutoFocus: true});
assert(onMenuItemFocusChangeSpy.calledWith(null, -1),
'changing disableAutoFocus to true invokes callback');
onMenuItemFocusChangeSpy.reset();

wrapper.unmount(); // Otherwise the timer in FocusRipple keeps Node from exiting
});

it('is invoked for hotkeys', () => {
const onMenuItemFocusChangeSpy = spy();
const props = {
disableAutoFocus: false,
onMenuItemFocusChange: onMenuItemFocusChangeSpy,
};
const menu = (
<Menu {...props}>
<MenuItem primaryText="a00" />
<MenuItem primaryText="b11" />
<Divider />
<MenuItem primaryText="b00" />
</Menu>
);
const wrapper = mountWithContext(menu);

wrapper.simulate('keydown', keycodeEvent('b'));
assert(onMenuItemFocusChangeSpy.calledWith(match.object, 1),
'"b" invokes callback with focus index 1');
onMenuItemFocusChangeSpy.reset();

wrapper.simulate('keydown', keycodeEvent('0'));
// The Divider is incorrectly counted by Menu.setFocusIndexStartsWith().
assert(onMenuItemFocusChangeSpy.calledWith(match.object, 3),
'"b0" invokes callback with focus index 3, which is probably a bug');
onMenuItemFocusChangeSpy.reset();

wrapper.simulate('keydown', keycodeEvent('0'));
assert(onMenuItemFocusChangeSpy.calledWith(match.object, 3),
'"b0" invokes callback with focus index 3');
onMenuItemFocusChangeSpy.reset();

wrapper.simulate('keydown', keycodeEvent('!'));
// It seems like the focus index should be changed to -1 here.
assert(onMenuItemFocusChangeSpy.notCalled,
'"b00!" does not change the focus index, which is probably a bug');
onMenuItemFocusChangeSpy.reset();

wrapper.unmount(); // Otherwise the timer in FocusRipple keeps Node from exiting
});
});

it('should render MenuItem and Divider children', () => {
const wrapper = shallowWithContext(
Expand Down