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.

Sign up for GitHub

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

Jump to bottom

feat(Menu): improve accessibility around dismissing #2028

Merged
merged 3 commits into from
Jul 16, 2020
Merged

feat(Menu): improve accessibility around dismissing #2028

merged 3 commits into from
Jul 16, 2020

Conversation

brunohkbx
Copy link
Collaborator

@brunohkbx brunohkbx commented Jun 30, 2020 •
edited

following up #2010

Test plan

Android

  1. Check accessibilityLabel of the overlay with TalkBack

iOS

  1. Check whether onAccessibilityEscape dismiss the menu with the escape/scrub gesture

@brunohkbx brunohkbx added the Menu label Jun 30, 2020
@callstack-bot
Copy link

callstack-bot commented Jun 30, 2020 •
edited

Hey @brunohkbx, thank you for your pull request 🤗. The documentation from this branch can be viewed here.

@github-actions
Copy link

The mobile version of example app from this branch is ready! You can see it here

.

thymikee
thymikee reviewed Jun 30, 2020
View reviewed changes
src/components/Menu/Menu.tsx Outdated
<TouchableWithoutFeedback onPress={onDismiss}>
<TouchableWithoutFeedback
onPress={onDismiss}
testID={overlayTestID}
Copy link
Member

Choose a reason for hiding this comment

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

I preferred the one passing testID to the actual view with content, instead of an overlay. Can we bring that back and get rid of overlayTestID ?

Copy link
Member

Choose a reason for hiding this comment

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

Is there a need to pass testID to the menu content? It should already be possible to pass them to menu items which the user interacts with.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's possible to pass to menu items. However, there's no way to trigger dismiss on them.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand what you mean by triggering dismiss on menu items

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@satya164 I added a test to illustrate my point.
In testing library 2 we could use waitForElementToBeRemoved instead of checking whether onDismiss was called or not. Either way, we need testID to fire the event dismiss on the menu.

Copy link
Member

Choose a reason for hiding this comment

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

@brunohkbx after talking with @thymikee, maybe instead of overlayTestID, it'd be better to expose a prop overlayAccessibilityLabel which defaults to "Close menu" that you can use to press on it?

Copy link
Member

@thymikee thymikee Jul 1, 2020 •
edited

Choose a reason for hiding this comment

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

We've concluded that non-visual users don't have an easy way to dismiss the menu at the moment, so the right thing to do would be empowering them to do so. We think that's the best way forward and it would be lovely if you could make it happen as a part of this PR (rewording it a bit).

Once we're done, we leave the library more accessible and testable, which is a win-win!

Please note that this will be slightly harder to develop, because it involves testing with VoiceOver and TalkBack. Hope you're not mad at us :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds great! will do.
Thanks for your efforts on this 😃

Copy link
Member

Choose a reason for hiding this comment

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

In terms of accessibility, I can suggest:

  • iOS: use prop called onAccessbilityEscape where you can pass a function responsible for closing Menu. It can help users with disabilities to close it doing Z shape gesture.
  • Android: I can recommend to add a listener on hardware back button using the BackHandler API

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's done. Menu already has a BackHandler listener, so we are good.

@brunohkbx brunohkbx force-pushed the feat/add-prop-overlayTestID-to-menu branch from eae20b2 to 3ac6c34 Compare July 1, 2020 12:42
@brunohkbx brunohkbx changed the title feat(Menu): add prop overlayTestID feat(Menu): add prop testID Jul 1, 2020
@brunohkbx brunohkbx force-pushed the feat/add-prop-overlayTestID-to-menu branch from 3ac6c34 to 86ea6ad Compare July 1, 2020 12:44
@brunohkbx brunohkbx requested a review from thymikee July 1, 2020 13:07
@brunohkbx brunohkbx force-pushed the feat/add-prop-overlayTestID-to-menu branch 2 times, most recently from 954194d to e1a2af5 Compare July 1, 2020 16:07
@brunohkbx brunohkbx changed the title feat(Menu): add prop testID feat(Menu): improve accessibility Jul 1, 2020
@brunohkbx brunohkbx force-pushed the feat/add-prop-overlayTestID-to-menu branch 2 times, most recently from 849e95e to 0f1f50d Compare July 2, 2020 12:33
thymikee
thymikee approved these changes Jul 2, 2020
View reviewed changes
Copy link
Member

@thymikee thymikee left a comment •
edited

Choose a reason for hiding this comment

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

LGTM, feel free to consult the best prop naming with @satya164 and @Trancever :)

@brunohkbx brunohkbx force-pushed the feat/add-prop-overlayTestID-to-menu branch from 0f1f50d to 7537fab Compare July 2, 2020 13:18
@thymikee thymikee changed the title feat(Menu): improve accessibility feat(Menu): improve accessibility around dismissing Jul 2, 2020
Trancever
Trancever reviewed Jul 3, 2020
View reviewed changes
src/components/Menu/Menu.tsx Outdated
/**
* Accessibility label for the overlay. This is read by the screen reader when the user taps outside the menu.
*/
dismissAccessibilityLabel?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think overlayAccessibilityLabel makes more sense. @satya164 What do you think?

Trancever
Trancever reviewed Jul 3, 2020
View reviewed changes
Copy link
Contributor

@Trancever Trancever left a comment

Choose a reason for hiding this comment

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

Great work @brunohkbx. Thanks!

@Trancever Trancever merged commit 1052ed6 into master Jul 16, 2020
@Trancever Trancever deleted the feat/add-prop-overlayTestID-to-menu branch July 16, 2020 07:24
Trancever pushed a commit that referenced this pull request Jul 16, 2020
@brunohkbx @Trancever
fix: improve accessibility around Menu dismissing (#2028)
a25397b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thymikee thymikee thymikee approved these changes

@Trancever Trancever Trancever approved these changes

@satya164 satya164 Awaiting requested review from satya164

@lukewalczak lukewalczak Awaiting requested review from lukewalczak

Assignees
No one assigned
Labels
Menu
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@brunohkbx @callstack-bot @satya164 @thymikee @Trancever @lukewalczak