[UI Framework] Add KuiKeyboardAccessible component to UI Framework. #11743

cjcenizal · 2017-05-11T19:41:16Z

This React component is an analogue to the kbn-accessible-click directive introduced in #11591. You can wrap an element within this component to make it keyboard-accessible per #11513.

CC @Bargs if you're interested.

tsullivan · 2017-05-12T17:37:31Z

ui_framework/components/accessibility/keyboard_accessible.js

+    // If the developer hasn't already specified attributes required for accessibility, add them.
+    const props =  Object.assign({
+      tabIndex: '0',
+      role: 'button',


Shouldn't this role be applied only to elements that "appear as a button" to the user with a screenreader?

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_button_role

If I understand the intention, this container class would also be applied to anchor links in text - that doesn't seem like something we want 100% of the time.

You're absolutely right man. I'll update this PR. I also created #11810 so that we update kbn-accessible-click, too.

To add more to my comment, the MDN docs say this role will make screenreaders announce that the element is a button. Although, it might not be a button visually, for example we will use KuiKeyboardAccessible on an anchor tag when simply using href is not an option (i.e. the anchor tag just has an onClick).

If a screenreader announces something is a button when it is visually not a button, that seems like a negligible issue. Unless the way it is announced is totally confusing. That might be worth looking into. I'm ok with this as it is though.

tsullivan · 2017-05-12T17:47:32Z

ui_framework/components/accessibility/keyboard_accessible.js

+    // Support keyboard accessibility by emulating mouse click on ENTER or SPACE keypress.
+    if (accessibleClickKeys[e.keyCode]) {
+      // Delegate to the click handler on the element.
+      this.props.children.props.onClick(e);


Have you thought about making SPACE emulate click just for button elements? I’m pretty used to having SPACE act as page down unless focus is on a button. If focus is on a link, it should just be an ENTER keypress to emulate a click.

Thanks for catching this, great point.

cjcenizal · 2017-05-16T23:58:12Z

After talking about this with @tsullivan we agreed that it doesn't make sense to use this component on an element which is acting as a link. In that case, you'd just use an anchor tag with an href. It only makes sense to use this component on an element which acts as a button or otherwise responds to the SPACE keypress, like checkboxes and radio buttons. See http://webaim.org/techniques/keyboard/ for more info on these types of elements.

kimjoar

LGTM. Seems to work as expected. Just some minor comments, feel free to skip any of them.

kimjoar · 2017-05-22T08:29:53Z

ui_framework/components/accessibility/keyboard_accessible.js

+    }, child.props, {
+      // We don't want to allow the child to overwrite these handlers.
+      onKeyDown: this.onKeyDown,
+      onKeyUp: this.onKeyUp,


Maybe throw errors in the proptypes stuff below if you override these? (It can be annoying to debug if you add onKeyUp and your function is just never called ;))

(We don't use them often, but there are a couple of keyDown uses in the app, at least)

kimjoar · 2017-05-22T08:32:56Z

ui_framework/components/accessibility/keyboard_accessible.js

+  }
+
+  applyKeyboardAccessibility(child) {
+    // If the developer hasn't already specified attributes required for accessibility, add them.


Maybe "Add attributes required for accessibility unless they are already specified"?

kimjoar · 2017-05-22T08:34:18Z

ui_framework/components/accessibility/keyboard_accessible.js

+
+  applyKeyboardAccessibility(child) {
+    // If the developer hasn't already specified attributes required for accessibility, add them.
+    const props =  Object.assign({


I'd probably write this as:

const props = { tabIndex: '0', role: 'button', ...child.props, onKeyDown: this.onKeyDown, onKeyUp: this.onKeyUp, }

I just think it reads better than Object.assign

kimjoar · 2017-05-22T08:47:58Z

ui_framework/components/accessibility/keyboard_accessible.js

+    }
+
+    // Support keyboard accessibility by emulating mouse click on ENTER or SPACE keypress.
+    if (accessibleClickKeys[e.keyCode]) {


nitpick. I think I'd prefer e.keyCode === ENTER_KEY || e.keyCode === SPACE_KEY, just to be clear. Or maybe make it a function instead isAccessibleClickKey(e.keyCode)? (I just think it reads better)

In either case I think the comment can be removed.

kimjoar · 2017-05-22T08:53:03Z

ui_framework/doc_site/src/views/accessibility/keyboard_accessible.js

+
+    <KuiKeyboardAccessible>
+      <div onClick={() => window.alert('Div clicked')}>
+        Click this div, which itself contains another acessible element:&nbsp;


I think we should specify the expectation/intention here (when you click on the inner KuiKeyboardAccessible it triggers both onClicks).

Good catch... I think this is actually a bug. We only want the child's click handler to be called it's clicked.

Hmm, on second thought, you're right. That is the expected behavior. I need to update the SPACE and ENTER keypress handlers to mirror this behavior.

I'm not sure if it should trigger both or not. I was mostly commenting because it did trigger both when I played around with this and I wasn't sure if it was expected or not, so my comment was just about making intention/expectation clear in the "Click this div" text.

Cool, thanks. I looked into it and I decided that the onClick on the parent element should be responsible for comparing the event's target and currentTarget, instead of making that the responsibility of this component.

…ents. This mirrors mouse click behavior. - Throw errors if KuiKeyboardAccessible child has onKeyDown or onKeyUp props. - Refine syntax and improve comments.

tsullivan · 2017-05-23T16:58:28Z

ui_framework/components/accessibility/keyboard_accessible.js

+      onKeyUp: this.onKeyUp,
+    };
+
+    return cloneElement(child, props);


pretty cool

tsullivan

LGTM 🎆

…lastic#11743) * Move accessibleClickKeys service into UI Framework. * Add KuiKeyboardAccessible component to UI Framework. * Change KuiKeyboardAccessible and kbn-accessible-click to propagate events. This mirrors mouse click behavior.

…11743) (#11974) * Move accessibleClickKeys service into UI Framework. * Add KuiKeyboardAccessible component to UI Framework. * Change KuiKeyboardAccessible and kbn-accessible-click to propagate events. This mirrors mouse click behavior.

…lastic#11743) * Move accessibleClickKeys service into UI Framework. * Add KuiKeyboardAccessible component to UI Framework. * Change KuiKeyboardAccessible and kbn-accessible-click to propagate events. This mirrors mouse click behavior.

cjcenizal added release_note:enhancement v5.5.0 v6.0.0 labels May 11, 2017

cjcenizal requested review from kimjoar and tsullivan May 11, 2017 19:41

cjcenizal force-pushed the feature/keyboard-accessible-component branch from c46aa05 to 7ddac92 Compare May 11, 2017 20:34

tsullivan reviewed May 12, 2017

View reviewed changes

cjcenizal added the Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. label May 15, 2017

epixa added the v6.0.0-alpha2 label May 17, 2017

kimjoar approved these changes May 22, 2017

View reviewed changes

cjcenizal added 4 commits May 22, 2017 08:00

Move accessibleClickKeys service into UI Framework.

fa8bb26

Add KuiKeyboardAccessible component to UI Framework.

d6cb9d6

Tag component with hasReact.

3b05612

Change KuiKeyboardAccessible and kbn-accessible-click to propagate ev…

8e4a06d

…ents. This mirrors mouse click behavior. - Throw errors if KuiKeyboardAccessible child has onKeyDown or onKeyUp props. - Refine syntax and improve comments.

cjcenizal force-pushed the feature/keyboard-accessible-component branch from 7ddac92 to 8e4a06d Compare May 22, 2017 17:23

tsullivan reviewed May 23, 2017

View reviewed changes

kimjoar approved these changes May 23, 2017

View reviewed changes

cjcenizal merged commit 66c6db0 into elastic:master May 23, 2017

cjcenizal deleted the feature/keyboard-accessible-component branch May 23, 2017 17:19

cjcenizal mentioned this pull request May 23, 2017

[5.x] [UI Framework] Add KuiKeyboardAccessible component to UI Framework. #11974

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[UI Framework] Add KuiKeyboardAccessible component to UI Framework. #11743

[UI Framework] Add KuiKeyboardAccessible component to UI Framework. #11743

cjcenizal commented May 11, 2017

tsullivan May 12, 2017

cjcenizal May 15, 2017

tsullivan May 16, 2017

tsullivan May 12, 2017

cjcenizal May 15, 2017

cjcenizal commented May 16, 2017 •

edited

Loading

kimjoar left a comment

kimjoar May 22, 2017

kimjoar May 22, 2017

kimjoar May 22, 2017

kimjoar May 22, 2017

kimjoar May 22, 2017

cjcenizal May 22, 2017

cjcenizal May 22, 2017

kimjoar May 22, 2017

cjcenizal May 22, 2017

tsullivan May 23, 2017

tsullivan left a comment

[UI Framework] Add KuiKeyboardAccessible component to UI Framework. #11743

[UI Framework] Add KuiKeyboardAccessible component to UI Framework. #11743

Conversation

cjcenizal commented May 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented May 16, 2017 • edited Loading

kimjoar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsullivan left a comment

Choose a reason for hiding this comment

cjcenizal commented May 16, 2017 •

edited

Loading