Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$250] mWeb – Chat – Blue frame appears around + button when open conversation link URL in a new tab #46109

Open
1 of 6 tasks
lanitochka17 opened this issue Jul 24, 2024 · 64 comments
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review

Comments

@lanitochka17
Copy link

lanitochka17 commented Jul 24, 2024

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: 9.0.11-2
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/
  2. Log in with Gmail account
  3. Open any chat and copy URL
  4. Open new tab and paste URL

Expected Result:

Chat opens without blue frame around + button

Actual Result:

Chat opens with blue frame around + button

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6551355_1721811739477.Blue_frame__.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~011ced2d499965abe3
  • Upwork Job ID: 1816181351346781688
  • Last Price Increase: 2024-08-21
  • Automatic offers:
    • tsa321 | Contributor | 103953960
Issue OwnerCurrent Issue Owner: @sobitneupane
@lanitochka17 lanitochka17 added DeployBlockerCash This issue or pull request should block deployment DeployBlocker Indicates it should block deploying the API labels Jul 24, 2024
Copy link

melvin-bot bot commented Jul 24, 2024

Triggered auto assignment to @srikarparsi (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@lanitochka17
Copy link
Author

We think that this bug might be related to #vip-vsp

@lanitochka17
Copy link
Author

Issue is not reproducible on Production:

Screenrecorder-2024-07-24-16-05-10-406.mp4

@francoisl francoisl removed the DeployBlocker Indicates it should block deploying the API label Jul 24, 2024
@francoisl
Copy link
Contributor

Possibly related to #45743 (?) Anyway, doesn't look like something worth blocking for – going to demote.

@francoisl francoisl added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Jul 24, 2024
@srikarparsi srikarparsi added the External Added to denote the issue can be worked on by a contributor label Jul 24, 2024
Copy link

melvin-bot bot commented Jul 24, 2024

Job added to Upwork: https://www.upwork.com/jobs/~011ced2d499965abe3

@melvin-bot melvin-bot bot changed the title mWeb – Chat – Blue frame appears around + button when open conversation link URL in a new tab [$250] mWeb – Chat – Blue frame appears around + button when open conversation link URL in a new tab Jul 24, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 24, 2024
Copy link

melvin-bot bot commented Jul 24, 2024

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

@srikarparsi
Copy link
Contributor

Hi @tsa321 @hoangzinh, if you have a second, do you think you could check if this issue is related to #45743

@tsa321
Copy link
Contributor

tsa321 commented Jul 24, 2024

@srikarparsi, I have tried reverting my PR, but this issue is still reproducible.

@tsa321
Copy link
Contributor

tsa321 commented Jul 25, 2024

Edited by proposal-police: This proposal was edited at 2024-08-13 12:48:11 UTC.

Proposal

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

Blue border appears on + button when user open report by url

What is the root cause of that problem?

We use focusTrap to maintain tab navigation. When a user visits a page via URL, focusTrap initially focuses on the tabbable element, causing the blue border to appear. This issue is not limited to the report page; it occurs on several other pages as well, such as the About page, Profile page, Workspace List page, and briefly in the display name field, etc

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

We can disable the blue border using CSS styling when the page loads. We can then enable the blue border only when the user presses the Tab key. If any other key is pressed, we should disable the blue border again. The code could be:

let sheet;
const mangaeFocusedElementBlueBorder = () => {
    if (!document?.body) {
        return;
    }
    sheet = document.createElement('style');
    sheet.innerHTML = '[tabindex="0"]:focus{box-shadow: none; outline: none;}'
    document.body.appendChild(sheet);
    window.addEventListener('keydown', (e) => {
        if (e.key === 'Tab' || e.key === 'Shift') {
            if (e.key === 'Tab' && !!sheet.parentNode) {
                sheet.parentNode.removeChild(sheet);
            }
        } else if (!sheet.parentNode) {
            document.body.appendChild(sheet);
        }
    }, true);
}

We could place it in the Accessbility library

For the css selector, we could use *:focus instead of [tabindex="0"]:focus

What alternative solutions did you explore? (Optional)

In the FocusTrapForScreen option:

focusTrapOptions={{
trapStack: sharedTrapStack,

we could add an onActivate property with the function: () => document?.activeElement?.blur(). Then, in initialFocus:

initialFocus: (focusTrapContainers) => {
if (!canFocusInputOnScreenFocus()) {
return false;
}
const isFocusedElementInsideContainer = focusTrapContainers?.some((container) => container.contains(document.activeElement));
if (isFocusedElementInsideContainer) {
return false;
}
return undefined;
},

we should always return false there.

The focus trap already listens for the focus event and will check whether the focused element is inside the element to which it is attached (in this case, inside the screen) and will automatically move the focus inside the screen if it is outside.

What alternative solutions did you explore? (2)

To also fix this in strict mode, we could add activeElementRef and include onActivate and onDeactivate options in focusTrapOptions. In onActivate, we store the current active element in the ref, then blur it; in onDeactivate, we focus that element or if don't want to return the we don't need the onDeactivate callback. We must also set initialFocus and setReturnFocus to always return false. I have created a test branch to highlight the code changes.

@sobitneupane
Copy link
Contributor

sobitneupane commented Jul 26, 2024

Thanks for your proposal @tsa321

Can you please elaborate the root cause section of your proposal?

When a user visits a page via URL, focusTrap initially focuses on the tabbable element, causing the blue border to appear.

Can you please link the code which implements above feature? Let's try to investigate more on the root cause.

Why is the issue limited to some pages only?

@tsa321
Copy link
Contributor

tsa321 commented Jul 28, 2024

@sobitneupane,

Here:

initialFocus: (focusTrapContainers) => {
if (!canFocusInputOnScreenFocus()) {
return false;
}
const isFocusedElementInsideContainer = focusTrapContainers?.some((container) => container.contains(document.activeElement));
if (isFocusedElementInsideContainer) {
return false;
}
return undefined;

In the initialFocus of focusTrap, if the return value is false, the focusTrap will not automatically focus on a tabbable element inside the screen. However, if the return value is undefined, the focusTrap will focus on an element inside it.

(Assuming the return value of initialFocus is undefined.) When a user opens a page by clicking a button, the blue border won't appear. However, when the user refreshes the page or opens it by pressing a keyboard key (for example, pressing Enter on a button), the blue border will appear. This is the default behavior of web browsers for tab navigation.

@melvin-bot melvin-bot bot added the Overdue label Jul 28, 2024
Copy link

melvin-bot bot commented Jul 29, 2024

@sobitneupane, @srikarparsi Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Sep 2, 2024
Copy link

melvin-bot bot commented Sep 4, 2024

@sobitneupane, @srikarparsi 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@tsa321
Copy link
Contributor

tsa321 commented Sep 5, 2024

Kindly bump for proposal review, thanks.

@srikarparsi
Copy link
Contributor

Asked in the swm channel since I'm not too familiar with focus trap.

Copy link

melvin-bot bot commented Sep 6, 2024

@sobitneupane, @srikarparsi 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

@mvtglobally
Copy link

Issue not reproducible during KI retests. (Second week)

@srikarparsi
Copy link
Contributor

I'm not able to reproduce this either, @sobitneupane can you check if this is reproducible for you.

@tsa321
Copy link
Contributor

tsa321 commented Sep 10, 2024

@srikarparsi This issue is still reproducible when refreshing other pages: such as the Preferences page, Workspace list page, Profile page, About page, many other pages.

@sobitneupane
Copy link
Contributor

@srikarparsi I can still reproduce the issue.

Screen.Recording.2024-09-10.at.17.03.38.mov

@melvin-bot melvin-bot bot removed the Overdue label Sep 10, 2024
@srikarparsi
Copy link
Contributor

Ah okay, I think @adamgrzybowski will be back tomorrow based on this comment so he might be able to better evaluate this proposal

@adamgrzybowski
Copy link
Contributor

Hey! I was working on a focus trap at some point but it changed a lot since then. However, with my limited context, the alternative solution 2 looks like a right direction to me

@mvtglobally
Copy link

Issue not reproducible during KI retests. (Third week)

@melvin-bot melvin-bot bot added the Overdue label Sep 13, 2024
Copy link

melvin-bot bot commented Sep 13, 2024

@sobitneupane, @srikarparsi Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@youssef-lr
Copy link
Contributor

This is still reproducible

@srikarparsi
Copy link
Contributor

Cool lets proceed with solution 2 then, assigning @tsa321

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 13, 2024
Copy link

melvin-bot bot commented Sep 13, 2024

📣 @tsa321 🎉 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 📖

@melvin-bot melvin-bot bot removed the Overdue label Sep 13, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 16, 2024
@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Oct 10, 2024
Copy link

melvin-bot bot commented Oct 10, 2024

This issue has not been updated in over 15 days. @sobitneupane, @srikarparsi, @tsa321 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!

@sobitneupane
Copy link
Contributor

PR deployed to production. Ready for payment.

@sobitneupane
Copy link
Contributor

@srikarparsi This is ready for payment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

9 participants