[UI Framework] Improve KuiContextMenu keyboard navigation UX #14434

cjcenizal · 2017-10-12T00:05:47Z

Resolves #14239
Also addresses #14183 (review)

@timroes I implemented your suggestions for the improved keyboard navigation. @weltenwort I refactored the code a bit and got rid of that isVisible prop. It turns out it wasn't necessary at all --embarrassingly, it didn't even do what I originally thought it was doing.

cjcenizal · 2017-10-12T22:32:02Z

jenkins, test this

timroes · 2017-10-13T11:21:39Z

@cjcenizal you have to merge master into this, to get rid of the licencing issue. I merged a fix for that yesterday.

timroes · 2017-10-13T11:37:38Z

I think this PR destroyed (on purpose?) some of the old behavior.

In panels that are menu only, I cannot use Shift+Tab anymore to move focus to the header (and press enter there to move an layer up). I can now only use Left to move a layer up. But the content dialog in the example (i.e. a non menu dialog?) can still focus that header and use enter.

Is this a desired behavior or got this broken by accident?

timroes · 2017-10-13T12:08:34Z

Due to the overflow: hidden on the .kuiContextMenuthe focus highlight isn't shown in the first example in the docs, but in the third it's shown.

To be honest I don't find that focus ring in this place very attractive and I would like to discuss other solutions.

I would either try to create a more beautiful focus ring for this, though I had a hard time coming up with an idea how this could look like, or get rid of the focus ring for the dialog in general.

Why would I even say such a thing? Here some arguments why I think we can get rid of it in this place:

Having focus rings is WCAG AA - so even they don't consider it the very most important thing to place it in A level compliance
Even though the element is focused, you cannot interact with it via keyboard in this moment. So pressing Enter or Space doesn't do anything, so I think it's not as important to know, that this element currently has the focus.
The popover is seen as kind of a modal over the page. If you click outside it, it vanished, if you press Escape, it vanishes. So as soon as it opens the attention of the user is anyway at it. Also pressing Tab in that moment will directly interact with the dialog, same as the arrow keys.

I just don't see a good reason, that this focus ring would add much for the user in this case. That she would expect more likely that pressing keys will now interact with it? Wouldn't most users assume that anyway for something behaving very modal like? I just feel we lose too much from a design perspective for gaining little to nothing in usability.

cjcenizal · 2017-10-13T16:00:05Z

@timroes Thanks for those terrific catches! I completely agree that the focus ring provides no benefit and sends the wrong message to the user (I didn't notice it being applied originally). I've removed it as you suggested. I also fixed that bug with the tabbing UX -- that was a regression. Could you take another look?

weltenwort

The UX appears fine to me. I have some reservations about the very frugal use of refs which are passed across component boundaries, which I think we could reduce. But that might better be done in a separate "clean up component interfaces and ref usage" PR.

isVisible won't be missed, but it still seems to be present in the KuiContextMenu tests. 😉

weltenwort · 2017-10-13T17:09:09Z

ui_framework/src/components/context_menu/context_menu_panel.js

+    if (node) {
+      this.content = node;
+    }
+  };


If we store a reference to the dom element here, we should also clear it when we're called with null. Just storing this.content = node would do that.

cjcenizal · 2017-10-13T17:52:07Z

@weltenwort Bah thanks for catching that mistake in the tests. I've addressed both of your points. Mind taking one last quick look?

weltenwort

Nice, thank you 😄

I just noticed a small thing with keyboard focus in the dashboard: After maximizing a panel via the context menu, I can not focus any other element in Kibana without first clicking somewhere. Maybe the focus trap was not reset properly?

weltenwort · 2017-10-13T18:18:33Z

ui_framework/src/components/context_menu/context_menu_panel.js

-      this.panel = node;
+    this.panel = node;
+
+    if (this.panel) {
      this.panel.addEventListener('animationend', this.onTransitionComplete);


Now if we just could remove the event listener again when the previous this.panel was an element but the node isn't... or even better replace it with the onAnimationEnd react prop 💭

cjcenizal · 2017-10-13T20:49:27Z

@weltenwort Done and done! You are my new favorite code reviewer.

timroes · 2017-10-16T14:56:32Z

Okay now I unfortunately must start nitpicking a bit 😅

When pressing Tab you can cycle through all options, and it will start from the top (i.e. the dialog itself) again when reaching the end. The same behavior doesn't work when using Shift+Tab to cycle backwards. You will be stuck on the dialog in this case.

If this is an actual issue in one of the libraries, I wouldn't consider it blocking or too worse, just crossed my mind.

timroes

If you could fix the small findings, it would be great. Otherwise this looks good to me.

timroes · 2017-10-16T15:02:22Z

src/core_plugins/kibana/public/dashboard/panel/panel_options_menu.js

+
+  onDeletePanel = () => {
+    this.closePopover();
+    this.props.onDeletePanel();


Do you think we should check whether the onDeletePanel prop exists, since it's not marked as required in the prop types, even though as far as I understand, this case can in our current setup never occur?

timroes · 2017-10-16T15:06:35Z

ui_framework/doc_site/src/views/context_menu/context_menu.js

      }],
    };

    this.panels = flattenPanelTree(panelTree);
  }

-  onButtonClick() {
+  onButtonClick = () => {
    this.setState({
      isPopoverOpen: !this.state.isPopoverOpen,


Not really touched in the PR, but since I currently see this in the review, might you change this to use an updater function?

timroes · 2017-10-16T15:06:49Z

ui_framework/doc_site/src/views/context_menu/single_panel.js

@@ -18,21 +18,21 @@ export default class extends Component {
    };
  }

-  onButtonClick() {
+  onButtonClick = () => {
    this.setState({
      isPopoverOpen: !this.state.isPopoverOpen,


Same here with updater function.

timroes · 2017-10-17T15:41:14Z

As discussed in person: If the trigger button is already focused, and you scroll away, pressing Enter will open the menu, but not scroll back to the menu (in contrast to all other actions with the menu). Very nitpicking, but you might want to look into it :-)

weltenwort

This is such a great component. I didn't notice anything else beyond what has been mentioned by @timroes and my comment below.

weltenwort · 2017-10-23T09:57:09Z

ui_framework/src/components/context_menu/context_menu_panel.js

+    // If there aren't any items then this is probably a form or something.
+    // So let's focus the first tabbable item and expedite input from the user.
+    if (!this.menuItems.length) {
+      const tabbableItems = tabbable(this.content);


Maybe we should guard against this.content being null | undefined?

cjcenizal · 2017-10-23T22:52:08Z

@timroes @weltenwort Comments addressed! Could you take another look?

cjcenizal · 2017-10-24T21:57:39Z

jenkins, test this

…Panel.

…it when you select an option.

… the menu when an item is clicked.

…event handler.

…ferences.

…#14434) * Refactor focus state logic to use the React lifecycle correctly. * Update KuiPopover snapshots. * Remove unnecessary isVisible prop from KuiContextMenu. * Allow user to both tab AND use the arrow keys for navigation. * Reinstate ability to tab and shift-tab to the title of KuiContextMenuPanel. * Release focus from Dashboard panel options KuiContextMenu by closing it when you select an option. * Update KuiContextMenu example to demonstrate best practice of closing the menu when an item is clicked. * Replace native transitionend event handler with onAnimationEnd React event handler.

…#14556) * Refactor focus state logic to use the React lifecycle correctly. * Update KuiPopover snapshots. * Remove unnecessary isVisible prop from KuiContextMenu. * Allow user to both tab AND use the arrow keys for navigation. * Reinstate ability to tab and shift-tab to the title of KuiContextMenuPanel. * Release focus from Dashboard panel options KuiContextMenu by closing it when you select an option. * Update KuiContextMenu example to demonstrate best practice of closing the menu when an item is clicked. * Replace native transitionend event handler with onAnimationEnd React event handler.

cjcenizal added Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. v6.1.0 v7.0.0 labels Oct 12, 2017

cjcenizal requested review from timroes and weltenwort October 12, 2017 00:05

cjcenizal force-pushed the 14239/context-menu-focus branch from bef2de5 to f63d868 Compare October 12, 2017 00:06

cjcenizal force-pushed the 14239/context-menu-focus branch from f63d868 to 2303ea7 Compare October 13, 2017 15:11

weltenwort reviewed Oct 13, 2017

View reviewed changes

timroes approved these changes Oct 16, 2017

View reviewed changes

weltenwort approved these changes Oct 23, 2017

View reviewed changes

cjcenizal force-pushed the 14239/context-menu-focus branch from 1f8862a to 3374111 Compare October 24, 2017 19:00

cjcenizal added 7 commits October 24, 2017 15:59

Refactor focus state logic to use the React lifecycle correctly.

84a086c

Update tests.

b0e4163

Update KuiPopover snapshots.

8a5e3dd

Remove unnecessary isVisible prop from KuiContextMenu.

23bba0b

Set index of incoming panel to 0.

9397fd1

Allow user to both tab AND use the arrow keys for navigation.

bd9743d

Reinstate ability to tab and shift-tab to the title of KuiContextMenu…

251c892

…Panel.

cjcenizal added 10 commits October 24, 2017 15:59

Remove focus ring from KuiContextMenuPanel.

44860a2

Remove isVisible from KuiContextMenu tests.

2fd5077

Update ref handlers to clear the reference.

4241b91

Release focus from Dashboard panel options KuiContextMenu by closing …

59889bf

…it when you select an option.

Update KuiContextMenu example to demonstrate best practice of closing…

c7bdbf5

… the menu when an item is clicked.

Replace native transitionend event handler with onAnimationEnd React …

3dd0284

…event handler.

Fix bug which prevented alt-tab from wrapping.

d03442d

Use setState updater functions and guard against null or undefined re…

eaa865a

…ferences.

Prevent isUsingKeyboardToNavigate state from being set unnecessarily.

bde4aaa

Fix failing functional test.

c427b62

cjcenizal force-pushed the 14239/context-menu-focus branch from 9649eb0 to c427b62 Compare October 24, 2017 22:59

cjcenizal merged commit ec38367 into elastic:master Oct 25, 2017

cjcenizal deleted the 14239/context-menu-focus branch October 25, 2017 00:37

cjcenizal mentioned this pull request Oct 25, 2017

[6.x] [UI Framework] Improve KuiContextMenu keyboard navigation UX (#14434) #14556

Merged

cjcenizal mentioned this pull request Oct 25, 2017

Improve KuiContextMenu keyboard navigation UX elastic/eui#44

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[UI Framework] Improve KuiContextMenu keyboard navigation UX #14434

[UI Framework] Improve KuiContextMenu keyboard navigation UX #14434

cjcenizal commented Oct 12, 2017 •

edited

Loading

cjcenizal commented Oct 12, 2017

timroes commented Oct 13, 2017

timroes commented Oct 13, 2017

timroes commented Oct 13, 2017

cjcenizal commented Oct 13, 2017 •

edited

Loading

weltenwort left a comment

weltenwort Oct 13, 2017

cjcenizal commented Oct 13, 2017

weltenwort left a comment

weltenwort Oct 13, 2017

cjcenizal commented Oct 13, 2017

timroes commented Oct 16, 2017

timroes left a comment

timroes Oct 16, 2017

timroes Oct 16, 2017

timroes Oct 16, 2017

timroes commented Oct 17, 2017

weltenwort left a comment

weltenwort Oct 23, 2017

cjcenizal commented Oct 23, 2017

cjcenizal commented Oct 24, 2017

[UI Framework] Improve KuiContextMenu keyboard navigation UX #14434

[UI Framework] Improve KuiContextMenu keyboard navigation UX #14434

Conversation

cjcenizal commented Oct 12, 2017 • edited Loading

cjcenizal commented Oct 12, 2017

timroes commented Oct 13, 2017

timroes commented Oct 13, 2017

timroes commented Oct 13, 2017

cjcenizal commented Oct 13, 2017 • edited Loading

weltenwort left a comment

Choose a reason for hiding this comment

weltenwort Oct 13, 2017

Choose a reason for hiding this comment

cjcenizal commented Oct 13, 2017

weltenwort left a comment

Choose a reason for hiding this comment

weltenwort Oct 13, 2017

Choose a reason for hiding this comment

cjcenizal commented Oct 13, 2017

timroes commented Oct 16, 2017

timroes left a comment

Choose a reason for hiding this comment

timroes Oct 16, 2017

Choose a reason for hiding this comment

timroes Oct 16, 2017

Choose a reason for hiding this comment

timroes Oct 16, 2017

Choose a reason for hiding this comment

timroes commented Oct 17, 2017

weltenwort left a comment

Choose a reason for hiding this comment

weltenwort Oct 23, 2017

Choose a reason for hiding this comment

cjcenizal commented Oct 23, 2017

cjcenizal commented Oct 24, 2017

cjcenizal commented Oct 12, 2017 •

edited

Loading

cjcenizal commented Oct 13, 2017 •

edited

Loading