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: added close drawer accessibility tap area #11184

Merged

Conversation

mikegarfinkle
Copy link
Contributor

@mikegarfinkle mikegarfinkle commented Jan 31, 2023

Motivation

Currently, a user can tap the Overlay next to an open drawer to close the drawer. This is useful in an Accessibility setting when swipe actions are not obvious to the user. Unfortunately, it is not obvious to someone with a visual deficit that this area can be tapped to close the drawer, as this overlay does not have an accessibilityLabel prop exposed.

This PR exposes an accessibilityLabel prop on this overlay component and modifies existing accessibility props to allow for accessibility tap permissions on the overlay component. It then surfaces this prop at the Drawer level as closeAccessibilityLabel.

Test plan

Open up the example app with TalkBack (Android) or VoiceOver (iOS) enabled. Tap on the overlay next to the drawer and observe that it will now read out as "Close drawer".

@github-actions
Copy link

Hey mikegarfinkle! Thanks for opening your first pull request in this repo. If you haven't already, make sure to read our contribution guidelines.

@netlify
Copy link

netlify bot commented Jan 31, 2023

Deploy Preview for react-navigation-example ready!

Name Link
🔨 Latest commit e7cbdf7
🔍 Latest deploy log https://app.netlify.com/sites/react-navigation-example/deploys/63e9336f79ff930008e5e011
😎 Deploy Preview https://deploy-preview-11184--react-navigation-example.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@mikegarfinkle mikegarfinkle changed the title fix: added close accessibility tap area fix: added close drawer accessibility tap area Jan 31, 2023
Copy link
Member

@satya164 satya164 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I have same comment as this PR.

If this is not correct, can you share a video of how a native drawer works with talkback so and other accessibility tech so the drawer implementation can match it?

@mikegarfinkle
Copy link
Contributor Author

Thanks for the PR. I have same comment as this PR.

If this is not correct, can you share a video of how a native drawer works with talkback so and other accessibility tech so the drawer implementation can match it?

Hi! Thanks so much for taking a look at this. To address your question (I've used Gmail as the example app to show default native behaviour):

Android setting:

In Android, there is no Talkback accessibility for the overlay area in a drawer. This is because the Android ever-present back button can dismiss the drawer AND the overlay area is not tappable.

image

However, in react-navigation drawer, the overlay is tappable and currently does not have an accessibility label to tell visually impaired users what tapping will do.

image

As all tappable components should have an accessibility label, I see two ways forward: 1) eliminate tappable overlay (swipe would still work, but this would involve simply pressing on the overlay) in Android, or 2) keep the overlay and add accessibility label as I have done in this PR.

iOS setting:

On iOS, Navigation Drawers are not a native design pattern. However, implementations on a drawer have been created, such as with the Gmail app. In the Gmail implementation, Google has made the overlay as a tappable button (with an accessibility label that is read by iOS VoiceOver) to help visually impaired users dismiss the drawer (as there is no ever-present back button like there is in Android).

image

I believe react-navigation drawer in iOS would benefit from a similar approach: ie. making the overlay tappable to dismiss (which is already done) with an accessibility label (which this PR adds).

@satya164
Copy link
Member

satya164 commented Feb 1, 2023

In Android, there is no Talkback accessibility for the overlay area in a drawer. This is because the Android ever-present back button can dismiss the drawer AND the overlay area is not tappable.

This isn't correct. I use Android as my daily driver and the drawer is always dismissable by tapping on the overlay area. Regarding the back button, there is no such button in recent versions of Android. There's a system back gesture on Android which does the same thing.

Google has made the overlay as a tappable button (with an accessibility label that is read by iOS VoiceOver) to help visually impaired users dismiss the drawer

Makes sense. Then we can do the same. It may also be necessary to accessibilityRole="button". And default the value of the accessibility label to "Close drawer"

packages/drawer/src/types.tsx Outdated Show resolved Hide resolved
@mikegarfinkle mikegarfinkle force-pushed the add-close-accessibility-tap-area branch from 040899a to 36ecb77 Compare February 1, 2023 18:29
@mikegarfinkle
Copy link
Contributor Author

In Android, there is no Talkback accessibility for the overlay area in a drawer. This is because the Android ever-present back button can dismiss the drawer AND the overlay area is not tappable.

This isn't correct. I use Android as my daily driver and the drawer is always dismissable by tapping on the overlay area. Regarding the back button, there is no such button in recent versions of Android. There's a system back gesture on Android which does the same thing.

Ah let me clarify. Google, at least in the Gmail app, hasn't made it accessible. So they have made it tappable, but they didn't add an accessibility label on this tap functionality. This I think we can add though for react navigation!

@mikegarfinkle
Copy link
Contributor Author

Google has made the overlay as a tappable button (with an accessibility label that is read by iOS VoiceOver) to help visually impaired users dismiss the drawer

Makes sense. Then we can do the same. It may also be necessary to accessibilityRole="button". And default the value of the accessibility label to "Close drawer"

Good call on explicitly adding the accessibilityRole, and will add default value too (being cognizant that will always be reading english if this is not set by the dev, regardless of local).

@mikegarfinkle
Copy link
Contributor Author

mikegarfinkle commented Feb 1, 2023

Android setting:

In Android, there is no Talkback accessibility for the overlay area in a drawer. This is because the Android ever-present back button can dismiss the drawer AND the overlay area is not tappable.

I was wrong here (thanks for pointing this out!). The overlay IS tappable and dismisses the drawer in native Android. However, Google has not made this tap functionality accessible. I think this is a bit of an oversight, and we could make this accessible, as I have done in my PR, for react-navigation drawer Android.

@mikegarfinkle mikegarfinkle force-pushed the add-close-accessibility-tap-area branch from 11a9b3e to 00940d9 Compare February 1, 2023 22:05
@mikegarfinkle mikegarfinkle force-pushed the add-close-accessibility-tap-area branch from 00940d9 to c48f2f0 Compare February 1, 2023 22:06
Copy link
Member

@satya164 satya164 left a comment

Choose a reason for hiding this comment

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

Thank you!

@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2023

Codecov Report

Base: 74.09% // Head: 74.09% // No change to project coverage 👍

Coverage data is based on head (e7cbdf7) compared to base (99ff9f7).
Patch coverage: 100.00% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #11184   +/-   ##
=======================================
  Coverage   74.09%   74.09%           
=======================================
  Files         176      176           
  Lines        5598     5598           
  Branches     2193     2193           
=======================================
  Hits         4148     4148           
  Misses       1401     1401           
  Partials       49       49           
Impacted Files Coverage Δ
packages/drawer/src/views/DrawerView.tsx 72.04% <ø> (ø)
packages/drawer/src/views/legacy/Drawer.tsx 58.88% <ø> (-0.23%) ⬇️
packages/drawer/src/views/legacy/Overlay.tsx 92.30% <100.00%> (+0.64%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@satya164 satya164 added this pull request to the merge queue Feb 12, 2023
@satya164 satya164 removed this pull request from the merge queue Feb 12, 2023
@satya164 satya164 added this pull request to the merge queue Feb 12, 2023
@satya164 satya164 removed this pull request from the merge queue due to the queue being cleared Feb 12, 2023
@satya164 satya164 enabled auto-merge (squash) February 12, 2023 18:44
@satya164 satya164 merged commit 061fb13 into react-navigation:main Feb 12, 2023
satya164 pushed a commit that referenced this pull request Feb 12, 2023
**Motivation**

Currently, a user can tap the `Overlay` next to an open drawer to close
the drawer. This is useful in an Accessibility setting when swipe
actions are not obvious to the user. Unfortunately, it is not obvious to
someone with a visual deficit that this area can be tapped to close the
drawer, as this overlay does not have an accessibilityLabel prop
exposed.

This PR exposes an `accessibilityLabel` prop on this overlay component
and modifies existing accessibility props to allow for accessibility tap
permissions on the overlay component. It then surfaces this prop at the
`Drawer` level as `closeAccessibilityLabel`.

**Test plan**

Open up the example app with TalkBack (Android) or VoiceOver (iOS)
enabled. Tap on the overlay next to the drawer and observe that it will
now read out as "Close drawer".
satya164 pushed a commit that referenced this pull request Feb 17, 2023
**Motivation**

Currently, a user can tap the `Overlay` next to an open drawer to close
the drawer. This is useful in an Accessibility setting when swipe
actions are not obvious to the user. Unfortunately, it is not obvious to
someone with a visual deficit that this area can be tapped to close the
drawer, as this overlay does not have an accessibilityLabel prop
exposed.

This PR exposes an `accessibilityLabel` prop on this overlay component
and modifies existing accessibility props to allow for accessibility tap
permissions on the overlay component. It then surfaces this prop at the
`Drawer` level as `closeAccessibilityLabel`.

**Test plan**

Open up the example app with TalkBack (Android) or VoiceOver (iOS)
enabled. Tap on the overlay next to the drawer and observe that it will
now read out as "Close drawer".
satya164 pushed a commit that referenced this pull request Feb 17, 2023
**Motivation**

Currently, a user can tap the `Overlay` next to an open drawer to close
the drawer. This is useful in an Accessibility setting when swipe
actions are not obvious to the user. Unfortunately, it is not obvious to
someone with a visual deficit that this area can be tapped to close the
drawer, as this overlay does not have an accessibilityLabel prop
exposed.

This PR exposes an `accessibilityLabel` prop on this overlay component
and modifies existing accessibility props to allow for accessibility tap
permissions on the overlay component. It then surfaces this prop at the
`Drawer` level as `closeAccessibilityLabel`.

**Test plan**

Open up the example app with TalkBack (Android) or VoiceOver (iOS)
enabled. Tap on the overlay next to the drawer and observe that it will
now read out as "Close drawer".
satya164 pushed a commit that referenced this pull request Mar 12, 2024
**Motivation**

Currently, a user can tap the `Overlay` next to an open drawer to close
the drawer. This is useful in an Accessibility setting when swipe
actions are not obvious to the user. Unfortunately, it is not obvious to
someone with a visual deficit that this area can be tapped to close the
drawer, as this overlay does not have an accessibilityLabel prop
exposed.

This PR exposes an `accessibilityLabel` prop on this overlay component
and modifies existing accessibility props to allow for accessibility tap
permissions on the overlay component. It then surfaces this prop at the
`Drawer` level as `closeAccessibilityLabel`.

**Test plan**

Open up the example app with TalkBack (Android) or VoiceOver (iOS)
enabled. Tap on the overlay next to the drawer and observe that it will
now read out as "Close drawer".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants