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 dropdown menu focus loss when using arrow keys with Safari and Voiceover #24186

Merged
merged 5 commits into from
Jul 27, 2020

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Jul 24, 2020

Description

Resolves the issues with dropdown menus detailed on #18537.

The NavigableContainer component had some specific handling that called preventedDefault on key events. This function call seems to prevent Voiceover from initiating its own navigation from text content.

Unfortunately this code was only checking for elements with the role menuitem, while the More menu also contains menuitemradios and could also contain menuitemcheckbox elements too.

This PR ensures the various kinds of menu items are handled.

The second part is that when pressing left or right arrow keys, Voiceover navigates through characters. This would result in a focus loss, since it Voiceover seems to move focus to the next bit of text outside the menu, and on focus loss the menu closes unexpectedly.

Left and right arrow key presses are now also prevented. This is debatable, perhaps left should only be prevented on the first menu item, and right on the last to prevent focus leaving the menu?

How has this been tested?

Added e2e tests

  1. Using Safari and Voiceover, start a new post
  2. Open the more menu
  3. Use up/down arrow keys
  4. Expect each item in the menu receive focus one-by-one and that the menu stays open
  5. Use left/right arrow keys
  6. Expect that nothing happens, the menu stays open

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@talldan talldan added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Regression Related to a regression in the latest release Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Jul 24, 2020
@talldan talldan self-assigned this Jul 24, 2020
@talldan talldan requested a review from afercia July 24, 2020 08:29
@talldan talldan added the Needs Accessibility Feedback Need input from accessibility label Jul 24, 2020
@github-actions
Copy link

github-actions bot commented Jul 24, 2020

Size Change: +28 B (0%)

Total Size: 1.15 MB

Filename Size Change
build/components/index.min.js 200 kB +28 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.min.js 1.14 kB 0 B
build/annotations/index.min.js 3.67 kB 0 B
build/api-fetch/index.min.js 3.43 kB 0 B
build/autop/index.min.js 2.82 kB 0 B
build/blob/index.min.js 620 B 0 B
build/block-directory/index.min.js 7.63 kB 0 B
build/block-directory/style-rtl.css 953 B 0 B
build/block-directory/style.css 952 B 0 B
build/block-editor/index.min.js 125 kB 0 B
build/block-editor/style-rtl.css 10.8 kB 0 B
build/block-editor/style.css 10.8 kB 0 B
build/block-library/editor-rtl.css 7.63 kB 0 B
build/block-library/editor.css 7.63 kB 0 B
build/block-library/index.min.js 132 kB 0 B
build/block-library/style-rtl.css 7.83 kB 0 B
build/block-library/style.css 7.83 kB 0 B
build/block-library/theme-rtl.css 728 B 0 B
build/block-library/theme.css 729 B 0 B
build/block-serialization-default-parser/index.min.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.min.js 3.1 kB 0 B
build/blocks/index.min.js 48.3 kB 0 B
build/components/style-rtl.css 15.7 kB 0 B
build/components/style.css 15.6 kB 0 B
build/compose/index.min.js 9.68 kB 0 B
build/core-data/index.min.js 11.5 kB 0 B
build/data-controls/index.min.js 1.29 kB 0 B
build/data/index.min.js 8.45 kB 0 B
build/date/index.min.js 5.38 kB 0 B
build/deprecated/index.min.js 772 B 0 B
build/dom-ready/index.min.js 568 B 0 B
build/dom/index.min.js 3.23 kB 0 B
build/edit-navigation/index.min.js 10.8 kB 0 B
build/edit-navigation/style-rtl.css 1.08 kB 0 B
build/edit-navigation/style.css 1.08 kB 0 B
build/edit-post/index.min.js 304 kB 0 B
build/edit-post/style-rtl.css 5.61 kB 0 B
build/edit-post/style.css 5.61 kB 0 B
build/edit-site/index.min.js 16.9 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/edit-widgets/index.min.js 9.37 kB 0 B
build/edit-widgets/style-rtl.css 2.45 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/index.min.js 45.3 kB 0 B
build/editor/style-rtl.css 3.78 kB 0 B
build/editor/style.css 3.78 kB 0 B
build/element/index.min.js 4.65 kB 0 B
build/escape-html/index.min.js 733 B 0 B
build/format-library/index.min.js 7.72 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.min.js 2.13 kB 0 B
build/html-entities/index.min.js 621 B 0 B
build/i18n/index.min.js 3.56 kB 0 B
build/is-shallow-equal/index.min.js 711 B 0 B
build/keyboard-shortcuts/index.min.js 2.51 kB 0 B
build/keycodes/index.min.js 1.94 kB 0 B
build/list-reusable-blocks/index.min.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.min.js 5.33 kB 0 B
build/notices/index.min.js 1.79 kB 0 B
build/nux/index.min.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.min.js 2.56 kB 0 B
build/primitives/index.min.js 1.4 kB 0 B
build/priority-queue/index.min.js 789 B 0 B
build/redux-routine/index.min.js 2.85 kB 0 B
build/rich-text/index.min.js 13.9 kB 0 B
build/server-side-render/index.min.js 2.71 kB 0 B
build/shortcode/index.min.js 1.7 kB 0 B
build/token-list/index.min.js 1.27 kB 0 B
build/url/index.min.js 4.06 kB 0 B
build/viewport/index.min.js 1.85 kB 0 B
build/warning/index.min.js 1.14 kB 0 B
build/wordcount/index.min.js 1.17 kB 0 B

compressed-size-action

@enriquesanchez
Copy link
Contributor

Hi @talldan 👋

This is what I see before the PR:

2020-07-24 15-51-00 2020-07-24 15_52_58 2020-07-24 16_02_21YAY

Focus is unpredictable, jumping between words and is lost after a short time.


This is what I see with the fix from this PR:

2020-07-24 15-47-35 2020-07-24 15_48_31 2020-07-24 15_58_54

Focus is predictable and consistent accross all items, and focus was kept inside the dropdown, as expected.

@enriquesanchez enriquesanchez self-requested a review July 24, 2020 21:03
Copy link
Contributor

@enriquesanchez enriquesanchez left a comment

Choose a reason for hiding this comment

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

Can confirm this fixes the focus issues on dropdowns with Safari + VoiceOver

@youknowriad youknowriad merged commit d984bcc into master Jul 27, 2020
@youknowriad youknowriad deleted the fix/dropdown-menu-voiceover-focus-loss branch July 27, 2020 08:01
@github-actions github-actions bot added this to the Gutenberg 8.7 milestone Jul 27, 2020
@youknowriad youknowriad removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jul 27, 2020
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). Needs Accessibility Feedback Need input from accessibility [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants