kavimuru commented 1 year 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!

Action Performed:

Go to any report
Right click to open the secondary menu
Right click again elsewhere on the screen

Expected Result

The previous secondary menu should close and replaced with a new one.

Actual Result

Previous secondary menu remains there. A new native secondary menu opens.

Workaround:

unknown

Platforms:

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

[ ] MacOS / Chrome / Safari

Version Number: 1.2.74-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:

https://user-images.githubusercontent.com/43996225/219976173-9e84d64b-6e0c-4c82-a552-82c52d26b85c.mov

https://user-images.githubusercontent.com/43996225/219976225-0f33db8e-301c-4e1f-82f9-34d1f98a9881.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~017f298b5f370cb2ba
Upwork Job ID: 1628420077151154176
Last Price Increase: 2023-05-09

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year 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

zanyrenney commented 1 year ago

Yup, I have had this happen before, so can confirm. adding labels.

MelvinBot commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~017f298b5f370cb2ba

MelvinBot commented 1 year ago

Triggered auto assignment to @greg-schroeder (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

zanyrenney commented 1 year ago

@greg-schroeder I'm going OOO which Is why I think this has assigned you as well of me as Bug0. Are you okay to buddy up on this one whilst I am out?

priyeshshah11 commented 1 year ago

I don't think this is a bug nor it is worth fixing as you're interacting with the native browser right click action. Why would you expect there to be any change in the app in this case? It would have been an issue if the secondary menu wasn't focused & you're not able to click on it but that's not the case here.

Any solutions would be adding extra/unnecessary code to handle this, we should not put in any custom code/hacks to solve such things and let native things work as per it's default behaviour unless it's causing major concerns within the app.

s77rt commented 1 year ago

Proposal

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

Context menu does not close on second right click.

What is the root cause of that problem?

contextmenu event is not being intercepted/handled on PopoverReportActionContextMenu.

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

Listen to contextmenu event on PopoverReportActionContextMenu and in the callback preventDefault and hideContextMenu.

What alternative solutions did you explore? (Optional)

None

aimane-chnaif commented 1 year ago

@dangrous @greg-schroeder do you think this is worth fixing? I don't find any negative impacts with current behavior.

dangrous commented 1 year ago

Yeah I'm not confident I know the answer to this question haha.

Personally, I would say this is a feature. I like always being able to access the right click menu and am annoyed when apps take it over. This seems like the best of both worlds. But, that's just my opinion, and not an official design statement or anything.

@greg-schroeder, how about you?

Maybe we ask the question in Slack and see what others think?

dangrous commented 1 year ago

As an example I just tested Gmail. Gmail has its own right click (if you click on an email) and if you right click elsewhere that moves around with you, and will never open the default right click menu unless you click on something that doesn't have that feature. And either way it still closes the original menu. Having two open at once does feel a bit weird, as I think about it more.

2023-02-26_09-45-28 (1)

I feel like I've interacted with other apps that are the same, so if we want to fit in we might want to do that too, but that's not necessarily a reason to do it either.

aimane-chnaif commented 1 year ago

@dangrous good example! I was actually finding any example popular app which has its own custom context menu. So it makes sense to prevent system context menu when click any area where our own context menu is applied.

allroundexperts commented 1 year ago

@aimane-chnaif @dangrous As another example, please check Google Drive. It also does not show the native context menu once the custom context menu is opened.

allroundexperts commented 1 year ago

Personally, when I'm using Google drive, I often use context menu switching when selecting different files. This is how I found this bug on our app in the first place. I copied one message using our context menu and then when I right clicked on another message (with the intention of copying that), the native context menu opened. Since I was used to doing this on Drive, this bug could be based on my bias only. Just letting you guys know.

allroundexperts commented 1 year ago

Another example from dropbox.

https://user-images.githubusercontent.com/30054992/221430025-84371909-8a36-4f1b-9932-16582765f7b5.mov

I think our implementation of the context menu might not be correct. We are treating our context menu as a modal instead of a popover. The same reason makes it very difficult to 'fix' this bug because if the context menu is open, you can not really hover over any other message (the overlay of modal blocks that). If we use a popover instead (without any background overlay), we'll easily solve this problem. If we are treating the context menu as a modal intentionally, then I think this bug can be ignored citing the same reason.

dangrous commented 1 year ago

Just to consolidate the above conversation as a tldr:

We should fix this bug.

I think what @allroundexperts is saying sounds right too. What would be involved in swapping it to a popover from a modal? Are there other issues that would introduce? This feels like it might be a good solution to prevent weird behavior in the future.

dangrous commented 1 year ago

Waiting for proposals

s77rt commented 1 year ago

Waiting for reviews ^ cc @aimane-chnaif When you get time ^^^

priyeshshah11 commented 1 year ago

Proposal

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

Context menu does not close on second right click.

What is the root cause of that problem?

We do not explicitly intercept the contextmenu in PopoverReportActionContextMenu

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

We should add a listener for contextmenu event in PopoverReportActionContextMenu and prevent the default behaviour & either hide the existing open context menu as if the user normally clicked outside the popover. Or we could leave the context menu open & just prevent the default browser context menu.

Optional solution

We allow the default context menu to open just like how we would allow it everywhere else in the app, but just close our own context menu first by intercepting the contextmenu event as mentioned above.

priyeshshah11 commented 1 year ago

Proposal 2

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

Context menu does not close on second right click.

What is the root cause of that problem?

We do not explicitly intercept the contextmenu in Popovers

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

We should add a listener for contextmenu event in Popover and prevent the default behaviour & either hide the existing open context menu as if the user normally clicked outside the popover. Or we could leave the popover open & just prevent the default browser context menu.

allroundexperts commented 1 year ago

Proposal

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

Native context menu opens when you right click if an existing context menu is opened.

What is the root cause of that problem?

The root cause of this issue is two fold. Firstly, we're not intercepting the context menu event in the Popovers. Secondly, our popovers are essentially modals which add a overlay that cover the whole screen. Even if we intercept the contextMenu event, it will just cause the original menu to close without opening another one at the new position.

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

We should refactor our popover component such that it behaves like a popover in reality. This means that it does not add an overlay to the rest of the screen. With rest of the screen interactive, right clicking on any other message will trigger a new popover menu and the previous popover menu can be closed by intercepting the same event in the previous context menu.

What alternative solutions did you explore? (Optional)

None

zanyrenney commented 1 year ago

woo, a bunch of proposals! Thanks everyone.

zanyrenney commented 1 year ago

@dangrous please can you review and let us know which we'll go ahead with?

dangrous commented 1 year ago

@allroundexperts can you add a bit more detail for how you'd plan to refactor the popover? Seems like it's probably the right direction but would love to hear more of a plan before moving forward.

aimane-chnaif commented 1 year ago

The previous secondary menu should close and replaced with a new one.

@s77rt @priyeshshah11 this should be expected result as per discussion. Other than that, I think it's better to do nothing and close issue than just closing context menu or replacing with system context menu or no actions.

@allroundexperts's proposal: I have same feedback as @dangrous

priyeshshah11 commented 1 year ago

The previous secondary menu should close and replaced with a new one.

@s77rt @priyeshshah11 this should be expected result as per discussion. Other than that, I think it's better to do nothing and close issue than just closing context menu or replacing with system context menu or no actions.

Ohh ok, I had misunderstood the expected behaviour. In that case, yes changing the popover to not be like a modal makes sense. But I don't really think it is really worth doing as that increases the chances of regressions such as having multiple popovers open at the same time such as FAB, EmojiPicker, etc.

It also becomes more of a design question whether you wan't users want to interact with the rest of the app or not while the context menu or any popovers are open.

MelvinBot commented 1 year ago

@dangrous @greg-schroeder @zanyrenney @aimane-chnaif 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!

allroundexperts commented 1 year ago

@dangrous @aimane-chnaif Following on the proposal above, I've created a simple popover component and integrated it in our code. It can be checked here: https://github.com/Expensify/App/commit/2eda49d79f8a8df6134dbc27e66ff776dd8f4b02

For saving time, I did not made a mobile version of it so for now, it will just work on desktop. In the PR phase, we can improve it further.

Here's a video of its working: https://user-images.githubusercontent.com/30054992/223154228-14caeaa2-edcd-43c2-b692-62dd6902495f.mp4

Please let me know if there's anything else I need to put in.

aimane-chnaif commented 1 year ago

@allroundexperts Great work! shortcut keys (i.e. Ctrl+K, Ctrl+I) also dismiss those popovers? Can you fully write down your approach in plain English as an updated proposal?

allroundexperts commented 1 year ago

@allroundexperts Great work! shortcut keys (i.e. Ctrl+K, Ctrl+I) also dismiss those popovers? Can you fully write down your approach in plain English as an updated proposal?

@aimane-chnaif Yes, it works.

allroundexperts commented 1 year ago

Updated Proposal

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

Native context menu opens when you right click if an existing context menu is opened.

What is the root cause of that problem?

Our popovers are essentially modals which add a overlay that cover the whole screen. Even if we intercept the contextMenu event, it will just cause the original menu to close without opening another one at the new position.

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

We need to create new popup component for Web (large screens only) that does not add overlay when the popup is created. The component will use the same props as our current Popover implementation is using. The main challenge in creating such a component is handling the closing of the popover. With the modal based approach that we're using right now, its easy to tell if click was outside the Modal because Modal has an overlay that sits on top of the screen.

With our new component, since there is no overlay, we need a reliable way to close the popup when clicked outside. This becomes difficult since at many places, we are stopping the propagation of the event to the body. This can be solved however, by reversing the order of the event bubbling. The other challenge is to optimise the solution such that each popover component does not have individual listeners defined in it as this might degrade performance. The two challenges can be solved by creating a Context that saves the state of any active Popover. This context will wrap our App component. The context will create listeners that will close the menu if: a. Escape key is pressed b. If anywhere else is clicked c. If tab is switched The list can be easily extended in the future.

Following shows example of such a Context in action:

const PopoverContextProvider = (props) => {
    const [isOpen, setIsOpen] = React.useState(false);
    const activePopoverRef = React.useRef(null);

    const closePopover = () => {
        if (!activePopoverRef.current) {
            return;
        }
        activePopoverRef.current.close();
        activePopoverRef.current = null;
        setIsOpen(false);
    };

    React.useEffect(() => {
        const listener = (e) => {
            if (
                !activePopoverRef.current
                || !activePopoverRef.current.ref
                || activePopoverRef.current.ref.current.contains(e.target)
            ) {
                return;
            }
            closePopover();
        };
        document.addEventListener('click', listener, true);
        return () => {
            document.removeEventListener('click', listener, true);
        };
    }, []);

    React.useEffect(() => {
        const listener = (e) => {
            if (e.key !== 'Escape') {
                return;
            }
            closePopover();
        };
        document.addEventListener('keydown', listener);
        return () => {
            document.removeEventListener('keydown', listener);
        };
    }, []);

    React.useEffect(() => {
        const listener = () => {
            if (document.hasFocus()) {
                return;
            }
            closePopover();
        };
        document.addEventListener('visibilitychange', listener);
        return () => {
            document.removeEventListener('visibilitychange', listener);
        };
    }, []);

    const onOpen = (popoverParams) => {
        if (activePopoverRef.current) {
            closePopover();
        }
        activePopoverRef.current = popoverParams;
        setIsOpen(true);
    };
    return (
        <PopoverContext.Provider
            value={{
                onOpen,
                popover: activePopoverRef.current,
                isOpen,
            }}
        >
            {props.children}
        </PopoverContext.Provider>
    );
};

We can then use the above context in a custom PopoverWithoutOverlay component

const Popover = (props) => {
     const ref = React.useRef(null);
     const {onOpen} = React.useContext(PopoverContext);

    React.useEffect(() => {
       if(props.isVisible) {
           onOpen({ref, close: props.onClose});
       }
    }, [props.isVisible]);

    return props.isVisible ? (
        <Pressable style={[modalStyle, {zIndex: 1}]} ref={ref}>
            <View>{props.children}</View>
        </Pressable>
    ) : null;
};

The component above will use the same styles as the current popover component such that the design remains consistent. Once this component is created, we can then add a new prop to the PopoverWithMeasuredContent component called withoutOverlay. If this prop is true, we can replace our existing Popover with the new one which we created.

        const PopoverComponentToUse = this.props.withoutOverlay ? PopoverWithoutOverlay : Popover;

We can use the above context in any other component to check if any Context menu is in open state. If so, we can modify the styles etc. This will cover this comment.

For testing purposes, I've created a a branch with my implementation of this new popover. It can be checked here.

This is how it looks: https://user-images.githubusercontent.com/30054992/223510550-a8ffebd9-7c3a-43ed-843c-ea0cec4dbe2f.mov

Since this is relatively a larger change, my suggestion is to add it incrementally. In the first step, we can add this to replace Context menu popover here only.

What alternative solutions did you explore? (Optional)

None

aimane-chnaif commented 1 year ago

@allroundexperts the another main difference between current popover and your new popover is that it will be mounted every time becomes visible and disappeared (without unmount call) when becomes invisible. I think there will be stale state values if we use conditional render based on isVisible flag. Current popover is Modal and it is mounted only once and not mounted again regardless of visibility state change.

allroundexperts commented 1 year ago

@allroundexperts the another main difference between current popover and your new popover is that it will be mounted every time becomes visible and disappeared (without unmount call) when becomes invisible. I think there will be stale state values if we use conditional render based on isVisible flag. Current popover is Modal and it is mounted only once and not mounted again regardless of visibility state change.

@aimane-chnaif Can you give an example of the stale state which you mentioned above?

aimane-chnaif commented 1 year ago

@allroundexperts your MVP looks good, but compared to other apps, can we improve these?

When right click area outside which doesn't support custom popover, close popover while opening default system menu

Multiple popovers/modals should not be visible at the same time. When open new one, old one should be closed

Close when app becomes inactive, i.e. when switch to another tab
Close on Escape shortcut key

I understand this is a big feature request but I want our popover to be implemented in top level, similar to Gmail. @dangrous what do you think?

allroundexperts commented 1 year ago

@allroundexperts your MVP looks good, but compared to other apps, can we improve these?

When right click area outside which doesn't support custom popover, close popover while opening default system menu

Multiple popovers/modals should not be visible at the same time. When open new one, old one should be closed

Close when app becomes inactive, i.e. when switch to another tab

Close on Escape shortcut key

I understand this is a big feature request but I want our popover to be implemented in top level, similar to Gmail. @dangrous what do you think?

@aimane-chnaif Did you test this on my branch specifically? The first point you raised can be fixed easily. Regarding the second point, can you mention how you reproduced it?

The third and fourth points can be implemented easily as well. If you want, I can add those to the POC.

aimane-chnaif commented 1 year ago

The first point you raised can be fixed easily. The third and fourth points can be implemented easily as well. If you want, I can add those to the POC.

Sure, that would be great if all of them can be done in POC

Regarding the second point, can you mention how you reproduced it?

https://user-images.githubusercontent.com/96077027/223783004-e084bc0f-fa19-412f-97c8-0d2c84fe29c6.mov

aimane-chnaif commented 1 year ago

Actually, 2nd point needs customization. While showing context menu, maybe prevent showing mini context menu.

allroundexperts commented 1 year ago

Actually, 2nd point needs customization. While showing context menu, maybe prevent showing mini context menu.

For some weird reason, I did not notice this when I was working on it. Like even in the video that I attached, this doesn't seem to occur. I'll double check though.

aimane-chnaif commented 1 year ago

For some weird reason, I did not notice this when I was working on it. Like even in the video that I attached, this doesn't seem to occur. I'll double check though.

Your video has that issue as well. Another concern is hover background. Maybe we need to separate color between selected and hovered (this is Gmail behavior), or leave as is.

allroundexperts commented 1 year ago

@aimane-chnaif I've tried to incorporate all of the requests in that branch. However, I feel like like the mini context menu is more of a tooltip and shouldn't be disabled when the context menu is open. Please let me know your thoughts.

allroundexperts commented 1 year ago

@aimane-chnaif Did you get a chance to look at the changes?

MelvinBot commented 1 year ago

@dangrous @greg-schroeder @zanyrenney @aimane-chnaif 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!

zanyrenney commented 1 year ago

Thanks Melv.

@dangrous Are you able to answer these comments from @aimane-chnaif and @allroundexperts to get this sped up here please?

aimane-chnaif commented 1 year ago

https://github.com/Expensify/App/issues/15289#issuecomment-1460517197 @zanyrenney I'd like to get your suggestion as well regarding expected behavior 🙂

dangrous commented 1 year ago

To me the behavior suggested in https://github.com/Expensify/App/issues/15289#issuecomment-1460517197 makes the most sense - as well as the idea to kind of revamp this on the top level.

However, I'm wondering how best to actually move forward with this. I'm wondering if it might make the most sense to go through a problem/statement, or at the very least start a conversation in Slack to make sure everyone's aligned (since it'll affect a number of different parts of the app). What do you think, @zanyrenney @aimane-chnaif @allroundexperts ?

Separately (for now) I'm going to revert the cost back to $2000 since we're still working through the proposals submitted at that point.

MelvinBot commented 1 year ago

Upwork job price has been updated to $2000

aimane-chnaif commented 1 year ago

I'm wondering if it might make the most sense to go through a problem/statement, or at the very least start a conversation in Slack to make sure everyone's aligned (since it'll affect a number of different parts of the app)

yes, we should do this since this is entirely a new feature request rather than bug.

allroundexperts commented 1 year ago

To me the behavior suggested in #15289 (comment) makes the most sense - as well as the idea to kind of revamp this on the top level.

However, I'm wondering how best to actually move forward with this. I'm wondering if it might make the most sense to go through a problem/statement, or at the very least start a conversation in Slack to make sure everyone's aligned (since it'll affect a number of different parts of the app). What do you think, @zanyrenney @aimane-chnaif @allroundexperts ?

Separately (for now) I'm going to revert the cost back to $2000 since we're still working through the proposals submitted at that point.

@dangrous I agree with this as well.

Expensify / App

[$8000] Context menu does not close when secondary menu opens or secondary menu should not open #15289

Action Performed:

Expected Result

Actual Result

Workaround:

Platforms:

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?

Optional solution

Proposal 2

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?

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)

Updated 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)