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

feat: add accessibility features to popover control #792

Merged
merged 3 commits into from
Nov 19, 2019

Conversation

meganaconley
Copy link
Contributor

Description

Add accessibility features to the Popover control prop. This way, any element can be passed as a control, but it will still work as a button.

  • tabIndex: puts normally non-tabbable elements into the normal tab flow
  • aria-haspopup: indicates the button triggers a popup
  • role="button": screenreader will announce element as button
  • onKeyPress handler: if the control is not a button, adds enter and space as a way to trigger the popover.

@CLAassistant
Copy link

CLAassistant commented Nov 14, 2019

CLA assistant check
All committers have signed the CLA.

@netlify
Copy link

netlify bot commented Nov 14, 2019

Deploy preview for fundamental-react ready!

Built with commit 4d53062

https://deploy-preview-792--fundamental-react.netlify.com

@meganaconley meganaconley requested a review from a team November 14, 2019 17:31
Copy link
Member

@prsdthkr prsdthkr left a comment

Choose a reason for hiding this comment

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

Generally the onKeyPress works well with simple Popover controls. It gets a bit tricky with complex controls like ComboboxInput, LocalizationEditor, SearchInput because they have additional input and button within.

src/Popover/Popover.js Outdated Show resolved Hide resolved
src/Shellbar/__snapshots__/Shellbar.test.js.snap Outdated Show resolved Hide resolved
src/SearchInput/__snapshots__/SearchInput.test.js.snap Outdated Show resolved Hide resolved
src/ComboboxInput/__snapshots__/ComboboxInput.test.js.snap Outdated Show resolved Hide resolved
@jbadan
Copy link
Contributor

jbadan commented Nov 15, 2019

Once the Popover is open - how do I navigate inside of it with keyboard? Or is that out of scope for this pr.
I forgot to turn keycastr on but I tabbed to the button dropdown and then was using the arrow keys - not the mouse like it looks. Bad gif sorry!

2019-11-15 13 51 52

@jacobdevera
Copy link
Contributor

@jbadan keyboard navigation for Popover is through tabbing in #793

I would think that a basic Popover should navigate via tabbing since it can contain any content, but it makes sense that certain compositions (Dropdown, ComboboxInput, etc.) would use arrow keys. Probably out of scope for now.

@meganaconley meganaconley requested review from prsdthkr and a team November 18, 2019 21:59
src/Popover/Popover.js Show resolved Hide resolved
src/Popover/Popover.js Outdated Show resolved Hide resolved
Copy link
Member

@prsdthkr prsdthkr left a comment

Choose a reason for hiding this comment

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

✅ QA looks good to me on Chrome with VoiceOver.

@meganaconley meganaconley requested a review from a team November 19, 2019 18:07
@meganaconley meganaconley merged commit 6ac4e11 into master Nov 19, 2019
@meganaconley meganaconley deleted the feat/accessible-popover-control branch November 19, 2019 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants