fix(overflow-menu): improve overall a11y

[tw15egan]

Closes #666

Adds in recommended a11y additions to Overflow Menu component

Changelog

New

• aria-expanded toggle when it is open / close
• correct a11y roles and and aria-labels

Changed

• Event listener now listens for keydown, instead of keypress, so that esc can be used to close the overflow menu and return focus. The overflow menu can now also be opened with the space bar. Was not able to add in navigation via keyboard arrows, might want to open a separate issue for that.

Testing / Reviewing

• Navigate to Overflow Menu, use enter and space to open / close the menu. Use esc to close the menu
• Ensure aria-expanded toggles correctly
• @carmacleod, it would be awesome if you could test these changes with JAWS, as I do not have a machine that can test that.

[lovemecomputer]

duplicate comment

[lovemecomputer]

good update! i've done a bit of keyboard shortcut work before and would def like to take a look about implementing it here for another PR

[carmacleod]

Wow, great @tw15egan ! I'm swamped today, but I can run it through the screen readers tomorrow afternoon.
Are the changes on the Carbon Overflow Menu page?

[joshblack]

🎉

[lovemecomputer]

i put an Issue for keyboard implementation at #1119

(or let me know if you want me to just make further commits to this PR!)

[tw15egan]

Great! Let's tackle that in another PR.

@carmacleod no rush! I will ping you when the updates are pushed out live to the website

[asudoh]

Thanks @tw15egan for jumping in! Just a question.

[asudoh]

The code here is for preventing link from being followed. Did you find a new condition where event's default behavior should be prevented?

[tw15egan]

I was running into issues when trying to open the menu with the enter key. Perhaps I should I only prevent that if the key === enter?

[tw15egan]

Let me know if this is a better solution

[asudoh]

Thanks @tw15egan for the background and the suggestion! What I saw is that the change from keypress to keydown caused the overflow menu to be open (and thus focus moving to the overflow menu item) in the middle of the keystroke (of enter key). When user finishes the keystroke, the keypress event fires on the overflow menu item given it's a <button> and now has focus.

One way to get around with this is what you suggested. Great to have above finding as comments so that future maintainers/consumers can figure out why, if we go with this approach.

Another approach to address this is below, which directly removes the weird behavior described above, and thus reduces possibility of future issues (esp. wrt browser compatibility).

diff --git a/src/components/overflow-menu/overflow-menu.js b/src/components/overflow-menu/overflow-menu.js
index bdc031f..4a7ffc2 100644
--- a/src/components/overflow-menu/overflow-menu.js
+++ b/src/components/overflow-menu/overflow-menu.js
@@ -82,7 +82,16 @@ class OverflowMenu extends mixin(createComponent, initComponentBySearch, evented
     );
     this.manage(
       on(this.element.ownerDocument, 'keydown', event => {
-        this._handleKeyPress(event);
+        if (event.which === 27) {
+          this._handleKeyPress(event);
+        }
+      })
+    );
+    this.manage(
+      on(this.element.ownerDocument, 'keypress', event => {
+        if (event.which !== 27) {
+          this._handleKeyPress(event);
+        }
       })
     );
     this.manage(
@@ -179,7 +188,7 @@ class OverflowMenu extends mixin(createComponent, initComponentBySearch, evented
       const state = shouldBeOpen ? 'shown' : 'hidden';

       if (isOfSelf) {
-        if (element.tagName === 'A' || key === 13) {
+        if (element.tagName === 'A') {
           event.preventDefault();
         }
         event.delegateTarget = element; // eslint-disable-line no-param-reassign

Don't hesitate to ping me if you have any questions/concerns. Thanks!

[tw15egan]

@asudoh made the changes, but still found the need to prevent default when the space key is pressed (https://github.com/IBM/carbon-components/pull/1115/files#diff-cdb22c353cb80d123893d9f188477d1bR191) to prevent the screen from jumping down

[asudoh]

Thanks alot @tw15egan! Could you add a comment to describing 32 key default prevention is for preventing screen from jumping down? I think this PR is good to go once it happens - Thanks!

[tw15egan]

Added the comment in, going to merge it! 😄 Thanks for helping out

[carbon-bot]

🎉 This PR is included in version 9.20.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀