App: [HOLD for payment 2022-04-13] [HOLD for payment 2022-04-12] Audit forms and fix inconsistencies with focus, tab and shift + tab behavior 1/4

Just a thought here, I think focus trapping is useful here RHN is kind of modal as they block interaction with the whole app behind them.

Thanks for the proposed solutions @Santhosh-Sellavel! Here are my thoughts:

Form 1 && Form 5 Can we remove the gap between borders somehow? It looks a bit off.

Form 2 && Form 3 Sounds good! Let’s do it.

Form 4 I agree. I think it would make sense to remove the tabbing if we didn’t have the input pattern defined there. Let’s do nothing here.

Checkbox & Text has separate onPress. So focusing on them separately makes sense. If we want to achieve the focus to both we can do this by wrapping Text & Checkbox inside a Pressable instead of Text alone here. But still it focuses the Checkbox alone separately after focusing the both, Checkbox alone is still clickable.

On second thought, let’s keep it as is.

https://github.com/Expensify/App/issues/7523#issuecomment-1065495907 - I think we should keep the focus on the show password button for accessibility. Could we make it more obvious though? I feel like the left border seems off.

https://github.com/Expensify/App/issues/7523#issuecomment-1065520027 - Hmm I think we should toggle the checkbox on “Enter” when it has focus, instead of submitting the form. Additionally, I think the checkbox and text should be focused at the same time.

https://github.com/Expensify/App/issues/7523#issuecomment-1065497572 - Same comment above about the text and checkbox being focused separately (this does not apply to the link though, as that one should be focused separately in case a user wants to click it). What happens when we press “Enter” with the checkbox in focus?

https://github.com/Expensify/App/issues/7523#issuecomment-1065512322 - Same comment as above about the checkbox. As for date picker, I think that we should remove tab from within it, i.e. pressing tab should move us to the next input.

https://github.com/Expensify/App/issues/7523#issuecomment-1065500916 - I think we should keep the focus on the show password button.

No worries, thanks for your input on this! 😃

When you tab and reach the last and now when you press tab you should focus on the first input and not on the element outside of the modal.

Wait, really? I don’t think we should do that. This will break accessibility features when using tab.

Please correct me if I’m wrong

Yep, and the order is from Top to Bottom and Left to Right for Tab

We don’t support right to left scripts yet, so this makes the most sense to me. Appreciate that you ask questions when in doubt 😃

In other words, the goal of the task is to make sure that performing tab & shift + tab operations ensure that the focus is switching to the next or previous input in focus cycles the top to bottom or bottom to top in the right orders. Once the end of When First or Last Input is reached by performing tab operations, the focus could switch to the next view that can be selected via tab.

cc: @rushatgabhane

Payment sent, contract ended, post removed!

Repost

Internal: https://www.upwork.com/ab/applicants/1514144553994493952/job-details External: https://www.upwork.com/jobs/~011512c770333f888a

cc @Santhosh-Sellavel

I believe the payout for this job was originally set to $250 each, right? Agree with @thienlnam 👍

Hmm, I’ve verified the audit. But I didn’t help here in any other way 😅 Sorry about that.

The PR was straightforward too. I’m gonna unassign myself as a C+ I hope this is okay

Form 1 && Form 5 Can we remove the gap between borders somehow? It looks a bit off.

How does this look?

5px corner radius

6px corner radius

7px corner radius

6px looks fine for me. Can leave it like the above, corner radius for the right side alone, or let’s add all sides?

cc: @luacmartins

Thanks, @luacmartins!

Form 2 && Form 3 Sounds good! Let’s do it.

Raised a request for this https://expensify.slack.com/archives/C01GTK53T8Q/p1648488615567699

@luacmartins

Form 1 && Form 5

https://github.com/Expensify/App/issues/7523#issuecomment-1065495907 - I think we should keep the focus on the show password button for accessibility. Could we make it more obvious though? I feel like the left border seems off. https://github.com/Expensify/App/issues/7523#issuecomment-1065500916 - I think we should keep the focus on the show password button.

Solution

The height of the showPassword button and password input is the same. Since it is rendered inside the container except left all other borders got cut off

Applying 1px margin and applying border-radius same as the container would look like this.

___________________________________________________

Form 2 && Form 3

https://github.com/Expensify/App/issues/7523#issuecomment-1065520027 - Hmm I think we should toggle the checkbox on “Enter” when it has focus, instead of submitting the form. Additionally, I think the checkbox and text should be focused at the same time. https://github.com/Expensify/App/issues/7523#issuecomment-1065497572 - Same comment above about the text and checkbox being focused separately (this does not apply to the link though, as that one should be focused separately in case a user wants to click it). What happens when we press “Enter” with the checkbox in focus?

Solution:

I proposed to use space for check selection. Discussed on the https://github.com/Expensify/App/issues/7523#issuecomment-1068209007 https://github.com/Expensify/App/issues/7523#issuecomment-1068548722

To use Space bar for selection is separate a feature request, I propose we can handle this as a separate feature request, not with this issue. If you agree with me I will post it on slack or let’s create a ticket on GH itself.

The above will address the Enter conflict behavior.

---

Form 4

https://github.com/Expensify/App/issues/7523#issuecomment-1065512322 - Same comment as above about the checkbox. As for the date picker, I think that we should remove the tab from within it, i.e. pressing the tab should move us to the next input.

For Date Picker, I believe working as expected. By default, it’s allowed to navigate through all fields for a date the reference here => https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/date.

---

Discussion

About,

I think the checkbox and text should be focused at the same time.

Checkbox & Text has separate onPress. So focusing on them separately makes sense. If we want to achieve the focus to both we can do this by wrapping Text & Checkbox inside a Pressable instead of Text alone here. But still it focuses the Checkbox alone separately after focusing the both, Checkbox alone is still clickable.

Please post your thoughts on this! cc: @thienlnam @rushatgabhane

Not yet started to work on the fixes, will provide an update on this next week!

The checkbox is getting selected & immediately form also gets submitted!

Hmm if we are using “Space” to toggle the checkbox, then pressing “Enter” should submit the form and I’d think that this is expected?

I believe using the “Space” key would make the most obvious choice to toggle between selections in the checkbox. That’s the commonly used way.

Yes, that makes sense. We should add that if it’s not currently a feature.

@Santhosh-Sellavel Please proceed with the form audits

I agree! Let’s stick to auditing the forms for now. We might be changing the way the RHN is displayed soon anyways.

Reassigning as I’m OOO next week! Thanks Hax! I’ll pick it up when I’m back!

Since there isn’t a PR associated with this, instead could you just record a video going through each form and demonstrating that these work as expected? That way, we can consider it completed

@luacmartins or @rushatgabhane Have a look at the below video. I am performing Tab and Shift + Tab operations. After reaching the First or Input at the top. When I perform Shift + Tab, the focus changes to the close button. This is expected behavior right?

https://user-images.githubusercontent.com/85645967/156248891-db7c2076-f56a-4dda-99be-0754ace2e2dc.mov

@Santhosh-Sellavel glad that I could help getting access to the forms. It seems that AddPlaidBankAccount.js will have to be audited internally though.

Still have some issues with adding a bank account.

Requested help on slack here

cc: @thienlnam @rushatgabhane

@dylanexpensify please note that we should add one milestone per 5 forms (4 milestones total).

Actually, we might use Applause to do this audit for us. Removing the External label.