[$4000] mWeb - Copy to email/URL is not working correctly - reported by @mateusbra

[kavimuru]

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

---

Issue was found when executing #8195

Action Performed:

1. Log in on New Expensify.
2. Open a report.
3. Type an URL and an e-mail on chat.

Expected Result:

The message "Copy e-mail to clipboard" displayed when you try to copy an email and "copy URL to clipboard" tapping URL

Actual Result:

The message "Copy to clipboard" displayed when you try to copy an email. Sometimes it shows as regular context menu

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Mobile Web

Version Number: 1.1.46-0

Reproducible in staging?: y

Reproducible in production?: y (New feature)

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/160028142-3fe96a20-6e66-4605-ae2a-618bc322f957.mp4

https://user-images.githubusercontent.com/43996225/160028204-1fe2a591-3466-430e-9802-147d0baa43ee.mp4

Expensify/Expensify Issue URL:

Issue reported by: Reported by @mateusbra https://expensify.slack.com/archives/C01GTK53T8Q/p1647474115298459

Slack conversation:

View all open jobs on GitHub Issue was found when executing #8195

[melvin-bot[bot]]

Triggered auto assignment to @johnmlee101 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[mateusbra]

@kavimuru I found this issue while working on https://github.com/Expensify/App/pull/8195 and reported on slack here: https://app.slack.com/client/T03SC9DTT/C01GTK53T8Q does that count for reporting bounty?

[kavimuru]

@mateusbra I don't have access to the slack. But as per this comment, https://github.com/Expensify/App/pull/8195#issuecomment-1070240184 yes. Also tester can repro this bug too https://github.com/Expensify/App/issues/8006#issuecomment-1069334038

[johnmlee101]

Opening this up!

[melvin-bot[bot]]

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

[johnmlee101]

This should stay daily priority

[michaelhaxhiu]

@mateusbra do you have interest in working on this one since you discovered it? Feel free to leave a proposal if so.

@kavimuru can you please make sure we at the "@ reported by" to a GH title if they are due the bug reporting bounty? Seems applicable in this case for @mateusbra but maybe I'm not understanding fully?

[MelvinBot]

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

[Christinadobrzyn]

Sorry for the delay, i was ooo. Posted job to Upwork;

Internal job - https://www.upwork.com/ab/applicants/1508625903649284096/job-details External job - https://www.upwork.com/jobs/~01bd32eda238a19082

It does look like @mateusbra found/reported this issue so added them to the OP as the reporter.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[zamarlancer]

Hi, This is Zamar from Upwork.com I will fix the issue by implementing URL parsing feature in ContextMenuAction. Now there is no checking parts for url/email when the context menu is shown. So there will be a validation for the clipboard text, which will check if it is an email address or an url; and switch the caption of the menu item according to the result. Thank you.

[parasharrajat]

I don't see how this is different from https://github.com/Expensify/App/pull/8195. This should have been fixed on https://github.com/Expensify/App/issues/8311. cc: @Santhosh-Sellavel @mateusbra

why do we need a different issue?

[Santhosh-Sellavel]

@parasharrajat #8195 - addresses the issue where clicking on an email in the message showed the copy URL option instead of copy email. https://github.com/Expensify/App/issues/8311 - But the existing copy URL itself not working on mWeb at all.

[marcochavezf]

Not overdue, waiting for proposals.

[Christinadobrzyn]

I'm going to be ooo until April 26th so I'm going to assign a new CM to this issue. Thank you!

[melvin-bot[bot]]

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

[Christinadobrzyn]

Price increase to $500

waiting on proposals...

[eVoloshchak]

Root cause: When we long press on an email/link, BaseAnchorForCommentsOnly's onSecondaryInteraction is fired, calling ReportActionContextMenu.showContextMenu with a corresponding type (CONTEXT_MENU_TYPES.EMAIL or CONTEXT_MENU_TYPES.LINK). Howewer, since BaseAnchorForCommentsOnly is wrapped inside a second PressableWithSecondaryInteraction on ReportActionItem page, it's showPopover method is also fired, calling ReportActionContextMenu.showContextMenu with a CONTEXT_MENU_TYPES.REPORT_ACTION type.

So, in short, when we long press, ReportActionContextMenu.showContextMenu is called with the right type and then almost instantly it gets called with the wrong type, which results in the wrong context menu being shown.

Proposal: Throttle showContextMenu method, so the second call get's omitted.

  showContextMenu = _.throttle((
      type,
      event,
      selection,
      contextMenuAnchor,
      reportID,
      reportAction,
      draftMessage,
      onShow = () => {},
      onHide = () => {},
  ) => {
      const nativeEvent = event.nativeEvent || {};
      this.contextMenuAnchor = contextMenuAnchor;

      // Singleton behaviour of ContextMenu creates race conditions when user requests multiple contextMenus.
      // But it is possible that every new request registers new callbacks thus instanceID is used to corelate those callbacks
      this.instanceID = Math.random().toString(36).substr(2, 5);

      // Register the onHide callback only when Popover is shown to remove the race conditions when there are mutltiple popover open requests
      this.onPopoverShow = () => {
          onShow();
          this.onPopoverHide = onHide;
      };
      this.getContextMenuMeasuredLocation().then(({x, y}) => {
          this.setState({
              cursorRelativePosition: {
                  horizontal: nativeEvent.pageX - x,
                  vertical: nativeEvent.pageY - y,
              },
              popoverAnchorPosition: {
                  horizontal: nativeEvent.pageX,
                  vertical: nativeEvent.pageY,
              },
              type,
              reportID,
              reportAction,
              selection,
              isPopoverVisible: true,
              reportActionDraftMessage: draftMessage,
          });
      });
  }, 100, {trailing: false})

Result:

Chrome on Android

https://user-images.githubusercontent.com/9059945/163683498-971dc82d-ae30-49a4-93df-ae03d8673527.mp4

Safari on iOS

https://user-images.githubusercontent.com/9059945/163683471-88facc87-5664-4af5-b166-934eddc856e3.mp4

[parasharrajat]

Root cause seems correct but I don't like the proposal.

[eVoloshchak]

Root cause seems correct but I don't like the proposal.

I've found a simpler fix: The issue arises because in PressableWithSecondaryInteraction/index.js, we have a LongPressGestureHandler, which wraps Pressable, so essentially links are wrapped like this:

<LongPressGestureHandler>
  <Pressable>
    <LongPressGestureHandler>
      <Pressable>
        <TextOfTheLink>
      </Pressable>
    </LongPressGestureHandler>
  </Pressable>
</LongPressGestureHandler>

To fix this, we need to:

1. Add longPressGestureHandlerEnabled={false} to this line on ReportActionItem (prop probably needs a better name)
2. Change this line to if (e.nativeEvent.state !== State.ACTIVE || !this.props.longPressGestureHandlerEnabled) {

To be honest, I'm not entirely sure why it works, but it does

https://user-images.githubusercontent.com/9059945/163777528-d0ace458-b6e2-40f8-91c7-534dce3f4870.mp4

[parasharrajat]

Ok. What is the base of this solution? Why do you think we should/have to do this?

The proposal is still lacking the proper explanation of Why the solution?

[eVoloshchak]

Ok. What is the base of this solution? Why do you think we should/have to do this?

The proposal is still lacking the proper explanation of Why the solution?

Case 1: user long presses on a comment: LongPressGestureHandler's onHandlerStateChange fires, which in turn calls onSecondaryInteraction={this.showPopover}

Case 2: user long presses on a link inside the comment: LongPressGestureHandler's onHandlerStateChange fires, which in turn calls onSecondaryInteraction={(e) => ReportActionContextMenu.showContextMenu()}. But, since in this case we have one LongPressGestureHandler's nested inside another one, both of their onHandlerStateChange events are fired, one on BaseAnchorForCommentsOnly/index.js (the right one) and one from ReportActionItem.js (the wrong one).

Adding longPressGestureHandlerEnabled={false} will essentially disable parent's onHandlerStateChange in case it's child has already fired onHandlerStateChange.

[parasharrajat]

I understood the root cause. I got your solution and how this works. But what I am asking is why your solution(Workaround)?

Why do you think we should do either of your changes? Is there any systematic way of preventing event bubbling?

---

I think there is a better solution. I will wait to see that proposal before making the choice.

[eVoloshchak]

I understood the root cause. I got your solution and how this works. But what I am asking is why your solution(Workaround)?

Why do you think we should do either of your changes? Is there any systematic way of preventing event bubbling?

Got it, sorry for the misunderstanding. I'll try to find a more universal approach a bit later

[marcochavezf]

Not overdue, waiting for proposals.

[arielgreen]

Price increased.

[b1tjoy]

Proposal

Reason

When we tap the email address, the inner PressableWithSecondaryInteraction component fire an event, in the event handler we call ReportActionContextMenu.showContextMenu() with type 'EMAIL': https://github.com/Expensify/App/blob/4c64bec3f38af925a01911155eab54feafc96cda/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly/index.js#L64-L76 And the outer PressableWithSecondaryInteraction component also fire an event after that, in the event handler we call ReportActionContextMenu.showContextMenu() with type 'REPORT_ACTION': https://github.com/Expensify/App/blob/4c64bec3f38af925a01911155eab54feafc96cda/src/pages/home/report/ReportActionItem.js#L99-L115 The outer component toggle the modal popover with regular context menu 'Copy to clipboard', so we can't see the modal popover with context menu 'Copy e-mail to clipboard'.

Solution 1

Since the inner PressableWithSecondaryInteraction already toggle the modal popover, and isPopoverVisible property stored in PopoverReportActionContextMenu's state, we can check isPopoverVisible is true in showContextMenu function, if it's true, do nothing and return. https://github.com/Expensify/App/blob/68f0bbf9c7fe8a95c7d65b0a9153229375863d1e/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js#L114-L124

+  if (this.state.isPopoverVisible) {
+    return;
+  }

https://github.com/Expensify/App/blob/68f0bbf9c7fe8a95c7d65b0a9153229375863d1e/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js#L125-L126

Solution 2

Add a parameter to function showContextMenu to record the inner component or the outer component called this function, and store this property in PopoverReportActionContextMenu's state, check the state value first in showContextMenu function, if inner component has already called this function, do nothing and return.

https://user-images.githubusercontent.com/103875612/166434253-ff37bc9c-34b5-43a1-871b-f20ba0bd1800.mp4

[parasharrajat]

Hmm! I strongly recommend looking back at the previous discussion. I asked

Why do you think we should do either of your changes? Is there any systematic way of preventing event bubbling?

@b1tjoy your proposal looks hacky to me.

[parasharrajat]

• We can re-imagine the LongPressHandler.
• Look at the history of the changes and see what broke this. Try to fix that.
• Try to replace that change with something else to see if you can fix this bug and still keep the previous fix.
• Try to see what platforms are affected. Do we need to find a solution for all platforms?
• Can we come up with a solution that works across platforms? If not, Is there a solution to that specific platform that is not working?
• Maybe try to use different solutions for different platforms.