Make Discover field chooser items keyboard accessible. #11591

cjcenizal · 2017-05-04T01:36:07Z

Partially addresses #11541

The general idea here is that you should be able to tab to interactive elements and use enter/space to interact with them (i.e. trigger a click handler). Ideally we'd use button elements, but because buttons can't contain other buttons, we have to make the containing div accessible by adding tabindex="0" and a keypress handler. See #11513 for more context.

snide · 2017-05-04T02:13:54Z

👍 Looks good. Seems to work. Think to address that issue completely we'll need to deal with the caret expand bits in the content rows as well.

cjcenizal · 2017-05-04T03:39:10Z

@snide Ah gotcha. BTW I just pushed a commit that also makes the links and +/- icons inside of the field chooser tabbable. I had to swap out the icons for buttons, and I used the MicroButton in the UI Framework, so there's a minor design change.

Before

After

The contrast goes down quite a bit. We could revert this change and create custom buttons that preserve the original appearance. OTOH the link color also contrasts poorly so maybe we should change the background color. Thoughts?

Bargs · 2017-05-04T22:27:42Z

src/core_plugins/kibana/public/discover/components/field_chooser/discover_field.js

+        if ([13, 32].includes(e.keyCode)) {
+          $scope.toggleDetails(field);
+        }
+      };


If we're going to need to do this often, do you think it would be worthwhile to create a custom attribute directive called kbn-buttonPress or something like that, which could handle this keycode checking and also do the preventDefault for the space bar on keyDown? It might make this a bit easier to implement in other components.

Thanks for bringing this up! I think it's a great idea but I'm a little stumped for the best place and name for this module. Suggestions?

src/ui/public/directives?

I suggested kbn-buttonPress because it is kind of similar to the angular event handling directives (ng-click, ng-keyup etc.) which we would be mimicking and a "buttonPress" (pressing space or enter on a button) is what we're trying to emulate.

Sounds good to me. How do you feel about kbn-accessible-click? I think it's good to emphasize that it's for aiding accessibility up-front (and I imagine we'll have other services and directives geared towards accessibility, so they'll be some cohesion here if they all have that word in their names), and I think there's nice symmetry with ng-click since they both have the word "click" in them.

Bargs · 2017-05-04T22:29:04Z

src/core_plugins/kibana/public/discover/components/field_chooser/discover_field.html

+      ng-bind="::field.display ? 'remove' : 'add'"
+      class="discover-sidebar-item-action kuiButton kuiButton--small kuiButton--primary"
+      data-test-subj="fieldToggle-{{::field.name}}"
+    ></button>


For some reason this button is not reacting to the space bar, even though it is a native button.

Which OS and browser are you using? When I tab to it and hit space, it's added to the selected fields list. I'm on Chrome on OS X.

Never mind, I can repro now that I implemented your preventDefault suggestion.

Bargs

Accidentally submitted that last review before adding these additional notes:

The filter buttons are grey now and don't match the ones in the doc table, is that intentional?

Pressing space to toggle the field details causes the page to scroll down, which doesn't happen with a native button. I played with this a bit, and it seems adding a preventDefault on keyDown will stop this from happening.

(probably not obvious, but I'm hitting space in this screenshot)

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

@Bargs Thanks for the great review. I believe I've addressed everything. Can you take another look?

Bargs · 2017-05-05T16:11:29Z

src/ui/public/accessibility/kbn_accessible_click.js

+      const elementType = element.prop('tagName');
+
+      if (elementType === 'BUTTON') {
+        throw new Error(`kbnAccessibleClick doesn't need to be used a button.`);


I think you lost an "on" :)

Bargs · 2017-05-05T16:13:44Z

src/ui/public/accessibility/kbn_accessible_click.js

+        }
+
+        // Prevent a scroll from occurring if the user has hit space.
+        if (accessibleClickKeys[e.keyCode]) {


This will preventDefault on both space and enter, but I'd lean toward only preventing space to lower the chance of any unintended side effects.

Bargs · 2017-05-05T16:14:58Z

src/ui/public/accessibility/kbn_accessible_click.js

+        if (accessibleClickKeys[e.keyCode]) {
+          // If a unique click handler has been specified, then call it.
+          if (attrs.kbnAccessibleClick) {
+            return scope.kbnAccessibleClick();


If we want to keep this, we should pass the event here

Bargs · 2017-05-05T17:20:34Z

src/ui/public/accessibility/kbn_accessible_click.js

+      }
+
+      element.on('keyup', e => {
+        // Support keyboard accessibility by toggling display on ENTER or SPACE keypress.


Comment seems out of date, should be something like

// Support keyboard accessibility by emulating mouse clicks on ENTER or SPACE keypress.

Bargs · 2017-05-05T17:21:33Z

src/ui/public/accessibility/kbn_accessible_click.js

+
+          if (!ngClick) {
+            throw new Error('kbnAccessibleClick requires a callback or ng-click defined on its element.');
+          }


Could you move this out of the event handler and into the main link function so that it fails early if ngClick is not specified?

Bargs · 2017-05-05T17:37:11Z

src/ui/public/accessibility/kbn_accessible_click.js

+            throw new Error('kbnAccessibleClick requires a callback or ng-click defined on its element.');
+          }
+
+          ngClick(scope.$parent);


I hadn't considered using this directive in conjunction with ng-click, it's a great idea! You've got me wondering, do we even want to provide the ability to specify a separate event handler on this directive, or should we always require ng-click? My gut says the latter because it would ensure the interaction is always the same regardless of whether you're using a keyboard or mouse to interact with the element.

I also think we could avoid manually parsing the ng-click attribute if we simply fire a click event and allow ng-click to do its work as usual. This block could be simplified to:

element.on('keyup', e => { if (accessibleClickKeys[e.keyCode]) { element.click(); } });

One other question along this line of thought: you mentioned in #11513 that listening to the keyup event was necessary because "some browsers do not trigger onclick events for such elements when activated via the keyboard." Are there browsers that do trigger click events and if so will having an ng-click and an kbn-accessible-click on the same element cause the event handler to execute twice?

I was afraid of being too prescriptive by requiring an ng-click, but I guess you're right. That's sort of the whole idea, so I'll remove the ability to provide a handler.

Great suggestion on delegating to the ng-click via a click event, I'll implement your suggestion.

I did a little research but I couldn't find any information on which browsers may trigger click events on non-interactive elements. Hopefully we can recruit the help of an expert to help us with this (@jimgoodwin is already looking into this).

cjcenizal · 2017-05-05T20:18:11Z

Thanks @Bargs, this is ready for your 👀 again.

lukasolson

It looks like the default behavior of links is to only open the href on enter, not on space.

This means that, currently, there are a couple of links in the sidebar that aren't activated on space: The "500/500" link, and the "Visualize" link (that appears as a button). Do we want to activate those links on space?

Bargs

Looks like we're in pretty good shape! Left just a couple minor comments.

Bargs · 2017-05-08T16:46:18Z

src/ui/public/accessibility/kbn_accessible_click.js

+  return {
+    restrict: 'A',
+    scope: {
+      kbnAccessibleClick: '&',


I think this scope variable can be removed now.

Bargs · 2017-05-08T16:48:14Z

src/ui/public/accessibility/kbn_accessible_click.js

+      }
+
+      // We're emulating a click action, so we should already have a regular click handler defined.
+      if (!$parse(attrs.ngClick)) {


It seems that $parse(undefined) returns a noop function, so this error never throws. if (!attrs.ngClick) should be sufficient.

Bargs · 2017-05-08T16:49:33Z

src/ui/public/accessibility/kbn_accessible_click.js

+import { uiModules } from 'ui/modules';
+
+uiModules.get('kibana')
+.directive('kbnAccessibleClick', function ($parse) {


$parse will be unused once that usage below is removed.

cjcenizal · 2017-05-08T19:27:54Z

@lukasolson Thanks for pointing out that behavior. Per our research and convo, links shouldn't activate on SPACE keypress, so we should be OK on that front.

@Bargs I've implemented your suggestions and added unit tests.

Could you both please take another look?

Bargs · 2017-05-08T20:35:02Z

src/ui/public/accessibility/__tests__/kbn_accessible_click.js

+    });
+
+    it('when the element is a link with an href', () => {
+      const html = `<a href="#"" kbn-accessible-click></a>`;


Looks like there's an extra " here.

Bargs · 2017-05-08T20:40:29Z

src/ui/public/accessibility/__tests__/kbn_accessible_click.js

+      const html = `<button kbn-accessible-click></button>`;
+      expect(() => {
+        $compile(html)($rootScope);
+      }).to.throwError(`kbnAccessibleClick doesn't need to be used on a button.`);


throwError takes regular expressions, not strings, so these assertions aren't checking the error message currently.

Damn, we have tests elsewhere that pass strings into throwError. Thanks for letting me know.

Bargs · 2017-05-08T21:08:22Z

src/ui/public/accessibility/__tests__/kbn_accessible_click.js

+      const html = `<a ng-click="noop" kbn-accessible-click></a>`;
+      expect(() => {
+        $compile(html)($rootScope);
+      }).not.to.throwError(`kbnAccessibleClick doesn't need to be used on a link if it has a href attribute.`);


If you switch this to a regex it will blow up. It seems that it's not possible to test for not throwing a specific exception, so you should just pass 0 args here.

Bargs · 2017-05-08T21:19:25Z

src/ui/public/accessibility/__tests__/kbn_accessible_click.js

+    const child = angular.element(`<button></button>`);
+    element.append(child);
+    const e = angular.element.Event('keydown'); // eslint-disable-line new-cap
+    e.which = ENTER_KEY;


Why are you setting e.which here when you were setting e.keyCode in the tests above?

Bargs · 2017-05-08T21:33:51Z

src/ui/public/accessibility/__tests__/kbn_accessible_click.js

+    });
+  });
+
+  it(`doesn't call ng-click when the originating element is a child`, () => {


This doesn't seem to be working correctly. This check happens in the keydown event handler. But the click gets fired in the keyup event handler. I set a breakpoint in the keyup event handler and it is still getting fired even if the event originated on a child. I think the only reason we're not noticing this with the "Add" button is that the element immediately gets destroyed.

If you switch line 115 from keydown to keyup and line 116 from e.which to e.keyCode you'll see the test fail.

Bargs · 2017-05-08T21:36:26Z

src/ui/public/accessibility/__tests__/kbn_accessible_click.js

+    child.trigger(e);
+    expect(scope.handleClick.callCount).to.be(0);
+  });
+});


Could you also add one more test for the preventDefault on keyDown for spaces?

cjcenizal · 2017-05-09T02:14:23Z

@Bargs OK man chickity check it. Thanks for the critique.

Bargs

Other than the one small issue I mentioned inline, this LGTM. Should be good to merge after that's fixed.

Really nice improvement, it's great to see the keyboard working in Discover.

Bargs · 2017-05-09T15:21:12Z

src/ui/public/accessibility/__tests__/kbn_accessible_click.js

+      const child = angular.element(`<button></button>`);
+      element.append(child);
+      const e = angular.element.Event('keyup'); // eslint-disable-line new-cap
+      e.keyCode = ENTER_KEY;


I think you mean SPACE_KEY here?

Great catch, as always.

lukasolson

LGTM!

cjcenizal · 2017-05-09T16:25:45Z

jenkins, test this

cjcenizal · 2017-05-09T18:20:27Z

jenkins, test this

…he default scroll action.

* Make Discover field chooser items keyboard accessible. * Make records count link and plus/minus icons tabbable. * Prevent scrolling when you hit spacebar to toggle a field. * Add accessibleClickKeys service and kbnAccessibleClick directive, with tests.

cjcenizal added Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. :Discovery release_note:enhancement v5.5.0 v6.0.0 labels May 4, 2017

cjcenizal requested review from snide, lukasolson and Bargs May 4, 2017 01:36

snide approved these changes May 4, 2017

View reviewed changes

cjcenizal mentioned this pull request May 4, 2017

Make Discover table keyboard accessible. #11604

Merged

2 tasks

Bargs suggested changes May 4, 2017

View reviewed changes

cjcenizal force-pushed the 11541/bug/discover-field-chooser-tabbable branch from 0b7fd50 to 6aa8a88 Compare May 5, 2017 01:15

Bargs suggested changes May 5, 2017

View reviewed changes

lukasolson reviewed May 8, 2017

View reviewed changes

Bargs suggested changes May 8, 2017

View reviewed changes

Bargs approved these changes May 9, 2017

View reviewed changes

lukasolson approved these changes May 9, 2017

View reviewed changes

cjcenizal force-pushed the 11541/bug/discover-field-chooser-tabbable branch from 6866324 to 7ac0a1e Compare May 9, 2017 16:29

cjcenizal added 2 commits May 9, 2017 13:10

Make Discover field chooser items keyboard accessible.

bd2140e

Make records count link and plus/minus icons tabbable.

c5cddc2

cjcenizal added 10 commits May 9, 2017 13:19

Prevent scrolling when you hit spacebar to toggle a field.

021e855

Revert to previous styles for +/- buttons.

7c95237

Add accessibleClickKeys service and kbnAccessibleClick directive.

7d42b74

Fix typos in warnings and comments, and be specific when preventing t…

b556f37

…he default scroll action.

Remove ability to specify a custom click callback.

3f58e09

Remove unused callback and simplify logic.

28c3f70

Add unit test for kbnAccessibleClick directive.

f9061ba

Improve tests and fix incorrect child-detection logic.

6bb8c76

Fix typo in tests.

4751f80

Refine styles for field detail filter buttons.

c7f820b

cjcenizal force-pushed the 11541/bug/discover-field-chooser-tabbable branch from 7ac0a1e to c7f820b Compare May 9, 2017 20:25

cjcenizal merged commit 4ba903e into elastic:master May 9, 2017

cjcenizal deleted the 11541/bug/discover-field-chooser-tabbable branch May 9, 2017 21:28

cjcenizal mentioned this pull request May 9, 2017

[5.x] Make Discover field chooser items keyboard accessible. (#11591) #11683

Merged

This was referenced May 10, 2017

Accessibility: Can't tab through Discover links #11541

Closed

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

Merged

cjcenizal added the Project:Accessibility label May 22, 2017

Make Discover field chooser items keyboard accessible. #11591

Make Discover field chooser items keyboard accessible. #11591

Conversation

cjcenizal commented May 4, 2017 • edited Loading

snide commented May 4, 2017

cjcenizal commented May 4, 2017 • edited Loading

Before

After

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal May 4, 2017 • edited Loading

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

Bargs left a comment

Choose a reason for hiding this comment

cjcenizal commented May 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented May 5, 2017

lukasolson left a comment • edited Loading

Choose a reason for hiding this comment

Bargs 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

cjcenizal commented May 8, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented May 9, 2017

Bargs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasolson left a comment

Choose a reason for hiding this comment

cjcenizal commented May 9, 2017

cjcenizal commented May 9, 2017

cjcenizal commented May 4, 2017 •

edited

Loading

cjcenizal commented May 4, 2017 •

edited

Loading

cjcenizal May 4, 2017 •

edited

Loading

lukasolson left a comment •

edited

Loading