Fix form accessibility 2/4

[mdneyazahmad]

Details

This PR tries to fix the issue with accessibility in the form (web and desktop). The user should be able to navigate with Tab and Shift + Tab to focus the next and prev focusable element and on pressing enter, it should submit the form.

Context:

This is a part of a bigger issue (form accessibility) broken into 4 issues #7523, #7916, #7917, #7918. I am assigned to 3 of these, and not on #7523.

As per the expected behaviour:

1. Tab and Shift + Tab should move the focus to next and previous element.
2. Checkbox should toggle with Space, and it should submit with Enter key.
3. Form can be submitted with Enter key.

I created a PR (#8874) to handle checkbox issue that is common to all the issues.

There are some challenges about the Enter key press to submit the form, as the current implementation attaches a global handler for each form. This submits the form whenever enter key is pressed, and this is not expected. Sometimes user wants to press enter on a button, and this produces some secondary interaction (also submits the form).

We have opened a discussion https://expensify.slack.com/archives/C01GTK53T8Q/p1654195847140899 to find a solution of this issue, and the consensious is to fix the issue with a poc given draft (#9749).

Later in the poc we have find that we still have some challenges to solve. Meanwhile, I proposed a soluton here #9749 (comment) to handle this issue, and this PR is based on that.

Fixed Issues

$ #7916
PROPOSAL: #7916 (comment)

Tests | QA Steps

Tests (Web and desktop)

Test 1 (Workspace general settings)

1. Open app
2. Goto Settings -> Workspace -> General Settings
3. Click on name field
4. Verify that Tab and Shift + Tab moves the focus to next and prev element.

Test 2 (Workspace general settings)

1. Open app
2. Goto Settings -> Workspace -> General Settings
3. Click on name field (now name field is focused)
4. Verify that pressing enter submits the form
5. Repeat 1 - 3
6. Press Tab to focus on currency field
7. Verify that pressing enter submits the form
8. Repeat 1 - 3
9. Press Tab to focus on save button.
10. Verify that pressing enter submits the form

Test 3 (Invite member)

1. Open app
2. Goto Settings -> Workspace -> Invite members
3. Click on invite button
4. Click on the search field
5. Verify that Tab and Shift + Tab moves the focus to next and prev element.

Test 4 (Invite member)

1. Open app
2. Goto Settings -> Workspace -> Invite members
3. Click on invite button
4. Select members
5. Click on search field
6. Press enter
7. Verify that it submits the form

Test 5 (set password web and mweb only)

1. Open app
2. Logout, if already logged in
3. Enter your email
4. Click continue
5. Click on Forgot?
6. Open email and copy set password link address to clipboard
7. Replace the host url of the copied link to localhost, and visit the page
8. Click on password input
9. Verify that Tab and Shift + Tab moves the focus to next and prev element.
10. Verify that when you are focussed on password input, checkbox, or submit button and pressing enter should submit the form.

Tests (native and mweb)

Test 1 (Workspace general settings)

1. Open app
2. Goto Settings -> Workspace -> General Settings
3. Change name or currency
4. Press on save button
5. Verify that the changes are saved

• Verify that no errors appear in the JS console

Test 2 (Workspace invite member)

1. Open app
2. Goto Settings -> Workspace -> Manage member
3. Press on invite button
4. Select members from options list
5. Press on invite button
6. Verify that you are back to manage members page

• Verify that no errors appear in the JS console

Test 3 (Set new password)

1. Open app
2. Log out, if you are logged in
3. Enter email and press on continue button
4. Press on Forgot? It should send a link to reset the password on your registered email.
5. Open email and open the link in the app
6. Enter your new password
7. Press on set password
8. Verify that the password is saved and you are redirected to home page.

• Verify that no errors appear in the JS console

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • iOS / native
  • Android / native
  • iOS / Safari
  • Android / Chrome
  • MacOS / Chrome
  • MacOS / Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product was added in all src/languages/* files
  • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• If a new component is created I verified that:
  • A similar component doesn't exist in the codebase
  • All props are defined accurately and each prop has a /** comment above it */
  • The file is named correctly
  • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • The only data being stored in the state is data necessary for rendering and nothing else
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • All JSX used for rendering exists in the render method
  • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

• I have verified the author checklist is complete (all boxes are checked off).
• I verified the correct issue is linked in the ### Fixed Issues section above
• I verified testing steps are clear and they cover the changes made in this PR
  • I verified the steps for local testing are in the Tests section
  • I verified the steps for Staging and/or Production testing are in the QA steps section
  • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
• I checked that screenshots or videos are included for tests on all platforms
• I included screenshots or videos for tests on all platforms
• I verified tests pass on all platforms & I tested again on:
  • iOS / native
  • Android / native
  • iOS / Safari
  • Android / Chrome
  • MacOS / Chrome
  • MacOS / Desktop
• If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
• I verified proper code patterns were followed (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product was added in all src/languages/* files
  • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I verified that this PR follows the guidelines as stated in the Review Guidelines
• I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• If a new component is created I verified that:
  • A similar component doesn't exist in the codebase
  • All props are defined accurately and each prop has a /** comment above it */
  • The file is named correctly
  • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • The only data being stored in the state is data necessary for rendering and nothing else
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • All JSX used for rendering exists in the render method
  • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots

Web

web-invite.mp4

web-setpassword.mp4

web-settings.mp4

Mobile Web - Chrome

mweb-chrome-invite.mov

mweb-chrome-setpassword.mov

mweb-chrome-settings.mov

Mobile Web - Safari

mweb-safari-invite.mov

mweb-safari-setpassword.mov

mweb-safari-settings.mov

Desktop

desktop-invite.mp4

desktop-settings.mp4

iOS

ios-invite.mov

ios-settings.mov

Android

android-invite.mov

android-settings.mov

[melvin-bot]

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

[michaelhaxhiu]

I see this is still coming along, and curious to ask for a potential ETA on completion. Like can we say this week for example (before Friday)?

[parasharrajat]

I have plans to continue the review today.

[mdneyazahmad]

@parasharrajat any update?

[parasharrajat]

Let's wrap this up. Sorry for the delay.

[parasharrajat]

Please merge main as well.

[luacmartins]

@parasharrajat given my comment here and the form guidelines, I think we are safe to continue without addressing #12370 (comment). Any thoughts?

[luacmartins]

Bump!

[parasharrajat]

I will check it tomorrow IST busy with other issues today.

[luacmartins]

Bump! Let's get this one through the finish line!

[parasharrajat]

Looking into it in a few hours.

[luacmartins]

@mdneyazahmad can you please merge main and resolve the conflicts?

[mdneyazahmad]

Main merged, conflicts resolved!

[luacmartins]

Thanks @mdneyazahmad!

[luacmartins]

Looks good, just a minor console error!

[luacmartins]

@mdneyazahmad let a small comment above. Let me know once you addressed that issue!

[luacmartins]

@mdneyazahmad in addition to my previous comment, there are conflicts now!

[mdneyazahmad]

Updated!

[luacmartins]

LGTM and tests well!

[luacmartins]

Reviewer Checklist

• I have verified the author checklist is complete (all boxes are checked off).
• I verified the correct issue is linked in the ### Fixed Issues section above
• I verified testing steps are clear and they cover the changes made in this PR
  • I verified the steps for local testing are in the Tests section
  • I verified the steps for Staging and/or Production testing are in the QA steps section
  • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
• I checked that screenshots or videos are included for tests on all platforms
• I included screenshots or videos for tests on all platforms
• I verified tests pass on all platforms & I tested again on:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop
• If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
• I verified proper code patterns were followed (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I verified that this PR follows the guidelines as stated in the Review Guidelines
• I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• If a new component is created I verified that:
  • A similar component doesn't exist in the codebase
  • All props are defined accurately and each prop has a /** comment above it */
  • The file is named correctly
  • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • The only data being stored in the state is data necessary for rendering and nothing else
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • All JSX used for rendering exists in the render method
  • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

chrome.mov

Mobile Web - Safari

safari.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

[luacmartins]

I'm gonna merge this to get the functionality out. We can address any issues in a follow up if necessary. Thanks for the reviews everyone!

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[parasharrajat]

Thanks for unblocking this @luacmartins.

[github-actions]

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	733.498 ms → 782.700 ms (+49.201 ms, +6.7%)
App start runJsBundle	200.258 ms → 220.125 ms (+19.867 ms, +9.9%)
App start nativeLaunch	19.767 ms → 20.548 ms (+0.782 ms, +4.0%)
App start regularAppStart	0.017 ms → 0.017 ms (+0.001 ms, +3.8%)
Open Search Page TTI	611.752 ms → 611.614 ms (-0.138 ms, ±0.0%)

Show details

Name	Duration
App start TTI	Baseline Mean: 733.498 ms Stdev: 38.528 ms (5.3%) Runs: 633.7445640005171 671.4245900008827 678.4024459999055 682.9775990005583 697.1268600001931 697.7495450004935 701.2398860007524 706.6555080004036 706.7535239998251 707.9574410002679 720.1273170001805 720.167439000681 720.4905559998006 726.5543409995735 733.2983899991959 734.8983369991183 736.587084999308 742.7556570004672 745.173762999475 746.6283960007131 750.1199249997735 756.7764720004052 758.4702700003982 760.1956510003656 760.9952329993248 761.8507349994034 769.7704550009221 774.8690039999783 779.0070440005511 783.9099759999663 787.0339419990778 818.2350510004908 Current Mean: 782.700 ms Stdev: 26.194 ms (3.3%) Runs: 733.1552200000733 743.1831229999661 747.2109260000288 753.2602040003985 756.091251000762 761.3331269994378 761.7638460006565 762.3699450008571 766.3906650003046 772.9618730004877 773.3138440009207 774.7977569997311 775.4540049992502 778.9151910003275 779.6438989993185 781.2378359995782 782.4664350003004 784.7189279999584 785.8366139996797 788.336194999516 788.52902200073 789.7198040001094 789.7529930006713 789.993212999776 799.4354449994862 809.9610109999776 810.0478050000966 813.4480729997158 813.9488500002772 845.5207180008292 850.8922850005329
App start runJsBundle	Baseline Mean: 200.258 ms Stdev: 15.132 ms (7.6%) Runs: 166 180 181 186 186 187 188 189 191 191 192 192 193 195 197 198 200 201 202 203 208 208 209 212 215 216 217 221 223 228 233 Current Mean: 220.125 ms Stdev: 18.621 ms (8.5%) Runs: 190 195 198 199 202 203 205 206 206 209 209 211 211 213 213 216 219 220 221 223 225 228 229 230 231 234 235 238 239 257 259 270
App start nativeLaunch	Baseline Mean: 19.767 ms Stdev: 1.334 ms (6.7%) Runs: 18 18 18 18 18 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 20 20 21 21 21 21 22 22 22 23 Current Mean: 20.548 ms Stdev: 1.411 ms (6.9%) Runs: 18 19 19 19 19 19 19 19 19 20 20 20 20 20 20 20 21 21 21 21 21 21 21 22 22 22 22 23 23 23 23
App start regularAppStart	Baseline Mean: 0.017 ms Stdev: 0.002 ms (10.4%) Runs: 0.013956001028418541 0.01411999948322773 0.014281999319791794 0.014444999396800995 0.014852000400424004 0.014973999932408333 0.015217998996376991 0.015381000936031342 0.015422001481056213 0.015543000772595406 0.0157880000770092 0.0157880000770092 0.01586899906396866 0.01590999960899353 0.01590999960899353 0.016194000840187073 0.016235999763011932 0.016641998663544655 0.016642000526189804 0.016642000526189804 0.016642000526189804 0.016845999285578728 0.01794400066137314 0.018025999888777733 0.018066000193357468 0.018309999257326126 0.018921000882983208 0.019164999946951866 0.019652999937534332 0.019979000091552734 0.020183000713586807 Current Mean: 0.017 ms Stdev: 0.001 ms (6.2%) Runs: 0.015096001327037811 0.01550300046801567 0.01603200100362301 0.016194000840187073 0.016235001385211945 0.016276000067591667 0.01631699874997139 0.01631700061261654 0.01643799990415573 0.016439000144600868 0.01648000068962574 0.01668200083076954 0.016804998740553856 0.016805000603199005 0.016845999285578728 0.01700800098478794 0.0172520000487566 0.017252998426556587 0.01729300059378147 0.0174150001257658 0.017496999353170395 0.017659001052379608 0.017782000824809074 0.018025999888777733 0.01810700073838234 0.018432000651955605 0.0185139998793602 0.018717000260949135 0.018718000501394272 0.019328000023961067 0.019408999010920525
Open Search Page TTI	Baseline Mean: 611.752 ms Stdev: 17.604 ms (2.9%) Runs: 578.0371909998357 583.6399750001729 584.0695799998939 597.6001389995217 597.687826000154 598.7982190009207 599.6643879991025 600.3988850004971 601.1388349998742 602.3514399994165 603.559529999271 603.6791590005159 604.6258140001446 605.4829519987106 606.7546389997005 606.8934330008924 607.5493980012834 611.2875159997493 614.683796999976 616.4624840002507 617.7241220008582 620.0079749990255 622.4278570003808 622.5877289995551 624.3886719997972 626.3465170003474 629.5530599988997 636.862914999947 638.9561769999564 641.9486090000719 659.1343189999461 Current Mean: 611.614 ms Stdev: 17.155 ms (2.8%) Runs: 580.9238690007478 583.1376550011337 584.5841879993677 586.2569180000573 593.4679770004004 594.019817000255 596.9050299990922 597.4798989985138 598.2527679987252 600.1875 600.7940669991076 601.7899179998785 607.0068769995123 610.606404999271 610.9627280011773 613.2451170012355 614.1711430009454 614.7362469993532 618.3815110009164 618.8228360004723 619.246866999194 619.282104998827 620.950154999271 621.7204179987311 623.5884610004723 625.0340170003474 625.24198500067 630.1503500007093 634.3700360003859 635.578979998827 640.7564700003713 650.000204000622

[OSBotify]

🚀 Deployed to staging by https://github.com/luacmartins in version: 1.2.60-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[OSBotify]

🚀 Deployed to production by https://github.com/luacmartins in version: 1.2.60-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅