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.

Sign up for GitHub

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

Jump to bottom

iOS/Android - Message - Copy message menu is not displayed when pressing on the message #2279

Closed
isagoico opened this issue Apr 7, 2021 · 37 comments · Fixed by #3659
Closed

iOS/Android - Message - Copy message menu is not displayed when pressing on the message #2279

isagoico opened this issue Apr 7, 2021 · 37 comments · Fixed by #3659
Assignees
@arielgreen @roryabraham @Viacheslav80
Labels
External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@isagoico
Copy link

isagoico commented Apr 7, 2021 •
edited by laurenreidexpensify
Loading

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

Expected Result:

Copy icon should appear and able to copy the message

Actual Result:

Copy icon does not appear, only device copy functionality opens up

Action Performed:

  1. Launch the app and login
  2. Select any message from LHN
  3. Long press on any message

Workaround:

If you press directly on the text the correct menu opens.

Platform:

Where is this issue occurring?

Web
iOS ✔️
Android ✔️
Desktop App
Mobile Web

Version Number: 1.0.15-0

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:
This was not reproducible on previous regression. The correct menu to copy the message would appear.

Video

Upwork post: https://www.upwork.com/jobs/~0117a4dc6fabf8301b
@isagoico

This comment has been minimized.

Sign in to view
@roryabraham roryabraham self-assigned this Apr 7, 2021
@roryabraham

This comment has been minimized.

Sign in to view
@isagoico isagoico mentioned this issue Apr 7, 2021
Closed
85 tasks
@roryabraham roryabraham added the Weekly KSv2 label Apr 10, 2021
@roryabraham roryabraham mentioned this issue Apr 10, 2021
Closed
5 tasks
@isagoico

This comment has been minimized.

Sign in to view
@roryabraham

This comment has been minimized.

Sign in to view
@roryabraham

This comment has been minimized.

Sign in to view
@isagoico

This comment has been minimized.

Sign in to view
1 similar comment
@isagoico

This comment has been minimized.

Sign in to view
@isagoico

This comment has been minimized.

Sign in to view
@roryabraham
Copy link
Contributor

My PR was pretty decayed and didn't work on all platforms, but since this can be worked on externally I think we should get this on Upwork. Hopefully my draft PR can still be a resource to the contributor that we hire to fix this issue. It's trickier than it looks! Curious to see what proposals people come up with.

@roryabraham roryabraham added the External Added to denote the issue can be worked on by a contributor label Jun 4, 2021
@MelvinBot

This comment has been minimized.

Sign in to view
@MelvinBot MelvinBot added Daily KSv2 and removed Weekly KSv2 labels Jun 4, 2021
@isagoico

This comment has been minimized.

Sign in to view
@laurenreidexpensify
Copy link
Contributor

https://www.upwork.com/jobs/~0117a4dc6fabf8301b

@raeidmir
Copy link

raeidmir commented Jun 7, 2021

I will check the call back implemented on press of text and the view that is carrying it. The problem should be somewhere in the callbacks.

@Viacheslav80
Copy link
Contributor

proposal:
/Users/syva80/Expensify.cash/src/pages/home/report/ReportActionItemFragment.js
I'm removing the prop "selectable"

62:       -  <Text selectable
          +  <Text
Screen.Recording.2021-06-07.at.21.07.44.mov

@roryabraham
Copy link
Contributor

roryabraham commented Jun 7, 2021 •
edited
Loading

@Viacheslav80 what about the case when there is markdown / html in a message? Have you thought about the other consequences of removing that prop? We still want to be able to select text to copy/paste on (desktop) web and the macOS app.

@Viacheslav80
Copy link
Contributor

Viacheslav80 commented Jun 8, 2021 •
edited
Loading

@roryabraham Hi! I tested it on the web and desktop - it is copied as before. Right now, markdown is not copied in the app, moreover, it is not displayed correctly right now. It doesn't depend on whether we have this prop or not

@roryabraham
Copy link
Contributor

@Viacheslav80, just removing the selectable prop from the component in that location is not an acceptable solution because it breaks the native highlight/copy functionality on the web & desktop apps.

@Viacheslav80
Copy link
Contributor

Viacheslav80 commented Jun 8, 2021 •
edited
Loading

@roryabraham Ok! We can do a check: selectable={(Platform.OS == "web") ? true : false} ?
Do you really not work selection in the web if you remove the "selectable" prop? it works for me on Mac (safari)

@roryabraham
Copy link
Contributor

roryabraham commented Jun 8, 2021 •
edited
Loading

Ah, I stand corrected, it seems removing the selectable prop doesn't break native highlight selection on web + desktop. But as a result, the solution doesn't seem to work on mobile web. I tested your original solution on iOS safari and it does not seem to reliably create the correct behavior:

image

image

image

@Viacheslav80
Copy link
Contributor

Viacheslav80 commented Jun 10, 2021 •
edited
Loading

@roryabraham Hi! Ok. We can do a check:
63 <Text selectable={isWeb && screenWidth >= 1024}>
or do something else to check for: isMobile.
Do we need the ability to highlight a part of the text on Mobile web?

Screen.Recording.2021-06-10.at.15.49.34.mov

@isagoico
Copy link
Author

isagoico commented Jun 13, 2021 •
edited
Loading

Issue reproducible today during KI retests

@laurenreidexpensify
Copy link
Contributor

@Viacheslav80 you're hired in Upwork now, so feel free to move forward with the PR. Thanks!

@Viacheslav80
Copy link
Contributor

Hi! Ok. I am started

@laurenreidexpensify laurenreidexpensify removed the External Added to denote the issue can be worked on by a contributor label Jun 17, 2021
@laurenreidexpensify laurenreidexpensify removed their assignment Jun 17, 2021
@laurenreidexpensify laurenreidexpensify added the External Added to denote the issue can be worked on by a contributor label Jun 17, 2021
@MelvinBot
Copy link

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

@laurenreidexpensify
Copy link
Contributor

@arielgreen I'm OOO til 5 July, so am reassigning my chores. Thanks.

@Viacheslav80 Viacheslav80 mentioned this issue Jun 17, 2021
Merged
5 tasks
@isagoico
Copy link
Author

isagoico commented Jun 21, 2021 •
edited
Loading

Issue reproducible during KI retests.

1 similar comment
@isagoico
Copy link
Author

Issue reproducible during KI retests.

@roryabraham
Copy link
Contributor

PR for this is in review

@arielgreen
Copy link
Contributor

Reopening, will close again upon payment July 6th.

@arielgreen arielgreen reopened this Jun 29, 2021
@arielgreen arielgreen changed the title iOS/Android - Message - Copy message menu is not displayed when pressing on the message [Hold for payment 2021-07-06] iOS/Android - Message - Copy message menu is not displayed when pressing on the message Jun 29, 2021
@arielgreen arielgreen added Weekly KSv2 and removed Daily KSv2 labels Jun 29, 2021
@arielgreen
Copy link
Contributor

Paid.

@rdjuric
Copy link
Contributor

rdjuric commented Jul 8, 2021

@roryabraham @arielgreen Shouldn't we reopen this? The PR was reverted so no fix is in place.

@puneetlath puneetlath reopened this Jul 8, 2021
@roryabraham roryabraham changed the title [Hold for payment 2021-07-06] iOS/Android - Message - Copy message menu is not displayed when pressing on the message iOS/Android - Message - Copy message menu is not displayed when pressing on the message Jul 9, 2021
@roryabraham
Copy link
Contributor

roryabraham commented Jul 9, 2021 •
edited
Loading

Good catch @rdjuric! @Viacheslav80 I saw you created this PR but quickly closed it. What happened there? Was there another PR I missed somewhere?

@Viacheslav80
Copy link
Contributor

@roryabraham Hi! I closed this PR because you already removed subscribe "touchstart" event
and @parasharrajat wrote that he would deal with the problem of text selection
Maybe I got something wrong but I didn't see the point in it PR
I haven't solved the text selection problem correctly yet, but I can create a pr and set delayLongPress={250} this partially solves the problem

@parasharrajat
Copy link
Member

parasharrajat commented Jul 9, 2021 •
edited
Loading

PR is ready here #3927. This will fix every context-menu glitch on M-web devices. I am not sure about native devices that should have been taken care of by this issue. My original comment was #3659 (comment).

I think we may need to block the native copy menu on the User name and avatar as well, as they are part of the message.

@roryabraham
Copy link
Contributor

Okay, @Viacheslav80 would you mind submitting a simplified version of your previous PR without the few touchstart lines that caused regressions?

@Viacheslav80 Viacheslav80 mentioned this issue Jul 13, 2021
Closed
5 tasks
@roryabraham
Copy link
Contributor

Sorry for the confusion here everyone. Between these three PRs, I believe this issue should be completely resolved:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@arielgreen arielgreen

@roryabraham roryabraham

@Viacheslav80 Viacheslav80

Labels
External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Milestone
No milestone
10 participants
@puneetlath @MelvinBot @parasharrajat @raeidmir @arielgreen @isagoico @roryabraham @Viacheslav80 @rdjuric @laurenreidexpensify