[$500] Remove Focus Outline for EngagementModal Backdrop

stitesExpensify commented 10 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: Reproducible in staging?: n/a Reproducible in production?: n/a 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 Expensify/Expensify Issue URL: Issue reported by: Slack conversation:

Action Performed:

Pull this PR https://github.com/Expensify/App/pull/32154
Create a new account on web
See that the backdrop of the engagement modal has a blue border

Expected Result:

The backdrop should not be focused

Actual Result:

The modal is focused

Workaround:

N/A

Platforms:

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

[ ] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Actual:

Expected:

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01882ae36fcc0be771
Upwork Job ID: 1737625479257964544
Last Price Increase: 2023-12-21
Automatic offers:

situchan | Contributor | 28066504
tienifr | Contributor | 28128940

melvin-bot[bot] commented 10 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01882ae36fcc0be771

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

Bug0 Triage Checklist (Main S/O)

[ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[ ] 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
[ ] 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.
[ ] 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.
[ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot[bot] commented 10 months ago

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

stitesExpensify commented 10 months ago

This is an issue for a PR that has not been merged yet but I am currently stuck on. There is a minor visual bug that needs to be fixed, and the accepted proposal will be paid even though I will be the one implementing it. https://github.com/Expensify/App/pull/32154

melvin-bot[bot] commented 10 months ago

📣 @situchan 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer 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 📖

stitesExpensify commented 10 months ago

@situchan is the C+ here, not the contributor

abzokhattab commented 10 months ago

Hi, @stitesExpensify I am not sure I see the blue borders can you please point them out or elaborate more? thanks

tienifr commented 10 months ago

Proposal

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

The modal is focused

What is the root cause of that problem?

The backdrop element of react-native-modal by default has tabindex=0, so it can get the focus. When the modal opens right when the page is loaded, the browser will focus on that backdrop since it's the largest focusable element.

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

We can either:

Create an upstream PR/patch to react-native-modal to disable the tabindex
Use a custom backdrop with tabindex disabled (by using the Overlay component for example)

What alternative solutions did you explore? (Optional)

Keep the tab index but just remove the blue focus ring.

situchan commented 10 months ago

@tienifr thanks for the proposal. Can you please checkout this branch and test your fix? Please share code diff so I can test as well

stitesExpensify commented 10 months ago

My apologies @abzokhattab here is the correct screenshot. It is a very small accessibility border

stitesExpensify commented 10 months ago

If we are going to make a custom backdrop (as opposed to a modal prop based solution), I think we should first look into how we currently do this for pages like "workspace settings" where we have a RHP with a backdrop that doesn't have this problem

tienifr commented 10 months ago

@tienifr thanks for the proposal. Can you please checkout https://github.com/Expensify/App/pull/32154 and test your fix? Please share code diff so I can test as well

Sure thing!

augustmuir commented 10 months ago

Hi, I fixed the bug, see video for proof. Please accept my Upwork Proposal (August Kimacovich), and I will create a branch with the fix. Thanks!

(I pressed tab in the video to show where the focus is at after page load).

cc @stitesExpensify

https://github.com/Expensify/App/assets/72067396/a9b356e9-5e1b-411f-bb0c-0ed334fffa9f

melvin-bot[bot] commented 10 months ago

📣 @augustmuir! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Make sure you've read and understood the contributing guidelines.
Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:
```
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
```

augustmuir commented 10 months ago

Contributor details Your Expensify account email: august.kimo@icloud.com Upwork Profile Link: https://www.upwork.com/freelancers/~01b8790c09920fd2c7

melvin-bot[bot] commented 10 months ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

augustmuir commented 10 months ago

@stitesExpensify @situchan Apologies for the bad PR - new here and thought I was in my own forked repo! 🙃

Anyways, see this commit for the fix: https://github.com/augustmuir/ExpensifyApp/commit/88c01af7b1cf7550532fb7c7f6fb00d9a55a9e38

Should be tested on iOS/Android as well to ensure the button is not visible.

Please accept my Upwork proposal as well, hope to start working on some of the issues here. Thanks!

tienifr commented 10 months ago

@tienifr thanks for the proposal. Can you please checkout https://github.com/Expensify/App/pull/32154 and test your fix? Please share code diff so I can test as well

@situchan @stitesExpensify Here're my changes https://github.com/Expensify/App/compare/stites-purposeForExpensify...tienifr:fix/33389?expand=1

bernhardoj commented 10 months ago

The blue outline shows because of the modal focus trap. It will focus on the first focusable element (tabindex = 0), the overlay/backdrop. Also, the outline will only show if we use the keyboard while logging in. If we input the magic code and manually press sign-in using a mouse, the blue outline won't show (the overlay is still focused).

Why do we use a modal instead of a screen?

situchan commented 10 months ago

Why do we use a modal instead of a screen?

cc: @stitesExpensify

I think it's because it's one time show only after sign up. There's no chance of showing that page again. So it's just replacement for auto FAB display.

stitesExpensify commented 10 months ago

Why do we use a modal instead of a screen?

cc: @stitesExpensify

I think it's because it's one time show only after sign up. There's no chance of showing that page again. So it's just replacement for auto FAB display.

That is correct

stitesExpensify commented 10 months ago

@bernhardoj is it possible to prevent that behavior? I would prefer that solution if it is

bernhardoj commented 10 months ago

@stitesExpensify I'm afraid not. I found no way to customize the focus trap.

stitesExpensify commented 10 months ago

In that case, I prefer @tienifr solution here since it is using an existing modal prop customBackdrop.

@augustmuir solution does appear to work, however creating an invisible button does not seem like the "correct" way to solve this problem.

mvtglobally commented 9 months ago

Issue not reproducible during KI retests. (First week)

tienifr commented 9 months ago

In that case, I prefer @tienifr solution here since it is using an existing modal prop customBackdrop.

@stitesExpensify Should we proceed with this solution then?

Thanks!

stitesExpensify commented 9 months ago

@tienifr yes we're going to proceed with this solution. It will be paid out after the main PR has been merged. That being said, I believe that this is causing a typescript error, do you know why that may be? https://github.com/Expensify/App/actions/runs/7416782811/job/20182296471?pr=32154

tienifr commented 9 months ago

@stitesExpensify sorry, it looks like the GH action failure you linked is related to another issue

Is that the correct link?

stitesExpensify commented 9 months ago

@tienifr that failure is happening in the overlay.tsx file, and only occurs when this line from your solution is present https://github.com/Expensify/App/compare/stites-purposeForExpensify...tienifr:fix/33389?expand=1#diff-c5dd0a065b12f253786ab2916473640a80014957475696db4d97bedc23f47af2R206

stitesExpensify commented 9 months ago

This was the fix FYI https://github.com/Expensify/App/pull/32154/commits/4df4d3aaa85d864949854068fc3b63a92ddd4255

mvtglobally commented 9 months ago

Issue not reproducible during KI retests. (Second week)

mvtglobally commented 9 months ago

Issue not reproducible during KI retests. (Third week)

melvin-bot[bot] commented 8 months ago

This issue has not been updated in over 15 days. @bfitzexpensify, @stitesExpensify, @situchan eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

melvin-bot[bot] commented 8 months ago

📣 @tienifr 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer 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 📖

stitesExpensify commented 8 months ago

The main PR has been merged here!

@bfitzexpensify can you please pay out @situchan and @tienifr ? What happened here is: I was working on this pr and needed help with a particular part of it. @tienifr came up with a solution that I used in that PR, and @situchan was the C+ that approved it, so we will pay them both out as if the PR was made separately from mine (AKA the normal way).

mvtglobally commented 8 months ago

Issue not reproducible during KI retests. (Fourth week)

situchan commented 8 months ago

Seems like this is out of everyone's radar. @stitesExpensify can you please remove Monthly, Reviewing and add Daily, Awaiting Payment?

bfitzexpensify commented 8 months ago

Thanks for the bump @situchan and sorry for the delay. Contracts have now been paid out.

Expensify / App