[HOLD for payment 2024-07-25] [$500] Web - Expense - Blue border selection on back icon RHP after hitting enter on BNP

[lanitochka17]

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: 1.4.83-0 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 staging.new.expensify.com
2. Go to chat
3. Go to + > Submit expense > Manual
4. Enter amount
5. Hit Enter

Expected Result:

The back icon on RHP will not have blue border selection

Actual Result:

The back icon on RHP has blue border selection

Workaround:

Unknown

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

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/cd5b4495-0c04-40dd-bac9-1c8018391d49

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018358d1ec7f51e120
• Upwork Job ID: 1801251133562170195
• Last Price Increase: 2024-06-27
• Automatic offers:
• suneox | Contributor | 102988636

Issue Owner

Current Issue Owner: @s77rt

[melvin-bot[bot]]

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

[github-actions[bot]]

:wave: 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]

@danieldoglas FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[lanitochka17]

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

[danieldoglas]

Not a deploy blocker related to web, removing the label

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~018358d1ec7f51e120

[melvin-bot[bot]]

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

[luacmartins]

This was likely caused by https://github.com/Expensify/App/pull/39520

[gawandeabhishek]

To address this issue, we can use the .that-element:focus { outline: none; } CSS class. This rule removes the default focus outline that appears on elements when they are focused. This can enhance the visual appearance and usability of the element, especially in cases where the default browser outline is intrusive or unnecessary.

[tsa321]

Proposal

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

Web - Expense - Blue border selection on back icon RHP after hitting enter on BNP

What is the root cause of that problem?

The SCREENS.MONEY_REQUEST.STEP_PARTICIPANTS screen isn't included in

https://github.com/Expensify/App/blob/2a641d98760dfdc1a2ebe9b958787c9cfacb02c2/src/components/FocusTrap/SCREENS_WITH_AUTOFOCUS.ts#L4

which will make focusTrap to focus the first element of the screen:

https://github.com/Expensify/App/blob/2a641d98760dfdc1a2ebe9b958787c9cfacb02c2/src/components/FocusTrap/FocusTrapForScreen/index.web.tsx#L52-L55

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

include SCREENS.MONEY_REQUEST.STEP_PARTICIPANTS in SCREENS_WITH_AUTOFOCUS. And if we need to disable the auto focus on all money request screen we can use ...Object.values(SCREENS.MONEY_REQUEST)

Alternative solutions

We can disable the initialFocus of FocusTrap. We set false in initialFocus for all screens.

and if there is problem with other screen element is still focused and receive key event we can set onActivate property: The code that work for me is to move the focus to other element that doesn't receive keydown event:

onActivate: () => document?.querySelector('[tabindex="0"]').focus()

This could fix that issue.

Alternative solutions (2)

We can defer the blue outline appearance so it will be shown only when it is focused after some milli seconds. By using this method we can remove the initialFocus property of FocusTrap in FocusTrapForScreen.

The code could be: In FocusTrapForScreen we add a ref: const screenRef = useRef(null);

then add the screenRef in children: const childElement = React.cloneElement(children, {ref: screenRef});

then onActivate will be:

onActivate: () => {
    const firstTabElement = screenRef?.current?.querySelector('[tabindex="0"]');
    if (firstTabElement) {
        const elementBoxShadow = firstTabElement.style.boxShadow;
        firstTabElement.style.boxShadow = "none";
        setTimeout(() => {
            firstTabElement.style.boxShadow = elementBoxShadow;
        }, 300);
    }
},

then change the returned children with childElement

[s77rt]

@gawandeabhishek Please checkout the contributing guide https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md

[s77rt]

@tsa321 Thanks for the proposal. Your RCA is correct. The solution works but I assume this bug exists in many places and we will face it again when a new screen is added. Can we explore a global solution here that will also prevent this bug from occurring?

[tsa321]

@s77rt do we need to set the auto focus from FocusTrap? How about disable the initialFocus of FocusTrap. We set initialFocus to false for all screens.

[s77rt]

@tsa321 This will keep the element outside of the focus trap still focused e.g. in the report screen we can even use the arrow keys.

https://github.com/Expensify/App/assets/16493223/621eea65-99cd-43ed-ba45-c8c46994b664

[tsa321]

@s77rt To fix the issue, I have tried to blur the the active element by adding onActivate property into FocusTrap. The callback will be executed before focustrap is activated. The callback is () => document.activeElement.blur();. but this doesn't work.
The code that work for me is to move the focus to other element that doesn't receive keydown event:

onActivate: () => document?.querySelector('[tabindex="0"]').focus()

This could fix that issue and doesn't change the focused html element passed in setReturnFocus as parameter. So it doesn't change the focused element catch by FocusTrap used to refocus the element after leaving the screen.

[s77rt]

@tsa321 After opening the RHP there should be focus ring on the RHP first element

https://github.com/Expensify/App/assets/16493223/2b10f449-86ba-4863-a10d-c124f38e198c

[tsa321]

@s77rt We can defer the blue outline appearance so it will be shown only when it is focused after some milliseconds. By using this method we can remove the initialFocus property of FocusTrap in FocusTrapForScreen.

The code could be: In FocusTrapForScreen we add a ref: const screenRef = useRef(null);

then add the screenRef in children: const childElement = React.cloneElement(children, {ref: screenRef});

then onActivate will be:

onActivate: () => {
    const firstTabElement = screenRef?.current?.querySelector('[tabindex="0"]');
    if (firstTabElement) {
        const elementBoxShadow = firstTabElement.style.boxShadow;
        firstTabElement.style.boxShadow = "none";
        setTimeout(() => {
            firstTabElement.style.boxShadow = elementBoxShadow;
        }, 300);
    }
},

then change the returned children with childElement. We can adjust the timeout time to match the animation of RHP time. I have tried, 300 is fine.

[melvin-bot[bot]]

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

[s77rt]

@tsa321 This is looking too workaround-ish. So far it may be best to either disable initial focus or enable it for all (as long as we get rid of the SCREENS_WITH_AUTOFOCUS list). But for now let's see if we can find other solutions

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@danieldoglas, @s77rt Eep! 4 days overdue now. Issues have feelings too...

[s77rt]

Still looking for proposals

[melvin-bot[bot]]

Upwork job price has been updated to $500

[danieldoglas]

Raising the issue value so we can get more proposals.

[s77rt]

Still looking for proposals

[suneox]

Proposal

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

Auto focus without screen context

What is the root cause of that problem?

At ScreenWrapper, we have a FocusTrapForScreens component that prevents accessibility focus out of current screen and automatically focus on the first actionable element of screen, This is a reason why screens that do not have autofocus handling will automatically focus on the first action element when opened.

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

We can patch the focus-trap library support when get getInitialFocusNode allow pass the current container into option initialFocus

  const getInitialFocusNode = function () {
-   let node = getNodeForOption('initialFocus');
+   let node = getNodeForOption('initialFocus', state.containers);

and config.delayInitialFocus allow number value to delay focus

    state.delayInitialFocusTimer = config.delayInitialFocus
      ? delay(function () {
          tryFocus(getInitialFocusNode());
-       })
+       }, config.delayInitialFocus)
      : tryFocus(getInitialFocusNode());

Then replace option initialFocus at focusTrapOptions by find the active focused element and check if it's within the focus trap container to prevent trying initial focus.

    delayInitialFocus: 300,
    initialFocus: (containers) => {
        const hasChildFocused = containers.some((container) => container.contains(document.activeElement));

        return !hasChildFocused;
    },

And handle set focus base on screen context

What alternative solutions did you explore? (Optional)

Add SCREENS.MONEY_REQUEST.STEP_CONFIRMATION into SCREENS_WITH_AUTOFOCUS and manual handle autofocus at this step

we can patch the focus-trap library to add a new option checkInitialFocus before tryFocus from these lines.

https://github.com/focus-trap/focus-trap/blob/1038a86ba2ebed583fdf6bd396d5df89be1698bd/index.js#L817-L821

+   config.checkInitialFocus(focusTrapContainer).then(canFocus => {
+     if (!canFocus) {
+       return;
+     }
      state.delayInitialFocusTimer = config.delayInitialFocus
        ? delay(function () {
            tryFocus(getInitialFocusNode());
          })
        : tryFocus(getInitialFocusNode());
+   })

Add option checkInitialFocus into focusTrapOptions then we can get the active focused element and check if it's within the focus trap container to prevent trying initial focus.

        <FocusTrap
            active={isActive}
            paused={!isFocused}
            focusTrapOptions={{
                trapStack: sharedTrapStack,
                allowOutsideClick: true,
                fallbackFocus: document.body,
+               checkInitialFocus: (focusTrapContainer) => new Promise((resolve) => {
+                   const currentActiveElement = document.activeElement;
+                   const buttonHasEnterListener = focusTrapContainer.querySelector('button[data-listener="ENTER"]');
+                   resolve(!focusTrapContainer.contains(currentActiveElement));
+               }),
                ....

[suneox]

Proposal updated

Add alternative solution

[s77rt]

@suneox The blue border issue could occur in many cases. Can you look into a global solution? (a solution that fixes the problem for all screens) so we can remove and avoid maintaining the SCREENS_WITH_AUTOFOCUS list.

[suneox]

@suneox The blue border issue could occur in many cases. Can you look into a global solution? (a solution that fixes the problem for all screens) so we can remove and avoid maintaining the SCREENS_WITH_AUTOFOCUS list.

@s77rt Sure, we can patch the focus-trap library to add a new option checkInitialFocus before tryFocus from these lines.

https://github.com/focus-trap/focus-trap/blob/1038a86ba2ebed583fdf6bd396d5df89be1698bd/index.js#L817-L821

ST like that

+   config.checkInitialFocus().then(canFocus => {
+     if (!canFocus) {
+       return;
+     }
      state.delayInitialFocusTimer = config.delayInitialFocus
        ? delay(function () {
            tryFocus(getInitialFocusNode());
          })
        : tryFocus(getInitialFocusNode());
+   })

At the checkInitialFocus function, we will check containerElements to see if they include elements that can auto-focus such as input, textarea, and button with a listener for the KEYBOARD_SHORTCUTS.ENTER event to prevent trying initial focus.

Add this option to focusTrapOptions like that.

        <FocusTrap
            active={isActive}
            paused={!isFocused}
            focusTrapOptions={{
                trapStack: sharedTrapStack,
                allowOutsideClick: true,
                fallbackFocus: document.body,
+               checkInitialFocus: () => new Promise((resolve) => {
+                   if (focusTrapContainerRef) {
+                       // check focusTrapContainerRef element include child tag INPUT or TEXTAREA
+                       const isFocusTrapContainerRefHasInput = focusTrapContainerRef.querySelector('input, textarea');
+                       // query button has attribute data-listener="ENTER"
+                       const buttonHasEnterListener = focusTrapContainerRef.querySelector('button[data-listener="ENTER"]');
+                       if (isFocusTrapContainerRefHasInput && buttonHasEnterListener) {
+                           resolve(false);
+                       } else {
+                           resolve(true);
+                       }
+                   }
+                   resolve(true);
+               }),
                ....

Solution 2: we can update the initialFocus option with a promise or getInitialFocusNode function to get the initial focusElement by a data attribute, such as data-focus="initial" instead of maintaining the SCREENS_WITH_AUTOFOCUS

[s77rt]

@suneox Instead of looking for inputs with attributes. Can we:

1. Get the active focused element
2. If that element is within the focus trap container, skip the focus trap auto focus

[suneox]

@suneox Instead of looking for inputs with attributes. Can we:

1. Get the active focused element
2. If that element is within the focus trap container, skip the focus trap auto focus

Yeah, we can do that at function checkInitialFocus the logic look like that

    checkInitialFocus: (focusTrapContainer) => new Promise(resolve => {
        const currentActiveElement = document.activeElement;
        resolve(!focusTrapContainer.contains(currentActiveElement));
    }),

[suneox]

Proposal update

@s77rt I have updated comment

[s77rt]

@suneox We are almost there. But let's do the following:

1. Instead of adding a new config option let's just use initialFocus and pass the containers (state.containers)
2. We need to delay the focus to account for the screen transition and the auto focus delay (300ms). For that we already have a config option delayInitialFocus but we need to add a new option to specify the delay duration.

Please update your proposal accordingly

[suneox]

@suneox We are almost there. But let's do the following:

1. Instead of adding a new config option let's just use initialFocus and pass the containers (state.containers)
2. We need to delay the focus to account for the screen transition and the auto focus delay (300ms). For that we already have a config option delayInitialFocus but we need to add a new option to specify the delay duration.

Please update your proposal accordingly

@s77rt I have updated my proposal based on patch initialFocus allow pass the containers and delayInitialFocus with value

[s77rt]

@suneox Overall looks good to me but do we need the buttonHasEnterListener check?

[suneox]

@suneox Overall looks good to me but do we need the buttonHasEnterListener check?

@s77rt We need to check buttonHasEnterListener because some screens don't have any focused element, so the first element will be focused. However, the submit button just listens for the enter event and is not focused. In that case, the back button is focused, and the button listening for the enter event doesn't work.

[s77rt]

@suneox I don't see the problem here. If you navigate to a page via keyboard the first element should be focused (for easy keyboard accessibility). The only time we avoid this is if we already have a focused element.

[suneox]

@suneox I don't see the problem here. If you navigate to a page via keyboard the first element should be focused (for easy keyboard accessibility). The only time we avoid this is if we already have a focused element.

@s77rt I think the expected result for this issue should also avoid focusing on the first element because the confirmation page has a submit button ready to listen for the enter key down.

[s77rt]

@suneox For this one can we just make the button focused?

[suneox]

@suneox For this one can we just make the button focused?

@s77rt We can make button focus by adding a submitButtonRef at MoneyRequestConfirmationList then trigger focus when rendered. I also update my proposal to remove check button has event listener

[s77rt]

@suneox Sounds good

🎀 👀 🎀 C+ reviewed Link to proposal

[melvin-bot[bot]]

Current assignee @danieldoglas is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[melvin-bot[bot]]

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

[danieldoglas]

assigned @suneox

[melvin-bot[bot]]

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

[suneox]

I'll create PR today

[s77rt]

Not overdue. Waiting on the PR

[suneox]

Hi @s77rt PR is ready for review

[melvin-bot[bot]]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot[bot]]

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.8-6 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

• https://github.com/Expensify/App/pull/44932

If no regressions arise, payment will be issued on 2024-07-25. :confetti_ball:

For reference, here are some details about the assignees on this issue:

• @suneox requires payment automatic offer (Contributor)
• @s77rt requires payment through NewDot Manual Requests