kavimuru commented 2 years 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!

Issue was found when executing #8195

Action Performed:

Log in on New Expensify.
Open a report.
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

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01a8294a9c054ef83d
Upwork Job ID: 1595258571112300544

melvin-bot[bot] commented 2 years ago

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

mateusbra commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

Opening this up!

melvin-bot[bot] commented 2 years ago

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

johnmlee101 commented 2 years ago

This should stay daily priority

michaelhaxhiu commented 2 years ago

@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 commented 2 years ago

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

Christinadobrzyn commented 2 years ago

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] commented 2 years ago

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

melvin-bot[bot] commented 2 years ago

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

zamarlancer commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

Not overdue, waiting for proposals.

Christinadobrzyn commented 2 years ago

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] commented 2 years ago

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

Christinadobrzyn commented 2 years ago

Price increase to $500

waiting on proposals...

eVoloshchak commented 2 years ago

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 commented 2 years ago

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

eVoloshchak commented 2 years ago

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:

Add longPressGestureHandlerEnabled={false} to this line on ReportActionItem (prop probably needs a better name)
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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Not overdue, waiting for proposals.

arielgreen commented 2 years ago

Price increased.

b1tjoy commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

arielgreen commented 2 years ago

Price increased.

eVoloshchak commented 2 years ago

Proposal for mWeb:

This is not a problem with the Expensify App, it's an inconsistency of react-native-gesture-handler. This is a minimal reproduction snack, running it on web you can see that when longpressing on text, two console statements appear, while there's should be only one ('pressed 2')
Since issue is occuring only on mWeb, we're interested in PressableWithSecondaryInteraction/index.js file. LongPressGestureHandler was added to this file in this PR, which resolved #7462. However, #7462 was occuring only on native platforms, longpressing on images worked fine on mWeb.
That means, we can remove LongPressGestureHandler from PressableWithSecondaryInteraction/index.js and add onLongPress listener to Pressable:
```
onLongPress={this.callSecondaryInteractionWithMappedEvent}
```

https://user-images.githubusercontent.com/9059945/166672914-0de59ede-13f4-46db-97be-91b745f74f54.mp4

This resolves the current issue and doesn't break the #7462

But While testing this on other platforms i found a similar issue on iOS, which is similar to this, has different root cause, but still caused by LongPressGestureHandler. Pressing on plain link works fine on iOS, but then you have a message that contains text and link, regular context menu is displayed

https://user-images.githubusercontent.com/9059945/166672883-57751255-0649-41df-81b9-43ac39b65ea3.mov

But the cause is different from web, where both LongPressGestureHandler listeners are called, on iOS only the parent's listener get's called. However, i'm not sure this is not a fluke of dev mode/simulator, since I'm getting mixed results when testing this (sometimes it works as expected, sometimes it doesn't) and when running snack on iOS, i'm also getting random results.

Can someone plese check if this issue present when runnig production build on physical iOS device?

parasharrajat commented 2 years ago

Thanks for the updated proposal @eVoloshchak. Seems to me the best proposal so far but let's discuss the iOS issue you mentioned first. Unfortunately, I don't have iOS. Let's see who can help us here to test it.

parasharrajat commented 2 years ago

I tested it on your snack but I also found the same issue. After some digging, I found out that iOS handle events differently, and all the handlers are triggered simultaneously when touches are overlapping. https://github.com/software-mansion/react-native-gesture-handler/issues/1930#issuecomment-1068920275.

They suggest using https://docs.swmansion.com/react-native-gesture-handler/docs/gesture-composition/#simultaneous. I don't know how they can be used.

Let me know if you find something.

eVoloshchak commented 2 years ago

I found out that iOS handle events differently, and all the handlers are triggered simultaneously when touches are overlapping. software-mansion/react-native-gesture-handler#1930 (comment).

They suggest using https://docs.swmansion.com/react-native-gesture-handler/docs/gesture-composition/#simultaneous. I don't know how they can be used.

@parasharrajat , this is a great find actually, I was lookig for something like this, but never managed to find it. But we can't use this in our scenario (at least I couldn't figure out a way to). Besides, as it turns out, this is not iOS specific, this problem is also present on Android (the issue description should probably be updated). Same as with iOS, wrong context menu is displayed when we have a message with link and text.

https://user-images.githubusercontent.com/9059945/167257217-cbb1fb21-7723-4f77-bd3f-e996a0aaac8b.mp4

I've recreated out setup in this snack, you can see, that no matter where you long press, only the parent handler registers the touch. And this is not a problem with nested gesture handlers either. Even if you delete the parent handler (const App, line 18), the handler in AnchorRenderer doesn't get called. It starts working if you wrap Text into View in AnchorRenderer, but i'm guessing that would break #4911.

But as this is the case with mWeb, we can look into why LongPressHandler was added in the first place. As I mentioned previuosly, it was added to fix https://github.com/Expensify/App/issues/7462. So actually, we need LongPressHandler only for messages with attachments, not for all the messages. That means we can:

Add an isAttachment method to ReportActionItem.js

isAttachment() {
const message = _.last(lodashGet(this.props.action, 'message', [{}]));
const isAttachment = _.has(this.props.action, 'isAttachment')
    ? this.props.action.isAttachment
    : ReportUtils.isReportMessageAttachment(message);

return isAttachment;
}

Then pass it to PressableWithSecondaryInteraction here
```
useLongPressHandler={this.isAttachment()}
```

And in PressableWithSecondaryInteraction/index.native.js use LongPressHandler only for attachments

const PressableWithSecondaryInteraction = (props) => {
// Use Text node for inline mode to prevent content overflow.
const Node = props.inline ? Text : Pressable;
if (props.useLongPressHandler) {
    return (
        <LongPressGestureHandler
            onHandlerStateChange={(e) => {
                if (e.nativeEvent.state !== State.ACTIVE) {
                    return;
                }
                e.preventDefault();
                HapticFeedback.trigger();
                props.onSecondaryInteraction(e);
            }}
        >
            <Node
                ref={props.forwardedRef}
                onPress={props.onPress}
                onPressIn={props.onPressIn}
                onPressOut={props.onPressOut}
            // eslint-disable-next-line react/jsx-props-no-spreading
                {...(_.omit(props, 'onLongPress'))}
            >
                {props.children}
            </Node>
        </LongPressGestureHandler>
    );
}

return (
    <Node
        ref={props.forwardedRef}
        onPress={props.onPress}
        onPressIn={props.onPressIn}
        onPressOut={props.onPressOut}
        onLongPress={(e) => {
            e.preventDefault();
            HapticFeedback.trigger();
            props.onSecondaryInteraction(e);
        }}
    // eslint-disable-next-line react/jsx-props-no-spreading
        {...props}
    >
        {props.children}
    </Node>
)
};

Android:

https://user-images.githubusercontent.com/9059945/167259600-8c55bf45-df90-4c3f-a40d-a9a636114c8d.mp4

iOS:

https://user-images.githubusercontent.com/9059945/167259617-e988c14f-a35d-4901-bbe3-26584e4d8fe2.mov

parasharrajat commented 2 years ago

Thank you for looking into this. I need some time to research more on what you presented.

parasharrajat commented 2 years ago

I haven't forgotten this. Please expect an update in < 20 hours.

parasharrajat commented 2 years ago

Looking now.

parasharrajat commented 2 years ago

@eVoloshchak this proposal somewhat matches with the approach from the previous proposal. This is doable but I don't think it would be much helpful. It is limited to this scenario only. If we are about to make any change it can break.

What if we manage the nested handlers. I have a crazy ideas.

Utilize this https://docs.swmansion.com/react-native-gesture-handler/docs/2.0.0/gesture-handlers/basics/interactions#simultaneous-recognition.
Or create event bubbling.

We create a component that creates a LongPressGestureHandler around the children.
This component will save the ref with a nesting level in the Context.
Context value can be Array<{ref: Ref, level: Number}>. we will push each instance on the context value.
Now as soon as any handler's gesture is activated, we bubble the event to each handler. It can be done via updating the contextValue and setting active true one by one based on level {ref: Ref, level: Number, active: Boolean}.
Updating the Context will trigger an update on each component from step one.
We can also cancel the event at any level. We just need to prevent bubbling.

What do you say? This will allow us to use this anywhere in the app.

eVoloshchak commented 2 years ago

What do you say? This will allow us to use this anywhere in the app.

Sounds fun :) I'll try to implement this, will come back with results in ~3 days

parasharrajat commented 2 years ago

I am not saying that it is the way but thought of sharing. Thanks.

eVoloshchak commented 2 years ago

@parasharrajat , just realised this is not gonna work, since the issue isn't with nested gesture handlers. Take a look at this snack, it has only one LongPressGestureHandler, but still doesn't work. However, if we take the same snack, but wrap our url Text with View, it starts working - snack. I'll try to look into why wrapping View with LongPressGestureHandler works, but wrapping Text doesn't

marcochavezf commented 1 year ago

Not overdue, waiting for proposals.

parasharrajat commented 1 year ago

I will open a slack discussion to discuss existing proposals. But this is still open for better proposals.

arielgreen commented 1 year ago

I will open a slack discussion to discuss existing proposals. But this is still open for better proposals.

@parasharrajat are we still waiting?

parasharrajat commented 1 year ago

Yeah, I will open the discussion soon. Need to summarize the discussion. But I open to proposals.

parasharrajat commented 1 year ago

I will open the discussion on slack shortly...

marcochavezf commented 1 year ago

I will open the discussion on slack shortly...

Hi @parasharrajat! :) any update on this?

parasharrajat commented 1 year ago

This got slipped my mind. Opening the discussion after a while (need to summarize the above proposals). But I am also open for new proposals.

parasharrajat commented 1 year ago

Summarizing it. I will post it tomorrow.

Expensify / App

[HOLD for payment 2023-01-23] [$4000] mWeb - Copy to email/URL is not working correctly - reported by @mateusbra #8311

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platform:

Upwork Automation - Do Not Edit

8195 - addresses the issue where clicking on an email in the message showed the `copy URL` option instead of `copy email`.

Proposal

Reason

Solution 1

Solution 2

Expensify / App

[HOLD for payment 2023-01-23] [$4000] mWeb - Copy to email/URL is not working correctly - reported by @mateusbra #8311

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platform:

Upwork Automation - Do Not Edit

8195 - addresses the issue where clicking on an email in the message showed the copy URL option instead of copy email.

Proposal

Reason

Solution 1

Solution 2

8195 - addresses the issue where clicking on an email in the message showed the `copy URL` option instead of `copy email`.