App icon indicating copy to clipboard operation
App copied to clipboard

[$500] Menu - Pin & Delete request appear on Receipt image when quickly tap on receipt and 3-Dot menu

Open izarutskaya opened this issue 2 years ago β€’ 34 comments

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.16-3 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. Navigate to New dot app as Employee
  2. Go to workspace chat> create Scan request
  3. On detail page tap receipt and 3-Dot menu in a quick manner

Expected Result:

The receipt should be open and no 3-Dot menu should be open

Actual Result:

3-Dot menu (Pin & Delete request) from detail page appear on Receipt image when quickly tap on receipt and 3-Dot menu

Workaround:

Unknown

Platforms:

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

  • [x] Android: Native
  • [ ] Android: mWeb Chrome
  • [ ] iOS: Native
  • [ ] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [ ] MacOS: Desktop

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/75e058b7-68dd-4ab6-94e7-bc978c6122ea

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01ea7a6ca457ab8814
  • Upwork Job ID: 1738519644108128256
  • Last Price Increase: 2024-01-06
  • Automatic offers:
    • c3024 | Contributor | 28106449

izarutskaya avatar Dec 23 '23 11:12 izarutskaya

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

melvin-bot[bot] avatar Dec 23 '23 11:12 melvin-bot[bot]

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

melvin-bot[bot] avatar Dec 23 '23 11:12 melvin-bot[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[bot] avatar Dec 23 '23 11:12 melvin-bot[bot]

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

melvin-bot[bot] avatar Dec 23 '23 11:12 melvin-bot[bot]

Proposal

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

Menu - Pin & Delete request appear on Receipt image when quickly tap on receipt and 3-Dot menu

What is the root cause of that problem?

The root cause is that the Receipt image view and 3-dot menu are modals. The modals always open on top of screen in react native and the last opened modal will be on top

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

I think would be better if we use one component for displaying any modals in root of project . But in this case, we can fix the problem with the following steps.

  1. Add call willAlertModalBecomeVisible(true) in this function
  2. Add this call willAlertModalBecomeVisible(false); in this function
  3. Use getModalState function or wrap ThreeDotsMenu component in withOnyx to access to modal state in component
export default withOnyx({
    modal: {
        key: ONYXKEYS.MODAL,
    },
})(ThreeDotsMenu);
  1. Change this function like below
const showPopoverMenu = () => {
        if (!modal.willAlertModalBecomeVisible) {
            setPopupMenuVisible(true);
        }
};

What alternative solutions did you explore? (Optional)

We can add another key in Onyx.modal like willAlertModalBecomeVisible and use it to avoid using the key which seems to be for modal alert only

shahinyan11 avatar Dec 23 '23 15:12 shahinyan11

Proposal

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

The three-dot menu shows on the receipt image attachment page.

What is the root cause of that problem?

This issue happens because of a delay. If the three dots menu shows after we are navigated to the receipt image attachment page, then we will see the menu on that page. We have faced a similar issue before here. Basically the same root cause.

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

The solution also will be the same as https://github.com/Expensify/App/issues/29379, we just need to apply it to the three-dot menu.

First, close the menu when the screen blurs.

useFocusEffect(
    useCallback(() => {
        return () => hidePopoverMenu();
    }, []),
);

Next, pass the focused state to the popover isVisible prop.

const isFocused = useIsFocused();
isVisible={isPopupMenuVisible && isFocused}

Optionally, prevent the press when the screen is not focused.

onPress={() => {
    if (!isFocused) return;
    ....
})

(it's optional because so far, we already have a singleExecution apply to BaseGenericPressable which disables the pressable when an interaction is happening, including the navigation transition)

(this issue can happen on every usage of modal, to fix it globally, we can apply the fix to BaseModal, we will call onClose when the screen blurs)

bernhardoj avatar Dec 23 '23 15:12 bernhardoj

Proposal

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

3-Dot menu (Pin & Delete request) from detail page appear on Receipt image when quickly tap on receipt and 3-Dot menu

What is the root cause of that problem?

If we click receipt image and 3 dot menu quickly, the receipt image will be opened and then 3 dot menu will be opened on top of it, and currently we don't have any logic to prevent 3 dot menu that are behind modal from being opened on top of the modal.

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

We should use similar approach as this PR that fixes a similar issue. Basically if the three dots menu is behind a modal, its popover should not be visible.

  1. Connect to ONYXKEYS.MODAL here
export default withOnyx({
    modal: {
        key: ONYXKEYS.MODAL,
    },
})(ThreeDotsMenu);
  1. If the three dots menu is behind a modal, hide the popover if it's currently opened (We need to check !shouldOverlay below because if shouldOverlay is true, that means the current three dots itself is inside a modal: AttachmentModal)
const isBehindModal = modal.willAlertModalBecomeVisible && !shouldOverlay;

useEffect(() => {
    if (isBehindModal && isPopupMenuVisible) {
        hidePopoverMenu();
    }
}, [isBehindModal, isPopupMenuVisible]);
  1. Add isBehindModal to the visible condition as well
isVisible={isPopupMenuVisible && !isBehindModal}
  1. We can optionally add || isBehindModal to the disabled condition here

This is basically the same approach already used in this PR for another component, just that in here we relies on the modal visibility rather than screen focus. This is a global fix that makes sure no such issue like this will occur again for the three dots menu, and rely on existing values/lifecycle of willAlertModalBecomeVisible rather than manipulating its lifecycle to workaround the race conditions as suggested in the above proposal, which unfortunately still doesn't work if pressing the receipt image and 3 dot very quickly.

What alternative solutions did you explore? (Optional)

The shouldOverlay name is a bit vague and can be considered renaming to isInModal for clarity.

We can consider relying on modal.isVisible (or both willAlertModalBecomeVisible and isVisible)

tienifr avatar Dec 23 '23 16:12 tienifr

I find this extremely difficult to reproduce on my device and emulator. I am not able to quickly press the three dot menu after pressing on the receipt. Any tips to consistently reproduce this?

c3024 avatar Dec 25 '23 06:12 c3024

I find this extremely difficult to reproduce on my device and emulator. I am not able to quickly press the three dot menu after pressing on the receipt. Any tips to consistently reproduce this?

@c3024 I run on my physical Android device and press the receipt with 1 finger and immediately press the three dot using another finger and can reproduce it every time, hope that helps πŸ‘

tienifr avatar Dec 25 '23 06:12 tienifr

Proposal

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

Menu - Pin & Delete request appear on Receipt image when quickly tap on receipt and 3-Dot menu

What is the root cause of that problem?

When the user clicks on 3-Dot menu, the menu will be open while the menu is rendering if a user quickly clicks on an attachment the Modal for attachment is also triggered to open, and this issue always happens when device performance is slow. We can perform by using scripting or putting a device to low-performance mode

  let menu = document.querySelectorAll('button[aria-label="More"]')[0];
  let attachment = document.querySelectorAll('[aria-label="View attachment"]')[0];
  menu?.click();
  attachment?.click()
Reproduce

https://github.com/Expensify/App/assets/11959869/ee0483dd-75f4-4175-9d62-b3edcb6e1753

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

Due to this issue can happen anywhere in the app so we need to handle it generally. We will create ThreeDotsMenuManager look like ReportActionComposeFocusManager has function isMenuOpen, openMenu, closeMenu When showModal we will call ThreeDotsMenuManager.closeMenu() if we have any ThreeDotsMenu has opened state

    const showModal = () => {
+       ThreeDotsMenuManager.closeMenu()
        ...
        onModalShow?.();
    };

What alternative solutions did you explore? (Optional)

Instead of create ThreeDotsMenuManager we can create PopoverManager

suneox avatar Dec 25 '23 14:12 suneox

Reviewing

c3024 avatar Dec 28 '23 04:12 c3024

πŸ“£ 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] avatar Dec 30 '23 16:12 melvin-bot[bot]

Is there a similar case anywhere else where an undesired three dot menu modal appears over another modal so that a global fix is warranted?

c3024 avatar Dec 31 '23 04:12 c3024

@c3024 I think a global fix should be made around a new feature, not around bug. Because AttachmentModal is not associated with other modals and works separately .

shahinyan11 avatar Jan 01 '24 10:01 shahinyan11

looks like we're reviewing this, @c3024 let me know if you need any help from me!

Christinadobrzyn avatar Jan 03 '24 00:01 Christinadobrzyn

Is there a similar case anywhere else where an undesired three dot menu modal appears over another modal so that a global fix is warranted?

@c3024 it happens on the normal chat report as well, when pressing three dots and sent attachment at the same time. I believe it happens basically everywhere an AttachmentModal and three dot menu appears together. So yeah I think we need a global fix

tienifr avatar Jan 05 '24 11:01 tienifr

I have not been able to reproduce this anywhere else. Could you post a video of it?

c3024 avatar Jan 06 '24 03:01 c3024

πŸ“£ 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] avatar Jan 06 '24 16:01 melvin-bot[bot]

@Christinadobrzyn @c3024 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!

melvin-bot[bot] avatar Jan 06 '24 17:01 melvin-bot[bot]

I can't reproduce this either - @tienifr are you able to take a video of what you see?

I'm going ooo next week so going to assign a buddy to monitor this for me

status - we're trying to reproduce this (and probably update the OP with the correct steps)

Christinadobrzyn avatar Jan 08 '24 00:01 Christinadobrzyn

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

melvin-bot[bot] avatar Jan 08 '24 00:01 melvin-bot[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[bot] avatar Jan 08 '24 00:01 melvin-bot[bot]

I have not been able to reproduce this anywhere else. Could you post a video of it?

@c3024 @Christinadobrzyn here you go, same issue with normal Attachment in chat report, I can reproduce every time, make sure to follow this step

https://github.com/Expensify/App/assets/113963320/4b75afd4-f42c-4f73-8893-7413760763c5

tienifr avatar Jan 09 '24 13:01 tienifr

Issue not reproducible during KI retests. (First week)

mvtglobally avatar Jan 11 '24 06:01 mvtglobally

The bug is still reproducible

https://github.com/Expensify/App/assets/46921547/961a682e-8faa-4a15-97e7-2e641fcc5150

shahinyan11 avatar Jan 11 '24 07:01 shahinyan11

Thank you all for your proposals.

willAlertModalBecomeVisible is largely modified in the base components.

So, I think the approach of not showing the popover when the three dot menu is behind a modal is the better way of fixing this than changing the willAlertModalBecomeVisible values in the AttachmentModal. This approach also fixes all possible cases where the three dot menu can be behind any modal.

Proposal here by @tienifr looks good to me.

πŸŽ€ πŸ‘€ πŸŽ€ C+ Reviewed

c3024 avatar Jan 11 '24 15:01 c3024

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

melvin-bot[bot] avatar Jan 11 '24 15:01 melvin-bot[bot]

@c3024 @tienifr Did you try opening only the 3-dot menu ? I faced this problem when I used @tienifr's approach

https://github.com/Expensify/App/assets/46921547/eb2fef43-05df-45e5-8fe9-2c05fa3a1f90

shahinyan11 avatar Jan 11 '24 16:01 shahinyan11

@puneetlath @Christinadobrzyn @c3024 this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] avatar Jan 13 '24 17:01 melvin-bot[bot]

Did you try opening only the 3-dot menu ? I faced this problem when I used @tienifr's approach

@shahinyan11 There'll be a isPopover field accompanying the willAlertModalBecomeVisible, so the isBehindModal won't be true if the modal that's "becoming visible" is the PopoverMenu itself.

I'll handle that during the PR.

tienifr avatar Jan 15 '24 12:01 tienifr