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.

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

[UI Framework] [K7] Add keyboard navigation to KuiContextMenu. #14108

Merged

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Sep 22, 2017

You can use the up and down keys to navigate through the items in each menu, and the left and right keys to go to the previous and next menus. You can use the escape key to close the enclosing popover.

TODO:

  • Return focus to originating element when you use escape to close the Popover.
  • Trap focus within Popover content.
  • Fix error with Popovers which contain no focusable elements.
  • Apply same focus trap to Modal.

@cjcenizal cjcenizal added Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. Project:Accessibility labels Sep 22, 2017
@cjcenizal cjcenizal requested review from snide and timroes September 22, 2017 02:24
Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Seems to work as advertised. Somehow got it into a state where the focus didn't match the click, but I couldn't replicate so I don't know what caused it. Took a screenshot though in case you can think something up.

TODOs make sense, feel free to merge whenever.

image

Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

This is gold! Thanks a lot for that. Except the two points you mentioned yourself, that should still be done (especially I like the Escape closing), I just found one bug (pressing left in upmost panel), and two suggestions for using state updater functions.

Since I am in vacation next week, I already give this a LGTM as long as those are fixed.

const nextFocusedMenuItemIndex = this.state.focusedMenuItemIndex - 1;

this.setState({
focusedMenuItemIndex: nextFocusedMenuItemIndex < 0 ? this.menuItems.length - 1 : nextFocusedMenuItemIndex,
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are updating the state from a previous state, we should rather use setState with an updater function here, to avoid issues with the async update. (This might even cause the issue @snide were having.)

const nextFocusedMenuItemIndex = this.state.focusedMenuItemIndex + 1;

this.setState({
focusedMenuItemIndex: nextFocusedMenuItemIndex > this.menuItems.length - 1 ? 0 : nextFocusedMenuItemIndex,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here: should rather use updater function

});
}, 250);
}

showPreviousPanel = () => {
const previousPanelId = this.props.idToPreviousPanelIdMap[this.state.currentPanelId];
Copy link
Contributor

Choose a reason for hiding this comment

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

There is actually a bug, that you can press Left in the topmost menu and will receive an empty menu. I assume a check here would be the fix.

@timroes
Copy link
Contributor

timroes commented Sep 22, 2017

Btw I've noticed something else, which may or may not be a bug in this component. On the ui framework docs page of this component the search in the menu doesn't work. It will immediately take the focus away when clicking into the search. It doesn't seem to happen on other component pages (though I haven't tested all of them).

@cjcenizal cjcenizal force-pushed the k7/accessible-context-menu branch from 180d73e to 24f0fa7 Compare September 22, 2017 21:24
…ver and return focus to originating element on close.
@cjcenizal
Copy link
Contributor Author

Thanks guys! I addressed the bugs and errors you found.

@cjcenizal cjcenizal merged commit 92e8197 into elastic:k7-ui-framework Sep 24, 2017
@cjcenizal cjcenizal deleted the k7/accessible-context-menu branch September 24, 2017 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Project:Accessibility Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants