Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2024-08-07] [HOLD] [$1000] ReportActionContextMenu not closing properly #23959

Closed
1 of 6 tasks
kavimuru opened this issue Jul 31, 2023 · 123 comments
Closed
1 of 6 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@kavimuru
Copy link

kavimuru commented Jul 31, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Open ND
  2. Hover on any chat for report action context menu
  3. Press Add reaction
  4. Now press CTRL+K
  5. Press the back arrow to close the search (RHP)
  6. Now check the report action context menu after closing modal

Expected Result:

Report action context menu should be hidden, and be shown only on hovering

Actual Result:

Report action context menu gets stuck

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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: 1.3.48-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
Notes/Photos/Videos: Any additional supporting documentation

Screen.Recording.2023-07-30.at.6.53.30.PM.mov
Recording.1415.mp4

Expensify/Expensify Issue URL:
Issue reported by: @shubham1206agra
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690723766793679

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~013c8f7f084556466f
  • Upwork Job ID: 1687252626772779008
  • Last Price Increase: 2024-07-07
  • Automatic offers:
    • bernhardoj | Contributor | 26013711
    • shubham1206agra | Reporter | 26013714
Issue OwnerCurrent Issue Owner: @adelekennedy / @adelekennedy
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 31, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 31, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 31, 2023

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

@vadymbokatov
Copy link
Contributor

vadymbokatov commented Jul 31, 2023

Proposal

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

ReportActionContextmenu not closing properly when mini emoji picker is opening.

What is the root cause of that problem?

When press openpicker button on mini report action context menu, it keeps context menu to open.

if (isMini) {
return (
<MiniQuickEmojiReactions
key="MiniQuickEmojiReactions"
onEmojiSelected={toggleEmojiAndCloseMenu}
onPressOpenPicker={openContextMenu}
onEmojiPickerClosed={closeContextMenu}
reportActionID={reportAction.reportActionID}
reportAction={reportAction}
/>
);
}

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

I think, to close context menu when press openpicker is the best option.

if (isMini) { 
     return ( 
         <MiniQuickEmojiReactions 
             key="MiniQuickEmojiReactions" 
             onEmojiSelected={toggleEmojiAndCloseMenu} 
             onEmojiPickerClosed={closeContextMenu} 
             reportActionID={reportAction.reportActionID} 
             reportAction={reportAction} 
         /> 
     ); 
 }

What alternative solutions did you explore? (Optional)

Result

Screen.Recording.2023-08-01.at.06.47.25.mov

@jfquevedol2198
Copy link
Contributor

jfquevedol2198 commented Jul 31, 2023

Proposal

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

ReportActionContextMenu not closing properly

What is the root cause of that problem?

const hideEmojiPicker = (isNavigating) => {
if (isNavigating) {
onModalHide.current = () => {};
}
emojiPopoverAnchor.current = null;
setIsEmojiPickerVisible(false);
};

In EmojiPicker, hideEmojiPicker action which is fired by onClose of Modal is not calling onModalHide.current if its being closed by navigating. Modal component's onModalHide performs parent element's close action, but when onClose is fired, it doesn't call onModalHide props of Modal component. so parent element (ReportActionContextMenu) is not closed and keeps showing although ReportAction is not hovered because shouldKeepOpen was not reseted by false.

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

If EmojiPicker is being closed by navigating, need to call onModalHide.current before initializing it.

    const hideEmojiPicker = (isNavigating) => {
        if (isNavigating) {
            if (onModalHide.current) onModalHide.current();
            onModalHide.current = () => {};
        }
        emojiPopoverAnchor.current = null;
        setIsEmojiPickerVisible(false);
    };
Result
Screen.Recording.2023-08-01.at.6.49.50.AM.mov

What alternative solutions did you explore? (Optional)

N/A

@bernhardoj
Copy link
Contributor

bernhardoj commented Aug 1, 2023

Proposal

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

The mini context menu does not hide if an emoji picker is open while navigating to another page with a shortcut.

What is the root cause of that problem?

The mini context menu will show when the report action is hovered or shouldKeepOpen state is true.

return (
(this.props.isVisible || this.state.shouldKeepOpen) && (

As soon as we open the emoji picker, the report action will lose its hovered state, and instead, the shouldKeepOpen state becomes true.

onPressOpenPicker={openContextMenu}

openContextMenu: () => this.setState({shouldKeepOpen: true}),

shouldKeepOpen will become false when we close the emoji picker.

onEmojiPickerClosed={closeContextMenu}

renderContent: (closePopover, {reportID, reportAction, close: closeManually, openContextMenu}) => {
const isMini = !closePopover;
const closeContextMenu = (onHideCallback) => {
if (isMini) {
closeManually();

close: () => this.setState({shouldKeepOpen: false}),

The emoji picker relies on the modal onModalHide callback to notify that the emoji picker is closed.

onModalHide={onModalHide.current}

const showEmojiPicker = (onModalHideValue, onEmojiSelectedValue, emojiPopoverAnchorValue, anchorOrigin, onWillShow = () => {}, reportActionValue) => {
onModalHide.current = onModalHideValue;

However, when we use a shortcut to go to other pages, we will close any modal that is visible, wait for it to close, and do the navigation. On closing the picker, we check if isNavigating is true, and if true, we clear the onModalHide callback.

const hideEmojiPicker = (isNavigating) => {
if (isNavigating) {
onModalHide.current = () => {};
}

isNavigating is true when we are navigating to other pages. This is useful to prevent the composer picker modal hide callback that will refocus the composer.

<EmojiPickerButton
isDisabled={isBlockedFromConcierge || this.props.disabled}
onModalHide={() => {
this.focus(true);
}}

Because the hide callback is cleared, we have no way to notify the mini context menu to set the shouldKeepOpen back to false.

Even though we fixed the above issue, we have another recent issue introduced by shouldFreezeCapture. If a modal/popover is visible, the Hoverable won't respond to any hover event, so the hover state is stuck.

<Hoverable
shouldFreezeCapture={modal?.willAlertModalBecomeVisible}

It's added so the action is highlighted while a report preview payment method popover is shown (confirmed here).

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

We shouldn't clear the modal hide callback because there are cases where we need to have the hide callback. Instead, we can pass the isNavigating value to the onModalHide callback and let each component that uses it decide what to do based on isNavigating value.

For example, on ReportActionCompose (and ReportActionItemMessageEdit), we will only focus back on the composer if isNavigating is false.

onModalHide={() => {
this.focus(true);
}}

onModalHide={(isNavigating) => {
    if (!isNavigating) this.focus(true);
}}

To fix the 2nd issue, we need to remove shouldFreezeCapture from the action Hoverable.

<Hoverable
shouldFreezeCapture={modal?.willAlertModalBecomeVisible}

Now, to highlight the action while report preview payment method popover is visible, we need to create a new state (called isPaymentMethodPopoverActive) and pass it here.

<View
style={StyleUtils.getReportActionItemStyle(
hovered || isWhisper || isContextMenuActive || !!isEmojiPickerActive || draftMessage !== undefined,

We set the state to true when the popover is shown and set to false when the popover is closed.

What alternative solutions did you explore? (Optional)

Alternative 1:
Pass a new parameter to showEmojiPicker called forceHideCallback. If forceHideCallback is true, we won't clear the hide callback.

Alternative 2:
This alternative requires the most changes, but I think the best. As explained in my root cause, isNavigating is useful to prevent the composer picker modal hide callback that will refocus the composer after navigating. The real reason isNavigating is added is that we put a delay before focusing the composer after the emoji picker is closed.

InteractionManager.runAfterInteractions(() => {
if (!this.textInput) {
return;
}
if (!shouldelay) {
this.textInput.focus();
} else {
// Keyboard is not opened after Emoji Picker is closed
// SetTimeout is used as a workaround
// https://github.com/react-native-modal/react-native-modal/issues/114
// We carefully choose a delay. 100ms is found enough for keyboard to open.
setTimeout(() => this.textInput.focus(), 100);
}
});

This means, without isNavigating, we will navigate to the search page and after some delay, we will focus the main composer.

The focus delay is added as a workaround for the modal issue where onModalHide is called too early (this is a well-known issue). If we remove this workaround, we don't need to have the isNavigating logic anymore. So, here is the plan:

  1. Fix the onModalHide callback is called too early. This fix requires an upstream (react-native-modal) fix.
    a. We should call onModalHide in Modal onDismiss callback. Will open PR after both below are fixed.
    b. onDismiss callback is not available in Android, but I have an open PR for it.
    c. onDismiss is called too early on rn-web, but I have an open PR for it.
  2. Remove the focus delay (InteractionManager + setTimeout) workaround from composer
  3. Remove all usage of isNavigating workaround.

@adelekennedy
Copy link

Trying to reproduce on chrome staging (v1.3.49-1) or prod (v1.3.48-5) and I'm not getting the same result - the ra context menu doesn't reappear. I think this may have been fixed. checking in Slack

@jfquevedol2198
Copy link
Contributor

jfquevedol2198 commented Aug 2, 2023

@adelekennedy I can see this on New Dot and Staging.
image

@adelekennedy
Copy link

moving this forward!

@adelekennedy adelekennedy added the External Added to denote the issue can be worked on by a contributor label Aug 4, 2023
@melvin-bot melvin-bot bot changed the title ReportActionContextMenu not closing properly [$1000] ReportActionContextMenu not closing properly Aug 4, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 4, 2023

Job added to Upwork: https://www.upwork.com/jobs/~013c8f7f084556466f

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 4, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 4, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 4, 2023

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

@studentofcoding
Copy link
Contributor

Proposal

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

ReportActionContextMenu not closing properly

What is the root cause of that problem?

This hideEmojiPicker function not covering the case when what to do when being overlapped by Navigating, therefore this condition should be added.

/**
* Hide the emoji picker menu.
*
* @param {Boolean} isNavigating
*/
const hideEmojiPicker = (isNavigating) => {
if (isNavigating) {
onModalHide.current = () => {};
}
emojiPopoverAnchor.current = null;
setIsEmojiPickerVisible(false);
};

Lines 71 to 77 in 024d210

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

Need to check the status of onModulaHide.current and init that.

const hideEmojiPicker = (isNavigating) => {
        if (isNavigating) {
            if (onModalHide.current) onModalHide.current(); // This is the condition to handle the case
            onModalHide.current = () => {};
        }
        emojiPopoverAnchor.current = null;
        setIsEmojiPickerVisible(false);
    };

What alternative solutions did you explore? (Optional)

N/A

Result

https://www.loom.com/share/6f506f43b8e04c57ae5da5a979c1df2e?sid=034af9be-bf27-490c-81f6-4a19184fa3c1

@nileshahir286
Copy link

Contributor details
Your Expensify account email: nileshahir286@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~0126f3665fe23ff297

Proposal

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

Report Context Menu should be closed, now it's stuck after close the modal.

What is the root cause of that problem?

Report Context Menu should only be show when hover on icon.

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

When you press CTRL+K, at that time the context menu should hide.

@melvin-bot
Copy link

melvin-bot bot commented Aug 4, 2023

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

@nileshahir286
Copy link

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

Already include Upwork profile as well as Expensify email id.

@bernhardoj
Copy link
Contributor

Updated my proposal to include more alternative solutions.

@melvin-bot melvin-bot bot added the Overdue label Aug 7, 2023
@adelekennedy
Copy link

pending review on the updated proposal

@melvin-bot melvin-bot bot removed the Overdue label Aug 7, 2023
@robertKozik
Copy link
Contributor

I'll review it as the first thing tomorrow 👌🏼

@robertKozik
Copy link
Contributor

Hi, thank you all for your proposals. I'll try to address them all:

  1. @studentofcoding / @jfquevedol2198 - both of your proposals suggest triggering the onModalHide function before clearing it. I think it could cause lots of regression problems. Basically, this trigger renders the entire if statement irrelevant in my eyes.
    if (isNavigating) {
    onModalHide.current = () => {};
    }
  2. @nileshahir286 - I think your proposal is too general to be taken under consideration.
  3. @bernhardoj - I think your proposal is well-suited to be chosen. I really like the idea of passing the isNavigating state to decide on the behavior of onHide using the callback function. I believe it's the most flexible solution and can be future-proof. I agree with you that your second alternative solution would be the go-to, as it could allow us to eliminate the need for delaying focus. However, it appears that we might have to wait a considerable time until your PRs are merged upstream, and then the versions of these libraries would have to be updated within this repository as well. In my opinion, we should address this bug more promptly, but let's see what CME will think about it.

With that said, I believe we should choose the @bernhardoj proposal and push it forward.

Selected Proposal: Proposal
Author of proposal: @bernhardoj

@robertKozik
Copy link
Contributor

forgot the secret formula 🥲

🎀 👀 🎀 C+ reviewed

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jul 23, 2024
@bernhardoj
Copy link
Contributor

PR is ready

@shubham1206agra bump on the above question so I can adjust the code if needed. Thanks!

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jul 31, 2024
@melvin-bot melvin-bot bot changed the title [HOLD] [$1000] ReportActionContextMenu not closing properly [HOLD for payment 2024-08-07] [HOLD] [$1000] ReportActionContextMenu not closing properly Jul 31, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jul 31, 2024
Copy link

melvin-bot bot commented Jul 31, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Jul 31, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.14-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-08-07. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Jul 31, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@shubham1206agra] The PR that introduced the bug has been identified. Link to the PR:
  • [@shubham1206agra] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@shubham1206agra] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@shubham1206agra] Determine if we should create a regression test for this bug.
  • [@shubham1206agra] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@adelekennedy] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@bernhardoj
Copy link
Contributor

I'll request in ND once payment is due.

@shubham1206agra
Copy link
Contributor

I'll request in ND once payment is due.

@bernhardoj Just informing you. You can only take payment from Upwork for this job as offer was made before 18 months deadline.

@bernhardoj
Copy link
Contributor

Oh no, I already ended the UW contract.

@shubham1206agra
Copy link
Contributor

@mallenexpensify knows about this.

@shubham1206agra
Copy link
Contributor

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

@shubham1206agra
Copy link
Contributor

@adelekennedy Can you issue payment here?

I belive the payment will be as follows:
@shubham1206agra - $1000 + $250 (reporting bonus as this was active when the issue created)
@bernhardoj - $1000

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Aug 7, 2024
@adelekennedy
Copy link

Can do - all of this has to be paid through UW correct? @shubham1206agra @bernhardoj?

@shubham1206agra
Copy link
Contributor

Yes

@adelekennedy
Copy link

@shubham1206agra I also had to send you a new offer (I ended the $250 automatic offer without adding the additional 1k)

@shubham1206agra
Copy link
Contributor

@adelekennedy Accepted the new offer.

@bernhardoj
Copy link
Contributor

@adelekennedy accepted the new offer

@adelekennedy
Copy link

Payments made!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
No open projects
Archived in project
Development

No branches or pull requests