Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
2.99k stars 2.5k forks source link

[HOLD for payment 2023-03-20] [HOLD #12370][$2000] Audit forms and fix inconsistencies with focus, tab and shift + tab behavior 3/4 #7917

Closed dylanexpensify closed 1 year ago

dylanexpensify commented 2 years ago

We should audit all our forms and fix any inconsistencies with focus, tab, shift + tab and enter behavior. The expected behavior is as follows:

  1. Tab navigates to the next input.
  2. Shift + tab navigates to the previous input.
  3. Enter submits the form.
  4. Space toggles checkboxes/dropdowns.

Note: We should make sure that tabbing cycles through the form in an order that makes sense, usually top to bottom.

Here's a list of forms to be audited:

MelvinBot commented 2 years ago

Triggered auto assignment to @marcochavezf (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

MelvinBot commented 2 years ago

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

dylanexpensify commented 2 years ago

Note: decision to split this main issue up into smaller issues came from this convo

marcochavezf commented 2 years ago

I've been working on an N7 issue these last few days and still today, but I will come back to this issue today at EOD or tomorrow morning.

Christinadobrzyn commented 2 years ago

@dylanexpensify can you clarify, are we paying the hired contributor $250 to update all of the forms in the OP? Also asked here

marcochavezf commented 2 years ago

Oh I hadn't seen the conversation. I think this case I can just unassign myself since the Exported label will be applied to assign reviewers.

Christinadobrzyn commented 2 years ago

Dylan confirmed this GH job is $250. Created Upwork job

Internal job posting - https://www.upwork.com/ab/applicants/1499638878787203072/job-details External job positing - https://www.upwork.com/jobs/~014a2b9d0ae263c283

MelvinBot commented 2 years ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (Exported)

MelvinBot commented 2 years ago

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

mdneyazahmad commented 2 years ago

Proposal

These 4 related issues are very similar. I proposed the solution here https://github.com/Expensify/App/issues/7918#issuecomment-1063298280

thienlnam commented 2 years ago

Still waiting on a volunteer to audit the forms

Christinadobrzyn commented 2 years ago

Price increase to $500

thienlnam commented 2 years ago

Price increased to $1000, still looking for a volunteer

Christinadobrzyn commented 2 years ago

Job price increase to $2,000. Waiting on takers.

mdneyazahmad commented 2 years ago

I have noticed that we have a team of accessibility engineer may be they can help us audit and the contributor can submit proposal for fixing that issues?

OneDevStopShop commented 2 years ago

Hi I am interested in this issue. I am confused about the expectations, do we just need to update the tab functionality if it does not work as expected?

How do you access the KYC flow page?

thienlnam commented 2 years ago

I have noticed that we have a team of accessibility engineer may be they can help us audit and the contributor can submit proposal for fixing that issues?

@mdneyazahmad Good suggestion, though our accessibility QA team is going through a different flow so they wouldn't test this unless a PR happened to touch these forms. We've narrowed the scope of this issue and since it's just checking a few things I think a contributor should be able to do it

@OneDevStopShop If you're looking for an example of what needs to be done, feel free to check this other similar issue out https://github.com/Expensify/App/issues/7523

How do you access the KYC flow page?

Good question, this step comes up when we're setting up your Expensify Wallet. Check out the EnablePaymentsPage and you'll likely have to be on a new account.

OneDevStopShop commented 2 years ago

I looked through the issue you linked and would like to take this issue

melvin-bot[bot] commented 2 years ago

📣 @OneDevStopShop You have been assigned to this job by @thienlnam! 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 📖

Christinadobrzyn commented 2 years ago

Hired @OneDevStopShop for the job and @Santhosh-Sellavel as the C+ in Upwork!

Santhosh-Sellavel commented 2 years ago

@OneDevStopShop If you have any questions please feel to post them here. Thanks!

OneDevStopShop commented 2 years ago

Hi I am having trouble with Upwork and cannot complete jobs at this time. Please find someone new for the job and sorry for the inconvenience

mdneyazahmad commented 2 years ago

I would like to take this one. I am also assigned other form audit issues and working on it. Thank you.

melvin-bot[bot] commented 2 years ago

📣 @mdneyazahmad You have been assigned to this job by @thienlnam! 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 📖

Santhosh-Sellavel commented 2 years ago

@mdneyazahmad Any update?

Christinadobrzyn commented 2 years ago

Wait, sorry @thienlnam I might have missed something.

I hired @OneDevStopShop for this job based on the 'thumbs up' to this post - https://github.com/Expensify/App/issues/7917#issuecomment-1090410859 and this bot comment - https://github.com/Expensify/App/issues/7917#issuecomment-1090577860

Should I also hire @mdneyazahmad?

thienlnam commented 2 years ago

@Christinadobrzyn No you were correct in hiring them. Though from this comment https://github.com/Expensify/App/issues/7917#issuecomment-1093009100 it seems like they're having an issue with Upwork and so I've hired @mdneyazahmad for the job instead. So if possible, revoke the job from @OneDevStopShop and then hire @mdneyazahmad instead

Christinadobrzyn commented 2 years ago

Oh I'm so sorry, not sure how I missed that comment. Thank you for clarifying!

I hired @mdneyazahmad for the job! Sorry about the confusion.

luacmartins commented 2 years ago

@mdneyazahmad please note that 4. Space toggles checkboxes. was added to the list of expected behavior.

Santhosh-Sellavel commented 2 years ago

@mdneyazahmad Kindly provide an update on how is it coming up?

mdneyazahmad commented 2 years ago

@Santhosh-Sellavel sorry for delay. I will work on this one today. Thank you.

Christinadobrzyn commented 2 years ago

I'm going to be ooo until April 26th so I'm going to assign a new CM to this issue. Thanks!

melvin-bot[bot] commented 2 years ago

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

mdneyazahmad commented 2 years ago

This issue depends on https://github.com/Expensify/App/issues/7623 I will have to wait until that issue is fixed.

Santhosh-Sellavel commented 2 years ago

@mdneyazahmad How does it depend on the #7623?

cc: @thienlnam

mdneyazahmad commented 2 years ago

In this issue we have to fix inconsistency with tab and shift + tab which seems to work fine and also the form should submit when enter is pressed.

Current implementation:

Button component accepts enterOnPress as prop and register a global keypress listener for Enter and calls form submit function. This however does not work as expected because enter can be pressed to press a button outside the form https://github.com/Expensify/App/issues/7918#issuecomment-1096631126

This depends on that issue because that tries to solve the same core problem how to handle the Enter key pressed and PR for this issue will depend on the decision taken there.

cc: @Santhosh-Sellavel

Santhosh-Sellavel commented 2 years ago

Thanks, @mdneyazahmad. Let's not wait for it. Perform the audit on each form and list out the issues/inconsistencies. if there are issues with Enter to submit feel free to point out that too.

cc: @thienlnam

mdneyazahmad commented 2 years ago

Form audit

IOU Confirmation Page

  1. tab and shift + tab works as expected.
  2. enter submits the form.
  3. There is no checkbox.

https://user-images.githubusercontent.com/77761491/163789014-37959fa4-795a-41e4-be5c-eda96c64b392.mp4

New Room Screen

  1. tab and shift + tab works as expected.
  2. enter submits the form.
  3. There is no checkbox.

https://user-images.githubusercontent.com/77761491/163789300-450bb9cd-b9a2-4cdc-a39a-8803e9cd2a07.mp4

Add Debit Card Flow

  1. tab and shift + tab works as expected.
  2. enter submits the form.
  3. space does not toggle checkbox.

https://user-images.githubusercontent.com/77761491/163789610-7fe7cde7-eaa6-4408-9937-758217b04a9d.mp4

New Password Form

  1. tab and shift + tab works as expected.
  2. enter submits the form.
  3. space toggles checkbox (eye icon).

https://user-images.githubusercontent.com/77761491/163789733-b9d4b2de-9d49-4a9d-9159-74d0e6b2cdf7.mp4

mdneyazahmad commented 2 years ago

There are some other issues.

New Room Screen

  1. Option select focus styles is applied even when it goes out of focus.

Add Debit Card Flow

  1. Option select (State Field) focus styles is applied even when it goes out of focus.
  2. State Field does not have error text.
  3. It automatically enters some text (possibly first 5 letters of email) into zip code field when page loads.

New Password Form

  1. Submit button is disabled by default.
  2. When we enter invalid password and press enter to submit. It produces error and validates the password on input. It should not validate on input instead it should on submit.
  3. It does not have red outline when it is invalid.
mdneyazahmad commented 2 years ago

@Santhosh-Sellavel Could you help me navigate KYC Flow?

Santhosh-Sellavel commented 2 years ago

@mdneyazahmad

Navigate to /settings/payments/enable-payments which KYCFLow start point

These are steps in the flow, make some code tweaks to test the same.

https://github.com/Expensify/App/blob/6c53be1ea7a022e8a5e788cc7e7efd62c4573474/src/pages/EnablePayments/EnablePaymentsPage.js#L59-L64

@luacmartins can you confirm this.

luacmartins commented 2 years ago

@Santhosh-Sellavel correct! I'd just hardcode each step individually to test each of them.

mdneyazahmad commented 2 years ago

Sometimes, it depends on the data from previous step and if not present there it will crash. I will test and let you know if this method works. Thank you!

mdneyazahmad commented 2 years ago

KYC Flow

Step 1
  1. tab and shift + tab works as expected.
  2. enter submits the form.
  3. There is no checkbox.

https://user-images.githubusercontent.com/77761491/164278074-db37063d-92e9-4801-a54f-bfe1bc2efb90.mp4

Step 2

There is no form element.

Screenshot 2022-04-20 at 9 50 41 PM
Step 3
  1. tab and shift + tab works does not work as expected. View printer friendly is skipped.
  2. enter does not submits the form.
  3. space does not toggle checkboxes.

https://user-images.githubusercontent.com/77761491/164278088-faab2d7c-68ec-4dc5-bc5c-ce8f28886b09.mp4

Step 4

There is no form element. It has just text.

mdneyazahmad commented 2 years ago

Other issues

  1. Step 1 and step 3 have footer button that is not fixed at the bottom.

https://user-images.githubusercontent.com/77761491/164279347-288eec03-1777-4cb5-98f5-b9ff8955c632.mp4

  1. Step 2 footer button has double spacing. Screenshot 2022-04-20 at 10 03 44 PM
thienlnam commented 2 years ago

@mdneyazahmad Thanks for the videos

For the KYC flow - Step 1 and step 2 look good. No changes needed there

Let's address these issues in step 3

tab and shift + tab works does not work as expected. View printer friendly is skipped. enter does not submits the form. space does not toggle checkboxes.

As for the other issues, let's go ahead and have those addressed in the PR as well

mdneyazahmad commented 2 years ago

I am not sure about the checkbox implementation. Current version uses a button for checkbox which is toggled when button is pressed (Enter). The ideal component to use here is the native checkbox element (eg. <input type="checkbox"> on web). This way browser will provide default behaviour for accessibility and will dispatch an SubmitEvent when Enter is pressed.

I am curious to know how the other issue is solved and based on that the implementation may change here for form submit with Enter.

cc: @thienlnam @Santhosh-Sellavel

Santhosh-Sellavel commented 2 years ago

I am curious to know how the other issue is solved and based on that the implementation may change here for form submit with Enter.

Sorry What other issue? could you add more context, please!

@mdneyazahmad

Santhosh-Sellavel commented 2 years ago

@mdneyazahmad

Regarding IOU Confirmation we should check all IOU operation confirmation pages.

  1. Request Money
  2. Send Money
  3. Split Bill Using FAB in LHN
  4. Split Bill from group chat/multiple participants.

Thanks!

thienlnam commented 2 years ago

The ideal component to use here is the native checkbox element (eg. on web).

This is interesting, we could add a web version of the checkbox that just uses this but is there a way we can have this done with React Native adding a shortcut when the checkbox is focused?