lanitochka17 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: 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:

Go to staging.new.expensify.com
Log in with any account
Go to Setting - Profile
Use Tab Key and navigate to default avatar
Press Enter Key - you will see the upload photo option displayed
Use Tab Key and navigate to Status
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
[x] Windows: Chrome
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/2ba52606-5f34-4d9a-bec3-5e2e45457ba7

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[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

Triggered auto assignment to @zanyrenney (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 - @Santhosh-Sellavel (External)

zukilover commented 10 months ago

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:

https://github.com/Expensify/App/blob/14abfa118d473baeba163f15e6042180892f71cf/src/components/PopoverMenu/index.js#L68-L70

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 commented 10 months ago

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)

https://github.com/Expensify/App/assets/56457735/b02deba8-9702-49e7-a3ce-dec029c834c4

Videos

MacOS: Chrome - Current proposal's solution

https://github.com/Expensify/App/assets/56457735/814a66ab-296a-4a74-be6c-b85d3aed0616

dukenv0307 commented 10 months ago

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 commented 10 months ago

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.

https://github.com/Expensify/App/blob/4952c39a6c2dd69663550bfaf11a29ee3c51943c/src/components/PopoverProvider/index.tsx#L28-L41

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 commented 9 months ago

@Santhosh-Sellavel please can you review the propsoals?

Santhosh-Sellavel commented 9 months ago

@dukenv0307's proposal a good solution!
@zukilover proposal causes a regression explained in @dukenv0307's proposal.
@ikevin127 it's a bad idea to close pop on pressing TAB.
@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[bot] commented 9 months ago

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

thienlnam commented 9 months ago

There was another PR I came across recently that addresses a similar issue with focus trapping in the RHP https://github.com/Expensify/App/pull/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 commented 9 months ago

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 commented 9 months ago

@dukenv0307 no idea why I was tagged here

dukenv0307 commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

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

zanyrenney commented 9 months ago

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

Santhosh-Sellavel commented 9 months ago

@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 commented 9 months ago

cool, so what's next? how can we get this moving more? Is this the latest plan? @thienlnam @Santhosh-Sellavel

thienlnam commented 9 months ago

This is the latest, there's another issue that likely has the same root cause as this issue. We're going to hold it until the fix for that is handled and see if this is still a problem. Now that this is holding on another issue going to move this to a weekly

zanyrenney commented 9 months ago

thanks for the update @thienlnam

zanyrenney commented 9 months ago

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

thienlnam commented 9 months ago

https://github.com/Expensify/App/issues/15631

thienlnam commented 8 months ago

Still on hold

Santhosh-Sellavel commented 7 months ago

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

cc: @zanyrenney

Santhosh-Sellavel commented 7 months ago

Back now

zanyrenney commented 6 months ago

No worries @Santhosh-Sellavel

zanyrenney commented 6 months ago

Stillon HOLD.

zanyrenney commented 5 months ago

@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 commented 5 months ago

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

zanyrenney commented 5 months ago

okay fab, thank you for taking a look!

zanyrenney commented 4 months ago

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

thienlnam commented 4 months ago

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

Santhosh-Sellavel commented 3 months ago

Still Occurs here

bernhardoj commented 3 months ago

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

bernhardoj commented 3 months ago

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

cc: @Santhosh-Sellavel

thienlnam commented 3 months ago

Great, thanks - going to close

Expensify / App

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

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Bug0 Triage Checklist (Main S/O)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Proposal

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

What is the root cause of that problem?

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

Videos

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)