[HOLD for payment 2023-04-05][$1000] Translate all accessibility features and update tests accordingly

dangrous commented 1 year ago

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

Forgoing the template below for now as this is a broader discussion. See Slack conversation for more context.

We currently only translate some accessibilityLabels and accessibilityHints in the App. For example this, this, and this are not translated.

This is due to us using them to query particular elements in automated testing. For example, this, and this are some of the tests that look for the labels above.

This is not desired behavior for users who rely on these accessibility features to navigate the app, as they are not translated if the user's language is set to something other than English.

In order to fix this, we should update the tests that look for these values to use nativeID or another method of searching for the elements. It looks like the majority of these use either renderedApp.queryByA11yHint, renderedApp.queryByA11yLabel, or renderedApp.queryAllByA11yLabel, but we should make sure we don't miss others that use different functions.

Once this is done, we should update all untranslated accessibilityHints and accessibilityLabels to use our traditional translation structure. This will get us closer to an app that is friendly and usable by all, no matter their background or abilities.

Action Performed:

Break down in numbered steps

Expected Result:

Describe what you think should've happened

Actual Result:

Describe what actually happened

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

Which of our officially supported platforms is this issue occurring on?

[X] Android / native
[X] Android / Chrome
[X] iOS / native
[X] iOS / Safari
[X] MacOS / Chrome / Safari
[X] MacOS / Desktop

Version Number: Reproducible in staging?: Reproducible in production?: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation Expensify/Expensify Issue URL: Issue reported by: @parasharrajat Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1675089156074799

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~010b3151ea23abc3f6
Upwork Job ID: 1640310625121943552
Last Price Increase: 2023-03-27

dangrous commented 1 year ago

Potential solution here: https://expensify.slack.com/archives/C01GTK53T8Q/p1675895442468689?thread_ts=1675089156.074799&cid=C01GTK53T8Q . Will probably take this and run with it this week.

dangrous commented 1 year ago

I have not had time for this. Hopefully soon?

Prince-Mendiratta commented 1 year ago

Hi @dangrous! I believe that this is an issue that can be worked on externally, so posting a proposal for the same, kindly consider. If you think it's doable, i'd love to take this off your hands and work on this issue and have it reviewed by you and a C+.

Proposal

Please re-state the problem that we are trying to solve in this issue.

In many places of the app, the accessibility hint is hard coded directly in components in English. This makes it difficult for any spanish user with a disability to navigate the app easily. The reason behind such a design is because we're using the label texts as reference in jest tests. Since the tests aren't localized, they make use of these accessibility hints/labels to anchor to DOM elements and run the test.

What is the root cause of that problem?

Since the tests just run on the English localized version, we need not hardcode the accessibility values. We can instead have the values localized and then in the tests, use the english values using the translate function.

What changes do you think we should make in order to solve the problem?

Firstly, we will perform the scope analysis about the impact these changes will cause. Currently, there are 2 props used to set the accessibility values - accessibilityHint and accessibilityLabel. From these 2 props, there are a 3 and 10 instances respectively which directly hardcode the values.

The functions that use of these accessibility hints are:

queryByAccessibilityHint
queryByLabelText
queryAllByAccessibilityHint
queryAllByLabelText
getAllByAccessibilityHint

These functions are used in the files - /tests/ui/UnreadIndicatorsTest.js, tests/unit/SidebarFilterTest.js and tests/unit/SidebarOrderTest.js.

With the incorrections pointed out, we can start targeting them. To fix this issue, we will do the following steps:

For all the hardcoded values of accessibilityHint and accessibilityLabel, we will add the respective localized texts.
We will modify all the tests to make use of the translateLocal function from the below mentioned file. https://github.com/Expensify/App/blob/b703eac7951ba946ba63e3fc83e6fe1b5c64f77d/src/libs/Localize/index.js#L67 The changes to the tests will have the values changed in all the above mentioned 5 functions to use the text obtained from translateLocal, instead of hardcoding values.

parasharrajat commented 1 year ago

I will be interested as C+ here.

dangrous commented 1 year ago

Hey! Thanks for being proactive! I'm syncing up internally on next steps here, will report back shortly.

dangrous commented 1 year ago

Okay! Would love your help. I'm going to get a bug-zero person on this, and then we can move forward with all the logistical stuff (assigning, etc.)

MelvinBot commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~010b3151ea23abc3f6

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @0xmiroslav (External)

MelvinBot commented 1 year ago

Current assignee @dangrous is eligible for the External assigner, not assigning anyone new.

MelvinBot commented 1 year ago

📣 @parasharrajat You have been assigned to this job by @dangrous! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

MelvinBot commented 1 year ago

📣 @Prince-Mendiratta You have been assigned to this job by @dangrous! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

dangrous commented 1 year ago

Hm @parasharrajat I think it doesn't know you're the C+ since I manually assigned, but you are! cc @joekaufmanexpensify

@Prince-Mendiratta I think you'll have to manually update the reviewers here due to that, so just remember that when a PR is ready (happy to review before it's ready too)

Prince-Mendiratta commented 1 year ago

PR is ready for review!

Applied in Upwork.

cc @dangrous @parasharrajat @joekaufmanexpensify

dangrous commented 1 year ago

Sigh the robot didn't appropriately post/update this, but it hit production on 3/29, so we should handle payment on 4/5 cc @joekaufmanexpensify

We also need to fill out the checklist:

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[x] [@parasharrajat / @dangrous] The PR that introduced the bug has been identified. Link to the PR: N/A
[x] [@parasharrajat / @dangrous] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N/A
[x] [@parasharrajat / @dangrous] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
[x] [@joekaufmanexpensify] Determine if we should create a regression test for this bug.
[x] [@parasharrajat] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[x] [@joekaufmanexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: N/A

dangrous commented 1 year ago

This was not a regression; just how we initially set up our tests. I don't think we need a discussion, or a new regression test. The existing I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the [translation method](https://github.com/Expensify/App/blob/4bd99402cebdf4d7394e0d1f260879ea238197eb/src/components/withLocalize.js#L60) should cover this in the future. The only thing that MIGHT be good is a more detailed guideline for how to handle accessibility in the app but as of now that's not a huge focus so I think we punt that until later.

Let me know if anyone disagrees, but I'll check off those items for now

Prince-Mendiratta commented 1 year ago

Posting the timeline for this issue here for an overview, calculated using this tool:

Issue timeline analysis: 🐛 Issue created at: Thu, 02 Feb 2023 17:37:10 GMT 🧯 Help Wanted at: Mon, 27 Mar 2023 11:11:39 GMT 🕵🏻 Contributor assigned at: Mon, 27 Mar 2023 11:12:51 GMT 🛸 PR created at: Mon, 27 Mar 2023 14:18:01 GMT 🎯 PR merged at: Tue, 28 Mar 2023 14:07:49 GMT ⌛ Business Days Elapsed between assignment and PR merge: 1

💰 Timeline Bonus/Penalty: 50% Bonus! 🎉

parasharrajat commented 1 year ago

Loved the DIY tool. Do you want to propose this to K2(Internal browser extension tool) on Slack? Might be helpful to the BugZero team.

Prince-Mendiratta commented 1 year ago

@parasharrajat already did!

https://expensify.slack.com/archives/C01GTK53T8Q/p1677225262796459?thread_ts=1677225262.796459&cid=C01GTK53T8Q