App icon indicating copy to clipboard operation
App copied to clipboard

[HOLD OnFido response][$1000] Pressing Enter/Return from DOB field highlights “choose your document “ text in next onfindo page.

Open kavimuru opened this issue 2 years ago • 43 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Go to staging.new.expensify.com on mweb
  2. Go to Settings > Workspaces > [select a workspace] > Connect bank account.
  3. Fill in routing and bank details in connect bank account page.
  4. Go to the Personal Information page. Fill in all the details.
  5. Now go to the Date of Birth field ( press click )
  6. Now press enter
  7. The application must redirect to a new page
  8. Notice that the text “ Choose your documents” is highlighted by a blue border.
  9. Plus in Mac chrome pressing enter on any field results in the highlighting issue.

Expected Result:

Text should not be highlighted while pressing return from previous page

Actual Result:

Highlighting certain text on the onfindo page degrades UI.

Workaround:

unknown

Platforms:

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

  • [ ] Android / native
  • [ ] Android / Chrome
  • [ ] iOS / native
  • [ ] iOS / Safari
  • [x] MacOS / Chrome / Safari
  • [ ] MacOS / Desktop

Version Number: 1.2.90-7 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/43996225/228321188-0330f68f-091b-4dee-bf0a-df9c0b2f4c17.mp4

https://user-images.githubusercontent.com/43996225/228321304-203f4621-cf9a-4cac-8794-7334d74afdcd.mp4

Expensify/Expensify Issue URL: Issue reported by: @ashimsharma10 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1679996395453299

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0143cbaf19b602982f
  • Upwork Job ID: 1642082307866021888
  • Last Price Increase: 2023-04-15

kavimuru avatar Mar 28 '23 17:03 kavimuru

Triggered auto assignment to @mallenexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

MelvinBot avatar Mar 28 '23 17:03 MelvinBot

Bug0 Triage Checklist (Main S/O)

  • [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • [x] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • [x] This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • [x] This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • [x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

MelvinBot avatar Mar 28 '23 17:03 MelvinBot

Job added to Upwork: https://www.upwork.com/jobs/~0143cbaf19b602982f

MelvinBot avatar Apr 01 '23 08:04 MelvinBot

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

MelvinBot avatar Apr 01 '23 08:04 MelvinBot

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

MelvinBot avatar Apr 01 '23 08:04 MelvinBot

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

MelvinBot avatar Apr 01 '23 08:04 MelvinBot

Confirmed it's a bug, it only happens the first time and it also might only stick around for a few seconds. When I went back then tried again the border wasn't there. ¯_(ツ)_/¯

Gonna guess this is external, correct me if I'm wrong @tgolen

@ashimsharma10 when you're including steps can you be more specific about the exact steps to take, assume that others won't know. ie.

Go to Settings > Workspaces > [select a workspace] > Connect bank account.

vs "Fill in routing and bank details in connect bank account page." Thx

mallenexpensify avatar Apr 01 '23 08:04 mallenexpensify

Proposal

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

Pressing Enter/Return on any of the form field in workspace bank account personal information step results in a blue border on the "choose your document" text in onfido page

What is the root cause of that problem?

The root cause is that Onfido has multiple elements set as having tabindex="-1". This particular value of tabindex makes a component highlighted whenever it comes into view via keyboard navigation.

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

We can disable focus-visible on the onfido container elements that have a tabindex of -1. To do this, we can add the following here:

        #onfido-mount [tabindex="-1"]:focus-visible,
        #onfido-mount [tabindex="-1"]:focus[data-focusvisible-polyfill] {
            box-shadow: none !important;
        }

This will not break the tab navigation but will just disable the focus-visible styles that get applied when the component mounts.

What alternative solutions did you explore? (Optional)

If we want, we can extend this further to apply it across our whole app (not just #onfido-mount children). Instead of using the #onfido-mount class, we can also use a class of the individual elements in onfido that have tabindex value of -1. I think that would be a stretch and we should apply this to all elements of the #onfido-mount container.

allroundexperts avatar Apr 01 '23 10:04 allroundexperts

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

MelvinBot avatar Apr 01 '23 10:04 MelvinBot

hey @allroundexperts same thought with you. To avoid it's duplicated, I have a suggestion for you that you can put your CSS style into this file https://github.com/Expensify/App/blob/main/src/components/Onfido/index.css for Onfido. Just a suggestion, so it's up to you.

hoangzinh avatar Apr 01 '23 10:04 hoangzinh

hey @allroundexperts same thought with you. To avoid it's duplicated, I have a suggestion for you that you can put your CSS style into this file https://github.com/Expensify/App/blob/main/src/components/Onfido/index.css for Onfido. Just a suggestion, so it's up to you.

@hoangzinh Thanks for the suggestion. I agree that Onfido/index.css is a better place to put the code at.

allroundexperts avatar Apr 01 '23 11:04 allroundexperts

Current status: Issue appears to have one proposal that still needs to be vetted by a C+.

tgolen avatar Apr 03 '23 15:04 tgolen

@allroundexperts's proposal looks good to me

This is technically a workaround and in an ideal world we would make changes to onfido-sdk-ui itself (by setting tabIndex to 0), but it doesn't look like it's possible (the code is minified). This is the next best thing we can do

🎀👀🎀 C+ reviewed cc: @tgolen

eVoloshchak avatar Apr 03 '23 19:04 eVoloshchak

Sorry for interrupt. I suggest to raise issue in onfido ui repo and they will handle it since we're already getting paid service from them. There are already many of our issues fixed by onfido team itself. IMO, it's better to do nothing than applying such workaround.

situchan avatar Apr 03 '23 20:04 situchan

Yeah, I agree with @situchan. Let's see if we can get them to fix their code, and only if we aren't able to get anywhere with them should we look at doing any workarounds.

tgolen avatar Apr 03 '23 21:04 tgolen

@tgolen I wonder if this is really a bug on there side? I think setting tabIndex to -1 whenever the component mounts as a result of keyboard navigation is fine. The styles for that tabIndex are being applied in our code.

allroundexperts avatar Apr 03 '23 21:04 allroundexperts

Note: tabindex="-1" may be useful for elements that should not be navigated to directly using the Tab key, but need to have keyboard focus set to them. Examples include an off-screen modal window that should be focused when it comes into view, or a form submission error message that should be immediately focused when an errant form is submitted.

Looks like this could be the expected behavior from onfido-sdk-ui's point of view, not a bug If so, they could add a prop that would disable this behavior

eVoloshchak avatar Apr 03 '23 21:04 eVoloshchak

Note: tabindex="-1" may be useful for elements that should not be navigated to directly using the Tab key, but need to have keyboard focus set to them. Examples include an off-screen modal window that should be focused when it comes into view, or a form submission error message that should be immediately focused when an errant form is submitted.

Looks like this could be the expected behavior from onfido-sdk-ui's point of view, not a bug If so, they could add a prop that would disable this behavior

I think its fine as it is. Perhaps we should be more careful with adding global outline styles?

allroundexperts avatar Apr 03 '23 21:04 allroundexperts

Maybe we could start with opening an issue (not claiming it's a bug) and trying to get their thoughts on it? Maybe they have some suggestions or would do a preference toggle for us.

tgolen avatar Apr 03 '23 21:04 tgolen

Maybe we could start with opening an issue (not claiming it's a bug) and trying to get their thoughts on it? Maybe they have some suggestions or would do a preference toggle for us.

Can I open it or is this something that someone from the team needs to do?

allroundexperts avatar Apr 03 '23 21:04 allroundexperts

Yeah, you can go for it!

tgolen avatar Apr 03 '23 22:04 tgolen

@tgolen I just found out that we have a dedicated CSM manager for Onfido. Should we email him to speed up the process?

allroundexperts avatar Apr 05 '23 17:04 allroundexperts

Yeah, sounds like a great plan. Thanks for running with this!

tgolen avatar Apr 05 '23 18:04 tgolen

Thanks @allroundexperts !

mallenexpensify avatar Apr 05 '23 19:04 mallenexpensify

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

MelvinBot avatar Apr 08 '23 16:04 MelvinBot

@allroundexperts did you reach out to Onfido? I think the best practice for a situation like this would be to cc [email protected] for visibility. This allows for Onfido (and/or other vendors) to be able to get a quick confirmation that the contributors inquiring on the behalf of Expensify, since [email protected] is regularly monitored by a BugZero team member

mallenexpensify avatar Apr 10 '23 14:04 mallenexpensify

@allroundexperts did you reach out to Onfido? I think the best practice for a situation like this would be to cc [email protected] for visibility. This allows for Onfido (and/or other vendors) to be able to get a quick confirmation that the contributors inquiring on the behalf of Expensify, since [email protected] is regularly monitored by a BugZero team member

@mallenexpensify I thought someone from the team was supposed to do this. Apologies for the confusion. I'll email them now.

allroundexperts avatar Apr 10 '23 14:04 allroundexperts

Update: I've sent an email and CC'd [email protected]

allroundexperts avatar Apr 10 '23 15:04 allroundexperts

Another update. Just received an automated reply saying that he will return to the office tomorrow ie Tuesday 11th of April.

allroundexperts avatar Apr 10 '23 15:04 allroundexperts

@tgolen @eVoloshchak @mallenexpensify this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

MelvinBot avatar Apr 11 '23 10:04 MelvinBot