Closed kavimuru closed 1 year ago
Triggered auto assignment to @mallenexpensify (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Platforms
in OP are ✅)Job added to Upwork: https://www.upwork.com/jobs/~0143cbaf19b602982f
Current assignee @mallenexpensify is eligible for the External assigner, not assigning anyone new.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External
)
Triggered auto assignment to @tgolen (External
), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
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
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
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.
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.
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.
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.
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.
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.
Current status: Issue appears to have one proposal that still needs to be vetted by a C+.
@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
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.
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 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.
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
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?
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.
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?
Yeah, you can go for it!
@tgolen I just found out that we have a dedicated CSM manager for Onfido. Should we email him to speed up the process?
Yeah, sounds like a great plan. Thanks for running with this!
Thanks @allroundexperts !
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@allroundexperts did you reach out to Onfido? I think the best practice for a situation like this would be to cc contributors@expensify.com 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 contributors@expensify.com is regularly monitored by a BugZero team member
@allroundexperts did you reach out to Onfido? I think the best practice for a situation like this would be to cc contributors@expensify.com 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 contributors@expensify.com 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.
Update: I've sent an email and CC'd contributors@expensify.com
Another update. Just received an automated reply saying that he will return to the office tomorrow ie Tuesday 11th of April.
@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, we're making active progress on this and waiting to hear back from Onfido.
@allroundexperts any response from Onfido yet? Maybe try bumping them again since it's been a couple of days since they were supposed to return?
@allroundexperts any response from Onfido yet? Maybe try bumping them again since it's been a couple of days since they were supposed to return?
@tgolen I've already sent a reminder email to them earlier today. I cc'd contributors@expensify.com as well.
Uodate. Received a reply. They said that someone from there team will be looking at this shortly.
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@tgolen, @eVoloshchak, @mallenexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!
Not overdue, waiting for a response from Onfido team
@tgolen @eVoloshchak @mallenexpensify this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!
I'm going to drop this down to weekly while we continue to wait for a response.
Opened the required slack discussion here: https://expensify.slack.com/archives/C01GTK53T8Q/p1681828171039139
Still no response from Onfido
Update: Received a response from Onfido. The engineer there was unable to reproduce the issue. I've sent him a reply with a screen recording of the steps that are required to reproduce the issue.
Thanks for the update!
@allroundexperts any new updates?
@tgolen This was the last reply 6 days ago:
Hi Sibtain,
Thank you for explaining the steps and sharing your code. I was able to reproduce a similar behaviour in both our Web SDK sample app.
Web sdk "10.4.0" https://jsfiddle.net/ThiagoFariasSupport/wd3mqj74/38/
Web sdk "12.2.1" https://jsfiddle.net/ThiagoFariasSupport/3L8edhjr/4/
Notice that on those apps the text "Choose your document" is not highlighted/selected while pressing return from the welcome screen (button "choose document). When landing on this screen to select the document type the key tab needs to be pressed.
It doesn't seem to be a bug, I'm confident it has always worked like that. Regardless, I'm reaching out to our SDK engineering team to confirm this I'll let you know as soon as possible.
OK, thanks! Would you mind reaching out to them and asking for an update?
On Mon, May 8, 2023 at 7:45 AM Sibtain Ali @.***> wrote:
@tgolen https://github.com/tgolen This was the last reply 6 days ago:
Hi Sibtain,
Thank you for explaining the steps and sharing your code. I was able to reproduce a similar behaviour in both our Web SDK sample app. Web sdk "10.4.0" https://jsfiddle.net/ThiagoFariasSupport/wd3mqj74/38/
Web sdk "12.2.1" https://jsfiddle.net/ThiagoFariasSupport/3L8edhjr/4/
Notice that on those apps the text "Choose your document" is not highlighted/selected while pressing return from the welcome screen (button "choose document). When landing on this screen to select the document type the key tab needs to be pressed.
It doesn't seem to be a bug, I'm confident it has always worked like that. Regardless, I'm reaching out to our SDK engineering team to confirm this I'll let you know as soon as possible.
— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/16638#issuecomment-1538387129, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJMAB5HZKUHCLIR5K3D3TTXFD2INANCNFSM6AAAAAAWK4BQNA . You are receiving this because you were mentioned.Message ID: @.***>
OK, thanks! Would you mind reaching out to them and asking for an update? … On Mon, May 8, 2023 at 7:45 AM Sibtain Ali @.> wrote: @tgolen https://github.com/tgolen This was the last reply 6 days ago: Hi Sibtain, Thank you for explaining the steps and sharing your code. I was able to reproduce a similar behaviour in both our Web SDK sample app. Web sdk "10.4.0" https://jsfiddle.net/ThiagoFariasSupport/wd3mqj74/38/ Web sdk "12.2.1" https://jsfiddle.net/ThiagoFariasSupport/3L8edhjr/4/ Notice that on those apps the text "Choose your document" is not highlighted/selected while pressing return from the welcome screen (button "choose document). When landing on this screen to select the document type the key tab needs to be pressed. It doesn't seem to be a bug, I'm confident it has always worked like that. Regardless, I'm reaching out to our SDK engineering team to confirm this I'll let you know as soon as possible. — Reply to this email directly, view it on GitHub <#16638 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJMAB5HZKUHCLIR5K3D3TTXFD2INANCNFSM6AAAAAAWK4BQNA . You are receiving this because you were mentioned.Message ID: @.>
@tgolen Sure.
@tgolen Received the following reply:
Hi Sibtain,
Sorry for the delay. I'm confirming with our Product manager if this is really a bug or a feature request, and I'll let you know as soon as possible.
On holding pending action from onFido
Update: I sent a reminder email 1 day ago. Still waiting for a reply.
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
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?
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