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

Accessibility Review #58

Open
ericvicenti opened this issue Jan 28, 2017 · 16 comments
Open

Accessibility Review #58

ericvicenti opened this issue Jan 28, 2017 · 16 comments

Comments

@ericvicenti
Copy link
Contributor

Before being prod-ready, we need to make sure a11y features work well on both platforms.

@kirakik
Copy link

kirakik commented Jul 15, 2018

Hey! I'd love to help out with this, I'm super interested in a11y. What does this involve in the context of React Navigation?

@ericvicenti
Copy link
Contributor Author

Hey Kenza, it was great meeting you in Portland!

The biggest thing we need here is to test a11y features comprehensively inside react-navigation apps. There may be unreported bugs. We also have a NavigationPlayground app which we use to test nav code inside this repo. We should make sure it has strong a11y support.

We could also potentially use the new https://github.com/FormidableLabs/eslint-plugin-react-native-a11y within this repo, or at least make sure we follow all of those guidelines.

@evanhackett
Copy link

evanhackett commented Aug 22, 2018

I’m wondering about the status of accessibility in the context of voiceover / talkback. I wrote a blog post several months ago about my concerns here: http://evanhackett.com/2018/react-native-voiceover-bug/.

The short version is I was running into the problem described in this issue: #377 which has to do with voiceover reading aloud labels from previous screens. I ended up fixing the issue by using forks of both RN and react-navigation as described in my blog post and the issue thread.

It seems with the latest version of React Native and the latest version of react-navigation, this issue still remains. Since the issue I linked above (#377) was closed and no one else seems to be talking about it, I’m wondering if other people are still experiencing this issue as well.

If I should open a separate issue about this let me know.

Do read my blog post if you want a somewhat detailed description of the problem, and also please let me know of any corrections you would make regarding what I said. I am a student at Portland State University interning at the Universal Design Lab where accessibility is a top priority. I wrote that post mainly as a way to communicate to my boss why we have an accessibility problem, so I hope everything I said is accurate. It may be outdated since I wrote it several months ago, so if there are other ways of fixing this problem let me know.

Thanks!

@brentvatne
Copy link
Member

brentvatne commented Aug 22, 2018

Hi @evanhackett! Thank you so much for the information. Can you let me know if the following seems like an appropriate fix? Thanks!

function getAccessibilityProps(isActive) {
  if (Platform.OS === 'ios') {
    return {
      accessibilityElementsHidden: !isActive,
      accessible: isActive,
    };
  } else if (Platform.OS === 'android') {
    return {
      importantForAccessibility: isActive ? 'yes' : 'no-hide-descendants',
    };
  } else {
    return null;
  }
}

/**
 * Component that renders the scene as card for the <StackView />.
 */
class Card extends React.Component {
  render() {
    const { children, pointerEvents, style, scene } = this.props;

    return (
      <Animated.View
        pointerEvents={pointerEvents}
        ref={this.props.onComponentRef}
        style={[styles.main, style]}
        {...getAccessibilityProps(scene.isActive)}
      >
        {children}
      </Animated.View>
    );
  }
}

@brentvatne
Copy link
Member

To clarify - this is my attempt to provide a JS-only fix. I don't have an opinion on what should happen upstream with respect to implementing importantForAccessibility on iOS except that from a superficial look at it I think it seems reasonable. We can get the same behavior as the native implementation that you're currently using with the getAccessibilityProps function above, I believe.

@evanhackett
Copy link

Hey @brentvatne. So I just re-read some of the PR's that originally discussed this stuff. Specifically, RN PR 11788. It looks like react native added support for the ios prop accessibilityElementsHidden since I originally wrote that post, which probably obviates the need for a fork of RN that adds importantForAccessibility! It looks like you already knew that though given your getAccessibilityProps function. Given that, your fix seems totally reasonable.

Is the fix you just posted readily applicable to the latest version of react-navigation? I'd like to test it out but I'm not sure where to add the code. In an older version I believe the code you wrote would be applied to src/views/CardStack/Card.js, is that correct? Although I cannot find that file in the new version. Where would I make the change in the latest version of react-navigation? My apologies, I'm not too familiar with the internals of the react-navigation library.

Thank you for all your help!

@brentvatne
Copy link
Member

You can find the appropriate changes for that here: react-navigation/stack@4e04428

I released a new version of react-navigation with this: 2.12.0. Let me know if this solves your issues! Also, if you have an opportunity to investigate any other accessibility concerns related to react-navigation it would be much appreciated. I'm spread fairly thin and so I won't have time to do this personally for a while, but I'm happy to read detailed reports like what you wrote above and help design fixes.

@evanhackett
Copy link

evanhackett commented Aug 23, 2018

Thanks @brentvatne. I am having trouble with my current RN project atm, so I had to test this out using the examples I found here: react-navigation/examples/NavigationPlayground. Voiceover is definitely not working as you would expect/want it to, though in different ways than in my RN project, so I can't say for sure if it is because of how the example app (NavigationPlayground) is setup or how the changes in 2.12.0 effected things (UPDATE: the changes in 2.12.0 definitely affected things, see below for more details).

If you run the example app, push a card on the stack, then turn on voiceover, you will see voiceover reads aloud elements from previous screens, not the current screen.

I am happy to investigate these things further and write more reports. At the moment this is a priority for me at work, so I can afford spending time on getting this library fixed.

I'm wondering if there is a way to write an automated test to check for this behavior? It is so cumbersome to have to manually try things out every time. Can you think of any way to write tests for something like: "when navigating to a new screen, voiceover should not detect elements from the previous screen"?

UPDATE:
I was able to test things out in my app as well now. Things are actually worse than before 😅.

Before:
Voiceover would read elements on the previous page after navigating to a new page. You could manually click on something from the new page and voiceover would focus on that, then voiceover would only read things from the new page.

Now:
I can no longer click on individual items on the current page. And navigating to a new screen still will have voiceover reading elements from the previous page as well.

I think this has to do with an incorrect understanding of accessibilityElementsHidden. My apologies for giving the thumbs up on your proposed fix without fully understanding what it would do! I need to research this and try things out to better understand what we should be doing instead.

@sfcgeorge
Copy link

I was just testing out iOS Voiceover support and noticed the same "reading things underneath the current screen" issue. Can be reproduced in current version of the example navigation app running in Expo — it even reads Expo's home screen through the example app (Profile heading, Options button).

2 specific accessibility consistency issues that should be easy to fix, perhaps for a beginner:

  • Back buttons — if for example you navigate away from a "Home" screen, you'll get a back button with a left chevron labelled "Home" in the top left of the nav bar. It should read as "Home, back button" but it actually just reads as "Home, button".
  • Tabs — When reading a selected tab in the tab bar at the bottom, assuming there are 5 tabs named Home, People, Chat, Settings, natively it should say "Selected, home, tab, one of five" and for an unselected tab it should say "People, tab, two of five". In RN it just says "Home" and "People".

@evanhackett
Copy link

I believe the issue I described above was fixed in this PR. The problem above was from adding this line:

accessible: isActive,

The accessible trait groups all the elements into a single selectable element for voiceover, which is why the entire card was being selected with no ability to select anything on the card. Removing this line (as the PR does) fixes the issue.

As of now it appears stock react-navigation can be used with stock RN to achieve the proper voiceover behavior! Thanks everyone.

@brentvatne
Copy link
Member

thank you for the followup @evanhackett!

@sfcgeorge - thank you for that information, I spun out a couple of issues for those points: #5029 and #5028

@sfcgeorge
Copy link

@brentvatne Thank you. I opened an issue for "Button Shapes" as well.

@kevinvangelder
Copy link

@evanhackett Which react-navigation version was working as expected? I'm working on an app where the Accessibility Inspector selects the entire screen (and lets me drill down from there) when inspecting the Simulator as a macOS app and doesn't let me select anything when inspecting the Simulator as a standalone target, neither of which is the expected behavior. I'd like to test it using the same version that you had working successfully to try to narrow down if there is a problem with my setup or with the version of react-navigation we are using.

@kevinvangelder
Copy link

For anyone finding this and wondering, my particular issue was a hidden popup menu was catching accessibility events, so I just had to make it invisible to accessibility when hidden.

@github-actions
Copy link

github-actions bot commented Feb 7, 2020

Hello 👋, this issue has been open for more than 2 months with no activity on it. If the issue is still present in the latest version, please leave a comment within 7 days to keep it open, otherwise it will be closed automatically. If you found a solution on workaround for the issue, please comment here for others to find. If this issue is critical for you, please consider sending a pull request to fix the issue.

@robergwillian
Copy link

any update on this issue? I'm trying to write automated tests using Appium and cant access elements inside the screens

ShyJuno pushed a commit to ShyJuno/react-navigation that referenced this issue Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants