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 keyboard trap in the form token component and improve accessibility #3866

Merged
merged 2 commits into from Dec 8, 2017

Conversation

@afercia
Copy link
Contributor

commented Dec 8, 2017

This PR tries to partially address #1339 fixing a couple issues, specifically:

  • keyboard trap when there's a token added and tabbing backwards with Shift+Tab
  • the aria-expanded attribute should be true only when the suggestions list is rendered

additionally:

  • renders the list <ul> only when there are matches, avoiding to render an empty <ul>
  • removes some unnecessary props

Re: the keyboard trap, what happens right now is that when there is one (or multiple) token added, tabbing backwards from the field:

  • focuses the token "remove" button
  • the onFocus callback triggers a state update
  • componentDidUpdate() kicks in and focus is moved back to the input field
  • every time Shift+Tab get used, this produces a sort of "focus loop" that continuously moves back focus to the input field

To fix this, I'm making isActive: false when the token "remove" button is focused. From a visual perspective, when one of the token "remove" buttons is focused, this removes the focus style from the wrapper but I'm OK with this since the element actually focused is the remove button:

screen shot 2017-12-08 at 11 52 48

If desired, styling improvements can go in a separate issue.

Testing with NVDA, the aria-expanded state is announced correctly:

screenshot 20

Note:
I had hard time to figure out what's going on in this component, mainly because lack of documentation and some naming that could be improved. For example, things like isActive and saveTransform don't tell me exactly what they do.
I guess this component could benefit from some polishing, given also the other issues mentioned on 1339 and still to address.

See #1339

@afercia afercia requested a review from youknowriad Dec 8, 2017

@youknowriad
Copy link
Contributor

left a comment

Really great work here 👍

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Dec 8, 2017

I guess this component could benefit from some polishing, given also the other issues mentioned on 1339 and still to address.

Agreed, this is not the simplest component we have :). It's also hard to get right (several variations depending on props...) which explains a bit why it's so complex.

@afercia

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2017

@youknowriad thanks! I'm seeing a small glitch where the caret is not visible in the input field, especially on Firefox but this happens also on master so I'm going to merge anyways. To reproduce:

  • add 2 tags then Tab away from the input field
  • Shift+Tab to move back to the input field
  • the "real" input field is focused, as expected, but the caret is not visible
  • start typing something (even just a space) and the caret is visible again

Will report also on #1339.

@afercia afercia merged commit a97251e into master Dec 8, 2017

3 checks passed

codecov/project 38.3% (+0.03%) compared to f9fdeae
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@afercia afercia deleted the fix/form-token-keyboard-trap-aria-expanded branch Dec 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.