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.

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 DropdownMenu arrows navigation and add missing aria-label. #1875

Merged
merged 2 commits into from
Jul 14, 2017

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Jul 12, 2017

Fixes the DropdownMenu navigation with arrows.
DOWN and RIGHT arrows were working, UP and LEFT weren't. Not sure why the difference, but this is probably related to the recent introduction of arrows navigation through blocks.
If so, there are probably more places where stopPropagation() will be needed.

Also, adds a missing label on the button that opens the drop down menu: it was completely empty... Uses wp.i18n.__() for consistency with what is already used in the file.

Todo: when one of the menu items is focused, pressing Escape should close the menu but it also makes the whole toolbar disappear. Will keep investigating and open a separate issue for this, probably together with other a11y improvements.

Fixes #1874

@afercia afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jul 12, 2017
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Yeah, appears this is needed because of the behavior of onKeyDown and block navigation in VisualEditorBlock.

onKeyDown={ this.onKeyDown }

We could move onKeyDown handler to the block's immediate wrapper element (i.e. not handling events in toolbar), but since the root element in this component can receive focus, we probably want arrow navigation to apply to the entire thing.

Which is all to say, this is probably a good solution, though problematic that descendants of the VisualEditorBlock component need to be aware of the behavior.

@@ -89,6 +89,7 @@ export default class TableBlock extends wp.element.Component {
<li>
<DropdownMenu
icon="editor-table"
label={ wp.i18n.__( 'Edit Table' ) }
Copy link
Member

Choose a reason for hiding this comment

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

Per #1332 (#1205), we should be explicit about dependencies on the i18n module, i.e. use import { __ } from 'i18n'; instead of assuming global.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep I know, I didn't want to change also all the other ones, because outside of the scope of this PR. But since we're here, I can do that if you agree.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't want to change also all the other ones, because outside of the scope of this PR.

Sure, especially in smaller pull requests like this one, I'm fine with some general refactoring sprinkled in as well. In an ideal world we'd have separate individual pull requests to refactor, but alas the world is not ideal and I find such PRs are few and far between.

But agree with the sentiment generally 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DropdownMenu arrow navigation doesn't work with up/left arrow keys
2 participants