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

Writing flow: Fix focus trap on non-text input types #32714

Merged
merged 2 commits into from Sep 16, 2021

Conversation

@mirka
Copy link
Member

@mirka mirka commented Jun 15, 2021

Fixes #32712

Description

Some existing logic to handle keydown events on input elements does not completely account for the fact that a lot of input types do not support selection ranges, which means we can't always assume they are "text fields".

This PR addresses the most serious problem caused by this naive handling, which is a focus trap on non-text inputs.

The downside to the simple fix in this PR is that up/down arrow keydowns will no longer trigger the native behavior for input types like number. I think this is a reasonable trade-off though, since a keyboard user has alternative ways to modify a number input but no way out of the focus trap. Smarter handling of various input types will require a more complex system, since they each react to arrow keys in different ways.

The rest of the user-facing issues caused by naive input handling seem to be less serious, and merely inconveniences, e.g. #32713 and #32715.

How has this been tested?

  • Unit tests pass.
  • The repro steps in the original issues are fixed (using non-text input types like color and file).

Screenshots

Down arrow navigates through a checkbox

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.
  • I've tested my changes with keyboard and screen readers.
  • 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 (please manually search all *.native.js files for terms that need renaming or removal).
@mirka mirka requested a review from ellatrix as a code owner Jun 15, 2021
@youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Jun 16, 2021

This makes sense to me but I'd appreciate a confirmation from @ellatrix

Loading

@mirka
Copy link
Member Author

@mirka mirka commented Sep 16, 2021

Sorry for the re-ping @ellatrix — would you have any objections to this? I'd really love get this focus trap fixed 🙂

Loading

Copy link
Contributor

@youknowriad youknowriad left a comment

LGTM. Let's get this in. We can always follow-up if anything comes up.

Loading

@youknowriad youknowriad merged commit 760f4d2 into WordPress:trunk Sep 16, 2021
19 checks passed
Loading
@github-actions github-actions bot added this to the Gutenberg 11.6 milestone Sep 16, 2021
@mirka mirka deleted the fix/input-focus-trap branch Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants