[$1000] mWeb/Safari - The modal remains open for a few seconds before closing after returning from a chat

[izarutskaya]

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

---

Action Performed:

1. Log in to ND.
2. Go to any chat.
3. Click on "Actions."
4. Use the device's physical back button to navigate back

Expected Result:

The modal should close before exiting the chat.

Actual Result:

It remains open even after going back to the LHN.

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: v1.3.55-7

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

Notes/Photos/Videos: Any additional supporting documentation

Rpreplay.Final1691677941.mp4

2023.08.21.18.16.Img.0462.mp4

Expensify/Expensify Issue URL:

Issue reported by: @aman-atg

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1691678959149429

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018a166524d05bfa35
• Upwork Job ID: 1694001762412531712
• Last Price Increase: 2023-10-03

[melvin-bot]

Triggered auto assignment to @NicMendonca (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

[akinwale]

Proposal

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

Menu modal remains open for a few seconds after navigating back from a chat report using the back button.

What is the root cause of that problem?

The modal menu visibility state value remains true at the point where navigation away from the report starts. The menu eventually gets unmounted only after the LHN is displayed.

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

We can use the window popstate event to check if the browser is about to navigate away from the report. Since this only occurs in mWeb, we can conditionally add / remove the event listener.

Solution 1
Based on comments on where the fix should be applied, we can make the following updates in the PopoverMenu component.

1. Add a useState hook to track whether or not the popover is visible.

const [isVisible, setIsVisible] = useState(props.isVisible);

2. Add a useEffect hook to update the visible state whenever props.isVisible changes.

useEffect(() => {
    setIsVisible(props.isVisible);
}, [props.isVisible]);

3. Add a useEffect hook to add / remove the popstate event listener.

useEffect(() => {
    if (Browser.isMobileChrome() || Browser.isMobileSafari()) {
        const popAction = () => {
            if (isVisible) {
                setIsVisible(false);
            }
            if (props.onClose) {
                props.onClose();
            }
        }

        addEventListener('popstate', popAction);
        return () => {
            removeEventListener('popstate', popAction);
        }
    }
}, []);

4. Update the useArrowKeyFocusManager and useKeyboardShortcut hooks to use the isVisible state instead of props.isVisible.

Solution 2
When the browser is about to navigate away from the report, we can set the menu visibility flag to false before navigation completes. In order to achieve this, we can add a useEffect hook to the ReportActionCompose component.

useEffect(() => {
    if (Browser.isMobileChrome() || Browser.isMobileSafari()) {
        const popAction = () => setMenuVisibility(false);

        addEventListener('popstate', popAction);
        return () => {
            removeEventListener('popstate', popAction);
        }
    }
}, []);

What alternative solutions did you explore? (Optional)

None.

25598-demo.mp4

[NicMendonca]

reproducible 👍

RPReplay_Final1692716304.MP4

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~018a166524d05bfa35

[melvin-bot]

Current assignee @NicMendonca is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[akinwale]

@abdulrahuman5196 When you get a chance to review, please check out my proposal. Thanks.

[NicMendonca]

@abdulrahuman5196 bump please!

[uafrontender]

Proposal

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

Menu modal remains open for a few seconds after navigating back from a chat report using the back button. Also I found that emojipicker stays opened forever after navigating back.

What is the root cause of that problem?

Modals should be hidden before navigating back.

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

Except described bug I found that emojipicker stays opened forever after navigating back and user has to close it manually. Also in IOU report "Delete request" modal remains opened for a few seconds after navigating back from a chat report using the back button.

For composer related modals I propose to add navigation listener for beforeRemove event something like this:

useEffect(() => {
        const hideModalsBeforeGoBack = navigation.addListener('beforeRemove', (e) => {
            if (!isMenuVisible && !EmojiPickerAction.emojiPickerRef.current.isEmojiPickerVisible) {
                return;
            }
            e.preventDefault();
            if (EmojiPickerAction.emojiPickerRef.current.isEmojiPickerVisible) EmojiPickerAction.hideEmojiPicker();
            if (isMenuVisible) setMenuVisibility(false);
            window.history.forward();
        });
        return () => {
            hideModalsBeforeGoBack();
        };
    }, [isMenuVisible, navigation]);

I think we should prevent go to previous screen at once, 1st - close modal, 2nd click - go back. It works the same way in native apps so we should keep consistency. Also I used window.history.forward(); to prevent changing address url. I tested this code, works fine. I will think on optimization if my proposal will be selected.

For IOU report "Delete request" modal, I can apply the same logic for isPopupMenuVisible variable in ThreeDotsMenu component.

What alternative solutions did you explore? (Optional)

We can avoid preventing go to previous screen and don't keep consistency between web version and native apps.

[melvin-bot]

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

[abdulrahuman5196]

Apologies for the delay. Will check and get back soon.

[melvin-bot]

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

[NicMendonca]

@abdulrahuman5196 bump on reviewing these proposals ^^

[melvin-bot]

@NicMendonca @abdulrahuman5196 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!

[abdulrahuman5196]

Sorry for the delay. Will check in my morning.

[melvin-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]

@NicMendonca @abdulrahuman5196 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]

@NicMendonca, @abdulrahuman5196 Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

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

[NicMendonca]

@aimane-chnaif do you have bandwidth to pick up this one? 🙏

[aimane-chnaif]

yes, I will update this by tomorrow

[melvin-bot]

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

[abdulrahuman5196]

Hi, @NicMendonca @aimane-chnaif Sorry for the delay. I am back from OOO.

I did initially review the proposals and provided comments. I can take this over again if @aimane-chnaif is fine.

[aimane-chnaif]

Seems like this was fixed in #27452 as same root cause and not reproducible anymore.
@aman-atg can you please confirm as a bug reporter?

[aimane-chnaif]

@abdulrahuman5196 yes, you can take over again if it's still reproducible and will not be closed

[abdulrahuman5196]

Sure @aimane-chnaif

[aman-atg]

I'm still able to repro it in staging however now it stays open for a few ms

safari-bug.mp4

[aman-atg]

cc: @abdulrahuman5196 @aimane-chnaif

[aimane-chnaif]

That should not be bug at all, or worth fixing.

[aimane-chnaif]

@akinwale's proposal made sense but we applied that solution in #27452.

@NicMendonca let's close if you agree

[melvin-bot]

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

[abdulrahuman5196]

@NicMendonca The issue is not reproducible now. We can close this issue.