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.

Sign up for GitHub

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

fix(overflow-menu): improve overall a11y #1115

Merged
merged 6 commits into from
Sep 24, 2018
Merged

fix(overflow-menu): improve overall a11y #1115

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bdd070e
fix(overflow-menu): improve overall a11y
tw15egan Sep 18, 2018
30a0949
fix(overflow-menu): prevent fewer defaults
tw15egan Sep 20, 2018
0c3ddb2
Merge branch 'master' into overflow-a11y
tw15egan Sep 20, 2018
c7b5770
fix(overflow-menu): update logic
Sep 21, 2018
917081c
fix(overflow-menu): add comment
Sep 24, 2018
4dbaa30
Merge branch 'master' into overflow-a11y
tw15egan Sep 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 19 additions & 12 deletions src/components/overflow-menu/overflow-menu.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,33 +1,40 @@
<div data-overflow-menu tabindex="0" aria-label="Overflow menu description" class="bx--overflow-menu">
<svg class="bx--overflow-menu__icon" width="3" height="15" viewBox="0 0 3 15">
<div data-overflow-menu tabindex="0" aria-label="Overflow" class="bx--overflow-menu" role="button" aria-haspopup="true"
aria-expanded="false">
<svg aria-hidden="true" class="bx--overflow-menu__icon" width="3" height="15" viewBox="0 0 3 15">
<g fill-rule="evenodd">
<circle cx="1.5" cy="1.5" r="1.5" />
<circle cx="1.5" cy="7.5" r="1.5" />
<circle cx="1.5" cy="13.5" r="1.5" />
</g>
</svg>
<ul class="bx--overflow-menu-options" tabindex="-1" data-floating-menu-direction="{{direction}}">
<ul class="bx--overflow-menu-options" tabindex="-1" role="menu" aria-label="Overflow" data-floating-menu-direction="{{direction}}">
{{#each items}}
<li class="bx--overflow-menu-options__option{{#if disabled}} bx--overflow-menu-options__option--disabled{{/if}}{{#if danger}} bx--overflow-menu-options__option--danger{{/if}}">
<button class="bx--overflow-menu-options__btn"{{#if title}} title="{{title}}"{{/if}}{{#if primaryFocus}} data-floating-menu-primary-focus{{/if}}{{#if disabled}} tabindex="-1"{{/if}}>{{label}}</button>
</li>
<li class="bx--overflow-menu-options__option{{#if disabled}} bx--overflow-menu-options__option--disabled{{/if}}{{#if danger}} bx--overflow-menu-options__option--danger{{/if}}"
role="presentation">
<button class="bx--overflow-menu-options__btn" role="menuitem" {{#if title}} title="{{title}}" {{/if}}
{{#if primaryFocus}} data-floating-menu-primary-focus{{/if}}{{#if disabled}} tabindex="-1" {{/if}}>{{label}}</button>
</li>
{{/each}}
</ul>
</div>

<div data-overflow-menu tabindex="0" aria-label="Overflow menu description" class="bx--overflow-menu">
<svg class="bx--overflow-menu__icon" width="3" height="15" viewBox="0 0 3 15">
<div data-overflow-menu tabindex="0" aria-label="Overflow" class="bx--overflow-menu" role="button" aria-haspopup="true"
aria-expanded="false">
<svg aria-hidden="true" class="bx--overflow-menu__icon" width="3" height="15" viewBox="0 0 3 15">
<g fill-rule="evenodd">
<circle cx="1.5" cy="1.5" r="1.5" />
<circle cx="1.5" cy="7.5" r="1.5" />
<circle cx="1.5" cy="13.5" r="1.5" />
</g>
</svg>
<ul class="bx--overflow-menu-options bx--overflow-menu--flip" tabindex="-1" data-floating-menu-direction="{{direction}}">
<ul class="bx--overflow-menu-options bx--overflow-menu--flip" tabindex="-1" data-floating-menu-direction="{{direction}}"
role="menu" aria-label="Overflow">
{{#each items}}
<li class="bx--overflow-menu-options__option{{#if disabled}} bx--overflow-menu-options__option--disabled{{/if}}{{#if danger}} bx--overflow-menu-options__option--danger{{/if}}">
<button class="bx--overflow-menu-options__btn"{{#if title}} title="{{title}}"{{/if}}{{#if primaryFocus}} data-floating-menu-primary-focus{{/if}}{{#if disabled}} tabindex="-1"{{/if}}>{{label}}</button>
</li>
<li class="bx--overflow-menu-options__option{{#if disabled}} bx--overflow-menu-options__option--disabled{{/if}}{{#if danger}} bx--overflow-menu-options__option--danger{{/if}}"
role="presentation">
<button class="bx--overflow-menu-options__btn" role="menuitem" {{#if title}} title="{{title}}" {{/if}}
{{#if primaryFocus}} data-floating-menu-primary-focus{{/if}}{{#if disabled}} tabindex="-1" {{/if}}>{{label}}</button>
</li>
{{/each}}
</ul>
</div>
27 changes: 20 additions & 7 deletions src/components/overflow-menu/overflow-menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ class OverflowMenu extends mixin(createComponent, initComponentBySearch, evented
})
);
this.manage(
on(this.element.ownerDocument, 'keypress', event => {
on(this.element.ownerDocument, 'keydown', event => {
this._handleKeyPress(event);
})
);
Expand All @@ -99,6 +99,12 @@ class OverflowMenu extends mixin(createComponent, initComponentBySearch, evented
* @param {Function} callback Callback called when change in state completes.
*/
changeState(state, detail, callback) {
if (state === 'hidden') {
this.element.setAttribute('aria-expanded', 'false');
} else {
this.element.setAttribute('aria-expanded', 'true');
}

if (!this.optionMenu) {
const optionMenu = this.element.querySelector(this.options.selectorOptionMenu);
if (!optionMenu) {
Expand Down Expand Up @@ -156,17 +162,24 @@ class OverflowMenu extends mixin(createComponent, initComponentBySearch, evented
*/
_handleKeyPress(event) {
const key = event.which;
if (key === 13) {
const { element, optionMenu, options } = this;
const { element, optionMenu, options } = this;
const isOfMenu = optionMenu && optionMenu.element.contains(event.target);

if (key === 27) {
this.changeState('hidden', getLaunchingDetails(event), () => {
if (isOfMenu) {
element.focus();
}
});
}

if (key === 13 || key === 32) {
const isOfSelf = element.contains(event.target);
const isOfMenu = optionMenu && optionMenu.element.contains(event.target);
const shouldBeOpen = isOfSelf && !element.classList.contains(options.classShown);
const state = shouldBeOpen ? 'shown' : 'hidden';

if (isOfSelf) {
if (element.tagName === 'A') {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@tw15egan tw15egan Sep 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me know if this is a better solution

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

event.preventDefault();
}
event.preventDefault();
event.delegateTarget = element; // eslint-disable-line no-param-reassign
}

Expand Down