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

[Hold for payment 2021-06-16] Accessibility improvement: Screen Reader focus does not highlight TOS/Privacy Policy links #3100

Closed
arielgreen opened this issue May 24, 2021 · 12 comments · Fixed by #3311 or #3502
Assignees
Labels
External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@arielgreen
Copy link
Contributor

arielgreen commented May 24, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Expected Result:

On sign up/sign in page, Screen Reader focus should move to "Terms of service and Privacy Policy" links.

Actual Result:

On sign up/sign in page, Screen Reader focus does not move to "Terms of service and Privacy Policy" links.

Action Performed:

  1. Launch the app/webapp
  2. Prerequisite: Turn ON the Screen reader.
    Mobile: Navigate using Swipe gesture(From Left to Right) and verily.
    Web: Navigate the page using arrow keys then verify.

Platform:

Windows/JAWS/Chrome
MACOs/Voiceover/Safari
Android/Talkback
iOS/Voiceover
MacWebapp/Voiceover

Notes/Photos/Videos:
https://user-images.githubusercontent.com/37308300/119418350-d4d15580-bcc5-11eb-89af-952a0a721cff.mp4

View all open jobs on Upwork

Job posting on Upwork: https://www.upwork.com/jobs/~0148dd67804d9a00f0

@arielgreen arielgreen added the External Added to denote the issue can be worked on by a contributor label May 24, 2021
@MelvinBot
Copy link

Triggered auto assignment to @arielgreen (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@MelvinBot MelvinBot added the Daily KSv2 label May 24, 2021
@MelvinBot
Copy link

Triggered auto assignment to @tgolen (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@tugbadogan
Copy link
Contributor

Hey, I'd like to work on this issue. Here's my proposal

I'm planning to resolve this issue by following the tips mentioned in this blog post (1st item is related to the problem we're facing here.)
https://callstack.com/blog/react-native-android-accessibility-tips/

We will modify the following module to add accessibilityRole="link" to Text components and we also need to split the phrase into 2 pieces but putting them into separate View components to make the links accessible separately.
https://github.com/Expensify/Expensify.cash/blob/c602342664a193a671399b7484d8721150895982/src/pages/signin/TermsAndLicenses/TermsWithLicenses.js

We may also need to use AccessibilityInfo.isScreenReaderEnabled to make it possible to access links by pressing Enter key.

@tgolen
Copy link
Contributor

tgolen commented May 25, 2021

@tugbadogan Thank you for your proposal. I think it is a good one, so you have a 🟢 to move forward with this.

I would not change your proposal, but I did have a thought that at some point it would be interesting to see if we could make some kind of accessibility component that makes implementing the screenreader functionality a bit easier. I don't really like having the duplicated openURL() calls.

@arielgreen
Copy link
Contributor Author

@tugbadogan I just sent over an offer via Upwork.

@tugbadogan
Copy link
Contributor

Thanks for assigning this issue to me, I will send the PR soon.

@roryabraham
Copy link
Contributor

@arielgreen PR caused some minor regressions

@tugbadogan
Copy link
Contributor

Hmm, this regression is caused by moving these texts out of nested Text component to make them accessible from Safari and iOS devices. We have two options to solve this regression, which one do you recommend here @roryabraham @tgolen @arielgreen ?

Option 1) Split this long phrase into smaller phrases, so they can fit to single line, so other Text components can be positioned in the same line https://github.com/Expensify/Expensify.cash/blob/3683d39ef4d0f5ab8dc7763ae67dfadfdeb4dff4/src/languages/en.js#L224

        phrase5_1: 'Money transmission is provided`
        phrase5_2: 'by Expensify Payments LLC',
        phrase5_3: '(NMLS ID:2017010) pursuant to its',

Option 2) Put these Texts and TextLinks into a nested Text component as before and add onPress prop to parent Text component, so the licenses link will be accessible in Safari and iOS devices and text wrapping will be handled properly.

https://github.com/Expensify/Expensify.cash/blob/83a06dd485f86ea08f8bd6fb116b5a24b0ce109a/src/pages/signin/TermsAndLicenses/TermsWithLicenses.js#L25-L31

will be replaced as follows

    <Text
        accessibilityRole="link"
        onPress={() =>
            AccessibilityInfo.isScreenReaderEnabled().then(
              enabled =>
                enabled && openURLInNewTab(CONST.LICENSES_URL),
    )}>
        <Text style={[styles.loginTermsText]}>
            {translate('termsOfUse.phrase5')}
        </Text>
        <TextLink style={[styles.loginTermsText]} href={CONST.LICENSES_URL}>
            {translate('termsOfUse.phrase6')}
        </TextLink>
        <Text style={[styles.loginTermsText]}>.</Text>
    </Text>

@roryabraham
Copy link
Contributor

Solution 2 seems much better to me. Are there any downsides to that approach?

@tugbadogan
Copy link
Contributor

Solution 2 seems much better to me. Are there any downsides to that approach?

I realised that links in nested text component are now accessible in Safari. It might be changed by recent version upgrade for react-native. The only downside was duplicated links and codes in text component. But they are not required now. I attached the PR and it is ready for review.

@arielgreen
Copy link
Contributor Author

Reopening, will close again upon payment June 16th.

@arielgreen arielgreen reopened this Jun 11, 2021
@arielgreen arielgreen changed the title Accessibility improvement: Screen Reader focus does not highlight TOS/Privacy Policy links [Hold for payment 2021-06-16] Accessibility improvement: Screen Reader focus does not highlight TOS/Privacy Policy links Jun 11, 2021
@arielgreen arielgreen added Weekly KSv2 and removed Daily KSv2 labels Jun 11, 2021
@arielgreen
Copy link
Contributor Author

Paid via Upwork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants