[$500] Web - After using the Enter key in any field, the 'Upload Photo' feature becomes visible on another

[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.2.0
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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Go to staging.new.expensify.com
2. Log in with any account
3. Go to Setting - Profile
4. Use Tab Key and navigate to default avatar
5. Press Enter Key - you will see the upload photo option displayed
6. Use Tab Key and navigate to Status
7. Hit Enter Key

Expected Result:

After using the Enter key in any field, the 'Upload Photo' feature should not be visible on another page

Actual Result:

After using the Enter key in any field, the 'Upload Photo' feature becomes visible on another page

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6287724_1700685497820.Recording__5501.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01faa982f8264324b6
• Upwork Job ID: 1727437775455944704
• Last Price Increase: 2023-11-22

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01faa982f8264324b6

[melvin-bot]

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

[melvin-bot]

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]

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

[zukilover]

Proposal

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

Upload Photo popover is still visible when clicking Enter on other field.

What is the root cause of that problem?

When user presses Enter outside of the PopoverMenu, it simply doing nothing instead of closing it.
See this line:

App/src/components/PopoverMenu/index.js

Lines 68 to 70 in 14abfa1

	if (focusedIndex === -1) {
	return;
	}

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

We should close the popover if user presses Enter outside of the popover.
We can do so by changing the line above to:

            if (focusedIndex === -1) {
                props.onClose();
                return;
            }

What alternative solutions did you explore? (Optional)

N/A

[ikevin127]

I was able to reproduce the issue on Web (Chrome / Safari / Firefox) and Desktop -> all platforms where the user can navigate using keyboard TAB key.

Proposal

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

After opening Avatar's popover menu 'Upload Photo' via keyboard Enter key, the popover remains opened after navigating away from Avatar to other fields via keyboard TAB key, the popover persisting even when navigating to one of the other fields pages like Status or any other.

What is the root cause of that problem?

This is happening because the AvatarWithImagePicker.js is missing logic that should handle the conflict between keyboard navigation using TAB key and the Avatar's popover component open state which is handled by the isMenuVisible state variable.

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

My solution for this is to writa a useEffect with isMenuVisible in the dependency array, where if isMenuVisible is true, I will re-use already written keydown listeners:

• addKeyDownPressListener
• removeKeyDownPressListener

from @libs/KeyboardShortcut/KeyDownPressListener to set isMenuVisible to false when TAB key is pressed again while the popover menu is open.

useEffect - Code implementation

    // Closes the opened popover menu when the user navigates
    // using keyboard TAB key on web / desktop.
    useEffect(() => {
        if (!isMenuVisible) {
            return;
        }

        function handleKeyDown(e) {
            if (e.key !== 'Tab') {
                return;
            }
            setIsMenuVisible(false);
        }

        addKeyDownPressListener(handleKeyDown);
        return () => removeKeyDownPressListener(handleKeyDown);
    }, [isMenuVisible]);

What's different in my proposal is that it closes the popover menu as soon as the TAB key is pressed and the focus is set to the next / previous item from the Avatar (popover origin), instead of closing the popup only when navigating to another section like Status by pressing the Enter key (which looks kinda buggy).

Video of first proposal's solution (for reference)

Screen.Recording.2023-11-23.at.03.44.35.mov

Videos

MacOS: Chrome - Current proposal's solution

Screen.Recording.2023-11-23.at.03.40.07.mov

[dukenv0307]

Proposal

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

After using the Enter key in any field, the 'Upload Photo' feature becomes visible on another page

What is the root cause of that problem?

Normally the PopoverMenu will be closed if there's a click outside, but in case we navigate to another page using tab and then clicking Enter, it will not trigger any click so the PopoverMenu will stay.

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

When we see an Enter event coming when the Popover is visible here, if there's no menu item currently focused, and the anchor is also not focused, we'll close the PopoverMenu

if (focusedIndex === -1) {
    if (props.anchorRef.current !== DomUtils.getActiveElement()) {
        props.onClose();
    }
    return;
}

We can also optionally make sure DomUtils.getActiveElement() is truthy first.

We need to check that the anchor is not currently focused before calling props.onClose because if the anchor is focused, it will also toggle the popover menu and there'll be a regression where you'll be unable to close the popover menu by pressing Enter on the anchor element, because the PopoverMenu visibility will now be toggled twice, once from the anchor and once from the popover menu.

What alternative solutions did you explore? (Optional)

We can add props.isVisible to the condition to make sure the PopoverMenu is visible at that time, although I think it might not be needed because the Enter listener will not be active if props.isVisible is false

[bernhardoj]

Proposal

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

Navigating to another page with keyboard won't close a visible popover.

What is the root cause of that problem?

We don't have a logic to handle such case. What we have is a logic to close the popover when a click happens outside of the menu.

App/src/components/PopoverProvider/index.tsx

Lines 28 to 41 in 4952c39

	React.useEffect(() => {
	const listener = (e: Event) => {
	// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
	if (activePopoverRef.current?.ref?.current?.contains(e.target as Node) || activePopoverRef.current?.anchorRef?.current?.contains(e.target as Node)) {
	return;
	}
	const ref = activePopoverRef.current?.anchorRef;
	closePopover(ref);
	};
	document.addEventListener('click', listener, true);
	return () => {
	document.removeEventListener('click', listener, true);
	};
	}, [closePopover]);

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

We have a logic to close the popover when a click happens outside of the menu, so I think it makes sense if also we add a logic to close the popover when a "click" with an ENTER key happens outside of the menu.

To do that, we can listen to keyup event.

React.useEffect(() => {
    const listener = (e: KeyboardEvent) => {
        if (e.key !== 'Enter') return;
        if (activePopoverRef.current?.ref?.current?.contains(e.target as Node) || activePopoverRef.current?.anchorRef?.current?.contains(e.target as Node)) {
            return;
        }
        const ref = activePopoverRef.current?.anchorRef;
        closePopover(ref);
    };
    document.addEventListener('keyup', listener, true);
    return () => {
        document.removeEventListener('keyup', listener, true);
    };
}, [closePopover]);

The ENTER "click" is only triggered when we lift the key, so we use keyup here which will be consistent with click

What alternative solutions did you explore? (Optional)

Before having our own popover code, we use a modal to show the popover. A modal in rn-web by default has a focus trap, so it's impossible to use a TAB key to "click" other button/pressable outside of the menu with an ENTER key.

If we add a focus trap to our popover, then this issue won't happen.

the recent focus trap PR is reverted, so if we want to use this solution, we should wait for the new PR

[zanyrenney]

@Santhosh-Sellavel please can you review the propsoals?

[Santhosh-Sellavel]

1. @dukenv0307's proposal a good solution!

2. @zukilover proposal causes a regression explained in @dukenv0307's proposal.

3. @ikevin127 it's a bad idea to close pop on pressing TAB.

4. @bernhardoj's proposal seems like a good solution too!

I'll let CME make a final decision @bernhardoj's seems like a better solution between the two (1 & 4)

C+ Reviewed
🎀 👀 🎀

[melvin-bot]

Triggered auto assignment to @thienlnam, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[thienlnam]

There was another PR I came across recently that addresses a similar issue with focus trapping in the RHP #27670

It had to get reverted but it seems like it might also resolve this issue - do you agree @Santhosh-Sellavel? If so I'll put this on hold for that issue

[dukenv0307]

I'll let CME make a final decision @bernhardoj's seems like a better solution between the two (1 & 4)

@Santhosh-Sellavel can you explain more on why you think the 4th solution is better?

It adds another listener which is overhead, while my proposal uses an exising listener. It’s also more complex and doesn’t use the recommended method of listening to key events.

[0xmiros]

@dukenv0307 no idea why I was tagged here

[dukenv0307]

@0xmiroslav sorry I meant to tag @Santhosh-Sellavel 🙏 updated comment

[melvin-bot]

@thienlnam, @zanyrenney, @Santhosh-Sellavel Whoops! This issue is 2 days overdue. Let's get this updated quick!

[zanyrenney]

bump @Santhosh-Sellavel what are your thoughts on what @thienlnam said above here

[Santhosh-Sellavel]

@thienlnam Sorry I didn't post a reply earlier, yes we can hold this for that as it seems it might resolve this one too!

[zanyrenney]

thanks for the update @thienlnam

[zanyrenney]

please can you link the other issue that we are holding this on though?

[thienlnam]

#15631

[thienlnam]

Still on hold

[Santhosh-Sellavel]

I'm unavailable next week, Please assign a new C+ Issue here if required while I am away, thanks!

cc: @zanyrenney

[Santhosh-Sellavel]

Back now

[zanyrenney]

No worries @Santhosh-Sellavel

[zanyrenney]

Stillon HOLD.

[zanyrenney]

@thienlnam I have been sifting through the projects but I am actually not 100% sure which one this would fit in. DO you have any ideas? Would appreciate a second opinion! I was thinking maybe performance?

[thienlnam]

I would probably categorize under vip-vsb as part of general improvements related to the individual user experience

[zanyrenney]

okay fab, thank you for taking a look!

[zanyrenney]

I believe this is still on hold @thienlnam let me know if you disagree.

[thienlnam]

Holding PR is complete, we should retest to see if this is still reproducible

[Santhosh-Sellavel]

Still Occurs here

[bernhardoj]

The previous focus trap PR was closed, so it's not completed yet. Here is the new focus trap PR.

[bernhardoj]

The focus trap PR is done. I can't repro it anymore.

cc: @Santhosh-Sellavel

[thienlnam]

Great, thanks - going to close