Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.52k stars 2.87k forks source link

[HOLD for payment 2023-08-28] [$2000] Web- Chat - Pressing Copy to clipboard, Copy link, or Mark as unread from the Mini context menu does not refocus the Composer #20079

Closed kbecciv closed 1 year ago

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

  1. Open App
  2. Go to any chat
  3. Send a message and click anywhere on the page to lose composer focus
  4. Hover over the message and click any of these three Copy to clipboard, Copy link, Mark as unread

Expected Result:

Pressing any of these three options Copy to clipboard, Copy link, Mark as unread should have to refocus the composer like we do when using the Context menu

Actual Result:

Pressing any of these three options, Copy to clipboard, Copy link, or Mark as unread is, not refocusing the composer

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.22.0

Reproducible in staging?: yes

Reproducible in production?: yes

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

https://github.com/Expensify/App/assets/93399543/17878553-d3bc-4891-9525-ed47bd10fceb

https://github.com/Expensify/App/assets/93399543/e4ae86d4-d71d-4d31-8a1c-b0eb4942501b

Expensify/Expensify Issue URL:

Issue reported by: @jayeshmangwani

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01517d6a9b05f42ab8
  • Upwork Job ID: 1666174440708157440
  • Last Price Increase: 2023-06-29
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

jayeshmangwani commented 1 year ago

Proposal

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

Pressing Copy to clipboard, Copy link, Download, or Mark as unread from the Mini context menu does not refocus the Composer, but Composer is refocused when pressed from the context menu opened from a long press

What is the root cause of that problem?

We are focusing the composer on the above actions if we press from the Context menu and we are doing nothing if we press from the mini menu here is the condition for checking if the press is from the popover or not and if the press is from the context menu then call the ReportActionComposeFocusManager.focus https://github.com/Expensify/App/blob/c917c447f225fe3b194a5e03f568cca0de9e5e71/src/pages/home/report/ContextMenu/ContextMenuActions.js#L102-L104

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

We need to add an else condition to Copy to clipboard, Copy link, Download, and Mark as unread(or any other actions) press, where we will call ReportActionComposeFocusManager.focus() if it is not the from Popover, it means mini context menu https://github.com/Expensify/App/blob/c917c447f225fe3b194a5e03f568cca0de9e5e71/src/pages/home/report/ContextMenu/ContextMenuActions.js#L45 here need to add else and all other presses like Copy to clipboard, Copy link, and Mark as unread https://github.com/Expensify/App/blob/c917c447f225fe3b194a5e03f568cca0de9e5e71/src/pages/home/report/ContextMenu/ContextMenuActions.js#L102-L104

  } else {
    ReportActionComposeFocusManager.focus()
  }

What alternative solutions did you explore? (Optional)

none

melvin-bot[bot] commented 1 year ago

@adelekennedy Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01517d6a9b05f42ab8

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

will0225 commented 1 year ago

I want to have this issue.

youssef-lr commented 1 year ago

@0xmiroslav what do you think of the proposal above?

0xmiros commented 1 year ago

@jayeshmangwani your solution easily causes regressions. Please test yourself after applying your solution.

youssef-lr commented 1 year ago

Waiting for more proposal.

the-mold commented 1 year ago

Proposal

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

Composer field must be focused, when user clicks on one of three actions(copy clipboard, copy link and mark message as new) in the popup element(MiniReportActionContextMenu component) that appears in the top right corner when a message is hovered.

What is the root cause of that problem?

All three actions do NOT implement methods for focusing the composer when actions are called from the MiniReportActionContextMenu component. In their onPress handlers, there is a method called hideContextMenu but it is not used in case of the MiniReportActionContextMenu which is our case. hideContextMenu is functional only for the ReportActionContextMenu(the popup that appears with the right click on an action). https://github.com/Expensify/App/blob/035783a7984e0214677b5364d9351c968887bf58/src/pages/home/report/ContextMenu/ContextMenuActions.js#L192

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

The first step is easy: we need to focus the composer manually when actions are triggered from the MiniReportActionContextMenu component. I will implement such function:

function hideAllContextMenus(isMini) {
    if (isMini) {
        ReportActionComposeFocusManager.focus();
        return;
    }

    hideContextMenu(true, ReportActionComposeFocusManager.focus);
}

and I will call it in onPress methods for all three actions, like so:

onPress: (closePopover, {reportAction, selection}) => {
    //...
    hideAllContextMenus(!closePopover);
}

But then I start getting such effect: compose field is focused but the click animation is broken:

https://github.com/Expensify/App/assets/15109844/cca15590-5cd5-44eb-a909-25ecff0a6f6c

Each action in the view is wrapped in a Hoverable HOC component. https://github.com/Expensify/App/blob/035783a7984e0214677b5364d9351c968887bf58/src/pages/home/report/ReportActionItem.js#L427-L428

I noticed that when I call the ReportActionComposeFocusManager.focus() method, the onBlur method in the Hoverable is triggered and the hover state is set to false for a moment. This triggers rerender in ContextMenuItem component and completion animation seems to be broken.

I propose to ignore state changes in Hoverable component in onBlur method(and ONLY this method) if the the composer element is clicked.

The current onBlur method in Hoverable component looks like this: https://github.com/Expensify/App/blob/035783a7984e0214677b5364d9351c968887bf58/src/components/Hoverable/index.js#L109-L117

This is the new onBlur method. Here I check if I am allowed to ignore hover state change for the element if this.props.ignoreBlurForElement prop allows it:

onBlur: (el) => {
    if (!this.wrapperView.contains(el.relatedTarget)) {
        if (!this.props.ignoreBlurForElement || !this.props.ignoreBlurForElement(el.relatedTarget)) {
            this.setIsHovered(false);
        }
    }

    if (_.isFunction(child.props.onBlur)) {
        child.props.onBlur(el);
    }
}

And this is how the prop is passed in the Hoverable component in the file ReportActionitem.js. The ignoreBlurForElement returns true if the element that user clicked on is the composer.

<Hoverable
  ...
  ignoreBlurForElement={(el) => el === lodashGet(ReportActionComposeFocusManager, 'composerRef.current')}>
...

This video is the end result. Notice that completion animation goes till the end and composer field is focused immediately after the click. https://github.com/Expensify/App/assets/15109844/82f9ad15-99fb-4b90-93ad-5a60a92233a6

What alternative solutions did you explore? (Optional)

In ContextMenuActions.js I could call ReportActionComposeFocusManager.focus() for MiniReportActionContextMenu component WITH a certain delay. I could make a setTimeout and wait for 1800ms(duration of the completion state animation) and only then shoot the ReportActionComposeFocusManager.focus() action. In this way the composer will be focused only after 1800ms. The initial solution is a bit harder but much cleaner.

youssef-lr commented 1 year ago

@0xmiroslav ^

0xmiros commented 1 year ago

@petromoldovan we don't recommend setTimeout unless it's the only solution. Still open for better proposals.

the-mold commented 1 year ago

@0xmiroslav I also do not recommend to set the setTimeout. It was just an alternative to emphasise how much better the main proposal is. What do you think about my main proposal?

melvin-bot[bot] commented 1 year ago

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

adelekennedy commented 1 year ago

Still pending more proposals!

melvin-bot[bot] commented 1 year ago

@youssef-lr @adelekennedy @0xmiroslav 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!

aliammar1995 commented 1 year ago

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

  Pressing Copy to clipboard, Copy link, or Mark as unread from the Mini 
  context menu does not refocus the Composer

What is the root cause of that problem?

  In MiniReportActionContextMenu component, when clicking menu items 
 (Copy to clipboard, copy link, Mark as unread), It  does not call
 ReportActionComposeFocusManager.focus method while calling 
  hideContextMenu.

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

   I found a solution.
   In hideContextMenu function, we can call 
   ReportActionComposeFocusManager.composerRef.current.focus() directly.

`function hideContextMenu(shouldDelay, onHideCallback = () => {}) {

.....
...
...

const instanceID = contextMenuRef.current.instanceID;
setTimeout(() => {
    if (contextMenuRef.current.instanceID !== instanceID) {
        return;
    }

    contextMenuRef.current.hideContextMenu(onHideCallback);
    ReportActionComposeFocusManager.composerRef.current.focus()
}, 800);

}`

It worked on my local.

https://github.com/Expensify/App/assets/136129373/57e19a9f-2c62-40a9-a913-20af079ca2cc

adelekennedy commented 1 year ago

pending proposal review

melvin-bot[bot] commented 1 year ago

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

youssef-lr commented 1 year ago

@0xmiroslav what do you think of the proposal above?

0xmiros commented 1 year ago

I don't follow any of proposals so far. Still open for better solutions

aliammar1995 commented 1 year ago

Exactly, hideContextMenu function is for hide context menu, not Mini context menu. so we have to add ReportActionComposeFocusManager.focus() in onPress method in contextMenuActions.js file.

      `onPress: (closePopover, {reportAction, selection}) => {
       ......
       ......

        if (closePopover) {
            hideContextMenu(true, ReportActionComposeFocusManager.focus);
        }
        ReportActionComposeFocusManager.focus()
    },`

It really works well.

melvin-bot[bot] commented 1 year ago

@youssef-lr @adelekennedy @0xmiroslav 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!

ShogunFire commented 1 year ago

@0xmiroslav Can you explicit what type of regressions you had with this proposal . @petromoldovan speaks about a problem with animation but I don't have it, was it also a problem with animation that you had ?

This is what I have with the first proposal:

https://github.com/Expensify/App/assets/19537677/40f79aef-e520-45d8-9c0a-9cc41612e7fd

adelekennedy commented 1 year ago

still pending proposal updates

melvin-bot[bot] commented 1 year ago

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

aliammar1995 commented 1 year ago

I think the bounty is needed. we have to redesign structure about Mini context menu.

aliammar1995 commented 1 year ago

https://github.com/Expensify/App/issues/20079#issuecomment-1602108187 Above proposal works well if we don't want to redesign structure.

ShogunFire commented 1 year ago

@aliammar1995 Just in case, you are responding to melvin who is a bot.

aliammar1995 commented 1 year ago

sorry, I am about to respond to everyone :)

adelekennedy commented 1 year ago

doubling - we're still open to better proposals

melvin-bot[bot] commented 1 year ago

Upwork job price has been updated to $2000

dayana7204 commented 1 year ago

Proposal

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

Pressing Copy to clipboard, Copy link, or Mark as unread from the Mini context menu does not refocus the Composer

What is the root cause of that problem?

We are focusing the composer only when we press these actions from the Context menu, but we don't set focus to composer when we press action from the mini menu.

ContextMenuActions.js

if (closePopover) {
      hideContextMenu(true, ReportActionComposeFocusManager.focus);
}

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

We can resolve this issue by adding code like as following in onPress function.

  } else {
    ReportActionComposeFocusManager.focus()
  }

Btw in this solution, composer get focused instantly after click action in mini context menu. If we need to give some delay, we can define call back function which is called after toggle action ended, and add above code in the call back function.

https://github.com/Expensify/App/assets/136715042/76381b68-203d-43ab-a07c-a6fc8bc16de4

What alternative solutions did you explore? (Optional)

I think there is no other better solution.

dragoilic commented 1 year ago

Proposal

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

Pressing Copy to clipboard, Copy link, or Mark as unread inside the Mini context menu does not refocus the Composer

What is the root cause of that problem?

The "onPress" handlers for the actions "Copy to clipboard", "Copy link", or "Mark as unread" are currently not refocusing the Composer correctly, following the interaction with the mini context menu.

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

I think creating a new function closeContextMenu in global scope in ContextMenuActions.js

function closeContextMenu(isMini, onHideCallback) {
    if (isMini) {
        onHideCallback();
    } else {
        hideContextMenu(true, onHideCallback);
    }
};

And then will update the related code in the component. For example, Existing code:

onPress: (closePopover, {reportAction}) => {
    const message = _.last(lodashGet(reportAction, 'message', [{}]));
    const html = lodashGet(message, 'html', '');
    const attachmentDetails = getAttachmentDetails(html);
    const {originalFileName, sourceURL} = attachmentDetails;
    const sourceURLWithAuth = addEncryptedAuthTokenToURL(sourceURL);
    const sourceID = (sourceURL.match(CONST.REGEX.ATTACHMENT_ID) || [])[1];
    Download.setDownload(sourceID, true);
    fileDownload(sourceURLWithAuth, originalFileName).then(() => Download.setDownload(sourceID, false));
    if (closePopover) {
        hideContextMenu(true, ReportActionComposeFocusManager.focus);
    }
},

Updated code:

onPress: (closePopover, {reportAction}) => {
    const message = _.last(lodashGet(reportAction, 'message', [{}]));
    const html = lodashGet(message, 'html', '');
    const attachmentDetails = getAttachmentDetails(html);
    const {originalFileName, sourceURL} = attachmentDetails;
    const sourceURLWithAuth = addEncryptedAuthTokenToURL(sourceURL);
    const sourceID = (sourceURL.match(CONST.REGEX.ATTACHMENT_ID) || [])[1];
    Download.setDownload(sourceID, true);
    fileDownload(sourceURLWithAuth, originalFileName).then(() => Download.setDownload(sourceID, false));
    closeContextMenu(!closePopover, ReportActionComposeFocusManager.focus)
},

If this is the ideal solution, I can confidently proceed with updating the component. Thanks

What alternative solutions did you explore? (Optional)

aliammar1995 commented 1 year ago

Proposal

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

Pressing Copy to clipboard, Copy link, or Mark as unread inside the Mini context menu does not refocus the Composer

### What is the root cause of that problem?

The method that let the composer refocus is ReportActionComposeFocusManager.focus in onPress method in ContextMenuActions.js

hideContextMenu(true, ReportActionComposeFocusManager.focus);

We have two context menu component, one is PopoverReportActionContextMenu and another is MiniContextMenu.

When the user click one of actions on context menu, it call above hideContextMenu function in onPress method and also calls hideContextMenu function in PopoverReportActionContextMenu component.

contextMenuRef.current.hideContextMenu(onHideCallback);

    hideContextMenu(onHideActionCallback) {
        if (_.isFunction(onHideActionCallback)) {
            this.onPopoverHideActionCallback = onHideActionCallback;
        }
        this.setState({
            reportID: 0,
            reportAction: {},
            selection: '',
            reportActionDraftMessage: '',
            isPopoverVisible: false,
        });
    }

this hideContextMenu function store onHideActionCallback which is ReportActionComposeFocusManager.focus and set IsPopoverVisible state variable as false, which let Popover close.

In that case ContextMenu is open, IsPopoverVisible set as true, if hideContextMenu is called (when the user click action), IsPopoverVisible set as false. so ContextMenu is closed. At that time onModalHide prop function is called This function calls above stored ReportActionComposeFocusManager.focus. so composer is refocused.

onModalHide={this.runAndResetOnPopoverHide}

  runAndResetOnPopoverHide() {
        this.setState({reportID: '0', reportAction: {}}, () => {
            this.onPopoverHide = this.runAndResetCallback(this.onPopoverHide);
            this.onPopoverHideActionCallback = this.runAndResetCallback(this.onPopoverHideActionCallback);
        });
    }
    runAndResetCallback(callback) {
        callback();
        return () => {};
    }

In MiniContextMenu that we are considering now, when the user click action, it calls hideContextMenu in PopoverReportActionContextMenu component like above.

But as you know, IsPopoverVisible variable is already set as false because ContextMenu ( which is not MiniContextMenu) is not open. so it can not call onModalHide props function which is called when Modal is closed.

As a result, In MiniContextMenu, ReportActionComposeFocusManager.focus can not be called, thus, composer is not refocused.

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

I think we have to make another method else that does not call hideContextMenu function for MiniContextMenu. For example, In case MiniContextMenu, It should call onPressMini, otherwise, calls onPress.

onPressMini: (closePopover, {reportAction}) => {
            const message = _.last(lodashGet(reportAction, 'message', [{}]));
            const html = lodashGet(message, 'html', '');
            const attachmentDetails = getAttachmentDetails(html);
            const {originalFileName, sourceURL} = attachmentDetails;
            const sourceURLWithAuth = addEncryptedAuthTokenToURL(sourceURL);
            const sourceID = (sourceURL.match(CONST.REGEX.ATTACHMENT_ID) || [])[1];
            Download.setDownload(sourceID, true);
            fileDownload(sourceURLWithAuth, originalFileName).then(() => Download.setDownload(sourceID, false));
            ReportActionComposeFocusManager.focus();
        },

or we can modify hideContextMenu function in PopoverReportActionContextMenu.js

hideContextMenu(onHideActionCallback, isMini = false) {
        if (_.isFunction(onHideActionCallback)) {
            this.onPopoverHideActionCallback = onHideActionCallback;
        }
        this.setState({
            selection: '',
            reportActionDraftMessage: '',
            isPopoverVisible: false,
        });

        if(isMini) {
            this.runAndResetOnPopoverHide()
        }
    }

It is working on my end.

### What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 1 year ago

@youssef-lr @adelekennedy @0xmiroslav this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Thanks!

melvin-bot[bot] commented 1 year ago

Current assignee @0xmiroslav is eligible for the Internal assigner, not assigning anyone new.

Skalakid commented 1 year ago

Hey, I’m Michał from Software Mansion, an expert agency, and I will help close this issue out.

aliammar1995 commented 1 year ago

@Skalakid How do you think this proposal

Skalakid commented 1 year ago

Hi @aliammar1995, the first solution that you described will have a lot of repetitive code. onPress and onPressMini are very similar. I the second solution I don't like that we are doing something connected with MiniContextMenu in the PopoverContextMenu file

Skalakid commented 1 year ago

@0xmiroslav how should the ideal solution look like? Should TextInput be focused instantly after the MiniContextMenu item is clicked? Or should it be focused after 1800ms when the item is ending to be in the success ✔️ state (like it is done in PopoverMenu)?

aliammar1995 commented 1 year ago

@Skalakid it should work like in PopoverMenu

aliammar1995 commented 1 year ago

As I describe above, onPress method is designed and needed for PopoverMenu, Exactly, not designed for MiniContextMenu. so we have to make other method for MiniContentMenu.

Skalakid commented 1 year ago

@dayana7204 Thanks for the proposal but I think it is basically the same as the previous one

Skalakid commented 1 year ago

@dragoilic I really like the fact that you created a separate function closeContextMenu to manage hiding menu and focusing TextInput. However in the end it is the same situation as I described in my previous comment, we already got similar proposal

Skalakid commented 1 year ago

Proposal

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

Pressing any of these three options, Copy to clipboard, Copy link, or Mark as unread is, not refocusing the composer

What is the root cause of that problem?

We are refocusing the composer only when hiding the context menu, but we don't do that when using options from mini context menu:

 if (closePopover) { 
     hideContextMenu(true, ReportActionComposeFocusManager.focus); 
 } 

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

If we want to refocus the composer immediately, we should add launching ReportActionComposeFocusManager.focus(); function after pressing items in the Mini context menu. We can do that by changing ContextMenuItem.js like that:

  if (successIcon || successText) {
      toggleDelayButtonState();
      if (isMini) {
          ReportActionComposeFocusManager.focus();
      }
  }

However, we will always focus on the main composer in this solution. But what if we start editing a message and want to copy something to it? With this solution, the main composer will be focused instead of the last edited message. To fix that we can modify ReportActionComposeFocusManager to remember the last focused composer, and then add to it a callback that will focus it. Let's remember that some other features are using this focus manager. In my opinion, we can add a new function that will manage focusing edited messages composers and main composer, to not break other functionalities. To do that we have to modify ReportActionComposeFocusManager:

let focusMainComposerCallback = null;

function init(callback) {
    focusCallback = callback;
    focusMainComposerCallback = callback;
}

function onMainComposerFocus(callback) {
    focusMainComposerCallback = callback;
}

function focusMainComposer() {
    if (!_.isFunction(focusMainComposerCallback)) {
        return;
    }
    focusCallback = focusMainComposerCallback;
    focusMainComposerCallback();
}

Now we these functions we can easily modify the behavior of refocusing. Now we have to add onComposerFocus to ReportActionItemMessageEdit.js to remember last focused edit message composer:

  useEffect(() => {
      if (isFocused) {
          ReportActionComposeFocusManager.onComposerFocus(() => {
              props.forwardedRef.current.focus();
          });
      }
      ...
  }
...
  const deleteDraft = useCallback(() => {
          ...
          ReportActionComposeFocusManager.focusMainComposer();
          ...
  }
...

Also, we have to add ReportActionComposeFocusManager.init and ReportActionComposeFocusManager.onComposerFocus in ReportActionCompose.js, to remember main composer and change onComposerFocus after focusing it. Here is the branch with all my changes

If we want to use different refocus logic, for example, always focus a composer that is at the top of the chat, we can modify ReportActionComposeFocusManager to have an array with opened composers and then always focus on the first ref in it. When a user saves or cancels the edited message, the ref will be removed from the array. The main composer ref will always be the last element, so the array will never be empty

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

melvin-bot[bot] commented 1 year ago

@youssef-lr, @adelekennedy, @0xmiroslav Eep! 4 days overdue now. Issues have feelings too...