[$250] Workspace switcher - Workspace switcher reopens after selecting a workspace with keyboard

[lanitochka17]

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

---

Version Number: .1.4-36.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: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

Precondition:

• User has less than 7 workspaces
• Workspace filter is "Expensify"

1. Go to staging.new.expensify.com
2. Open workspace switcher
3. Click on any workspace
4. Open workspace switcher again
5. Using keyboard arrow key, select another workspace and hit Enter

Expected Result:

The workspace switcher will not reopen

Actual Result:

The workspace switcher reopens

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/0f90ec75-d17d-4850-b413-86aa97ca8152

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~012b6b390114d02e71
• Upwork Job ID: 1754525872518934528
• Last Price Increase: 2024-02-26

[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.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~012b6b390114d02e71

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Upwork job price has been updated to $250

[melvin-bot[bot]]

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

[mountiny]

Not a blocker, making external

[godofoutcasts94]

I want to work on it

[lanitochka17]

We think that this bug might be related to #wave8 CC @zanyrenney

[agent3bood]

Proposal

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

Workspace switcher - Workspace switcher reopens after selecting a workspace with keyboard

What is the root cause of that problem?

The way we subscribe to 'enter' event has the wrong assumption about how to use KeyboardShortcut.subscribe. The current implementation uses shouldBubble arg to stop propagation, while it should be using the shouldStopPropagation arg.

• shouldBubble is used to stop the next listener for 'enter' key from executing when we have multiple listeners for the same key.
• shouldStopPropagation is used to stop the DOM event propagation.

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

Update how we subscribe to the enter event like so

    subscribeToEnterShortcut() {
        const enterConfig = CONST.KEYBOARD_SHORTCUTS.ENTER;
        this.unsubscribeEnter = KeyboardShortcut.subscribe(
            enterConfig.shortcutKey,
            this.selectFocusedOption,
            enterConfig.descriptionKey,
            enterConfig.modifiers,
            true,
            false,
            0,
            true,
            [],
            () => !this.state.allOptions[this.state.focusedIndex],
        );
    }

What alternative solutions did you explore? (Optional)

[melvin-bot[bot]]

📣 @agent3bood! 📣 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:

1. Make sure you've read and understood the contributing guidelines.
2. 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.
3. 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.
4. 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>

[agent3bood]

Contributor details Your Expensify account email: [email protected] Upwork Profile Link: https://www.upwork.com/freelancers/~01d5fb72b5d46a967a

[melvin-bot[bot]]

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

[muas19]

Proposal

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

Workspace switcher reopens after selecting a workspace with keyboard.

What is the root cause of that problem?

Whenever there are less than 8 items (workspaces)on the list, the focus remains on the workspace switcher button on the top left button. That is, it becomes the active element in the DOM. Now when we press Enter, it gets called again, that's why it reopens.

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

Remove focus from it

https://github.com/Expensify/App/blob/a1defcfa4dc708f4d4693484f6f65dbcf5044ab9/src/components/WorkspaceSwitcherButton.tsx#L47-L50

To become :

onPress={() =>{
    if (DomUtils.getActiveElement()) {
        (DomUtils.getActiveElement() as HTMLElement | null)?.blur();
    }
    interceptAnonymousUser(() => {
        Navigation.navigate(ROUTES.WORKSPACE_SWITCHER);                        
    })
}}


Video:
[outside.webm](https://github.com/Expensify/App/assets/3404159/4f44deb3-7054-49c0-b1e8-d9278d96dc82)

[sakluger]

@situchan it looks like there are a few proposals. Do any of them look good to you?

[situchan]

I am not able to reproduce on wrench tab

https://github.com/Expensify/App/assets/108292595/375446bd-f880-4049-8ebf-a7782a154498

but reproducible on chats tab

https://github.com/Expensify/App/assets/108292595/8778b6ea-ab9b-422f-a5a4-584f8baf0521

[situchan]

@muas19 it's intentional to hide text input when <8 so 👎 to your proposal. @agent3bood please explain your root cause in more detail. Especially why https://github.com/Expensify/App/issues/35823#issuecomment-1938914642.

[agent3bood]

@situchan Explaining root cause: The BaseOptionsSelector has two paths 1. when shouldShowTextInput == true the code will request focus on the input, removing focus from any previously focused node. 2. when shouldShowTextInput != true no focus is requested and whatever node was focused before opening the page will still has focus, in this case it is WorkspaceSwitcherButton.

When the user uses keyboard to select new workspace it will be handled by a global listener BaseOptionsSelector.subscribeToEnterShortcut, this listener needs to stop the event from propagating after handling it.

But if the user used mouse click to select new workspace, the event will be handled by the div that shows the workspace and cannot propagate to the WorkspaceSwitcherButton.

[mountiny]

Still waiting for proposals

[situchan]

@agent3bood thanks. The root cause makes sense but still not follow your solution. We should not easily stop propagation in general component.

[agent3bood]

@situchan I think this is the most practical solution, there are other places the same approach is used includes the general hook useKeyboardShortcut does have shouldPreventDefault and shouldStopPropagation props.

The other option would be requesting focus on the selected list item, this is more involved as it require passing refs with each item from the most top component.

[muas19]

https://github.com/Expensify/App/issues/35823#issuecomment-1930912196 Updated proposal @situchan

[muas19]

I would also like to report a bug that I found out after investigation. Where can I do this and propose another fix?

[mountiny]

@situchan no good proposals so far?

[melvin-bot[bot]]

@sakluger @mountiny @situchan 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!

[situchan]

No viable proposal yet

The ideal solution would be to lose focus on switcher button when switcher modal is open

[muas19]

@situchan My updated proposal has this implementation. Could you recheck? https://github.com/Expensify/App/issues/35823#issuecomment-1930912196

[situchan]

@muas19 I remember similar issues (with the same root cause) happened in the past. Can you find the code in our codebase which applied your solution?

[agent3bood]

Proposal

Updated

[muas19]

https://github.com/Expensify/App/issues/35823#issuecomment-1930912196 Proposal updated after C+ feedback https://github.com/Expensify/App/issues/35823#issuecomment-1952685403 Inspiration from this pull https://github.com/Expensify/App/pull/9093 @situchan