[Paid: Final Steps] [$1000] Web - Chat - When copying part of a multiline message, the entire message is copied

[kbecciv]

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

---

Action Performed:

1. Open the ND on the mac/Safari
2. Send multiline message
3. Select one word of the msg, click on the selected text via right button: context menu is opened
4. Click on the Copy icon
5. Paste the text

Expected Result:

Only selected text should be copied

Actual Result:

The whole message has been copied at the Safari

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Safari
• MacOS / Desktop

Version Number: 1.3.4.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

Bug6028917_Screen_Recording_2023-04-23_at_00.24.37.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01075913f2cc61e719
• Upwork Job ID: 1650506933540429824
• Last Price Increase: 2023-05-08

[MelvinBot]

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

[MelvinBot]

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

[tienifr]

Proposal

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

In Safari, when we try to select a part of a multiline message, then right click the selected text and press Copy to clipboard, it copies the whole message instead of the selected text

What is the root cause of that problem?

Let's look at the flow of the executeSecondaryInteractionOnContextMenu() function, which would be triggered when we right click a message. Basically, the current logic is blurring all active components -> showing the popover menu

App/src/components/PressableWithSecondaryInteraction/index.js

Lines 38 to 48 in 931e15f

	/**
	* This component prevents the tapped element from capturing focus.
	* We need to blur this element when clicked as it opens modal that implements focus-trapping.
	* When the modal is closed it focuses back to the last active element.
	* Therefore it shifts the element to bring it back to focus.
	* https://github.com/Expensify/App/issues/14148
	*/
	if (this.props.withoutFocusOnSecondaryInteraction && this.pressableRef) {
	this.pressableRef.blur();
	}
	this.props.onSecondaryInteraction(e);

The inconsistency lies in this piece of code. The blur() function has different behavior on Safari and Chrome. On Safari, the blur() function would de-select the user-selected text, causing the window.getSelection() function to return null, while it would not on Chrome. When the line this.pressableRef.blur() is triggered, the Safari browser would de-select the selected text, affecting the logic that was used to get the current selected text. Here, because the text was de-selected, and we are opening the popover after blurring, the window.getSelection() function would return a null component. The copied text now would be the whole message, which is similar to the behavior when we right click on the message without selecting anything.

In short, after blurring the text, the selection on Safari is cleared, while it is not on Chrome. Therefore, when the showPopover() function is triggered afterwards, the selected component is null on Safari, which make the app understand that the whole long message should be copied.

I believe this is a regression from #15375, where the blurring logic was added. A very similar issue of the selection API inconsistency after a component is blurred on Chrome and Safari (in another repo) can be found here. A possibility is that there is a difference between the way Safari and Chrome handle the blurring event.

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

Changing the order from blurring -> showPopover to showPopover -> blurring will solve this issue

-        if (this.props.withoutFocusOnSecondaryInteraction && this.pressableRef) {
-           this.pressableRef.blur();
-        }
-        this.props.onSecondaryInteraction(e);
+        this.props.onSecondaryInteraction(e);
+        if (this.props.withoutFocusOnSecondaryInteraction && this.pressableRef) {
+           this.pressableRef.blur();
+        }
      

I think this is a reasonable change, since

1. It's minimal. The changes is small.
2. It's logical. The onSecondaryInteraction() function is now using the user-selected text before blurring the current element, so we don't have to worry about any unexpected behavior that affect the user-selected text.
3. It will not re-introduce the bug that was fixed in Disable the focus trap for report page context menu #15375.

What alternative solutions did you explore? (Optional)

Alternatively, we could add a setTimeout around the blur() function. IMO this is a workaround and not a good way to do it, but given the blur() function has some kind of a small delay, and we want to preserve the selected component when the onSecondaryInteraction() is called, this is worth mentioning.

+            setTimeout(() => {
                this.pressableRef.blur();
+            }, 0)

Records

Working well after the fix

Screen.Recording.2023-04-24.at.15.30.07.mov

[conorpendergrast]

This isn't a bug: the context menu applies to all parts of the message, not individual components within the message.
eg: you can't select part of the message and delete just part of the message, or mark just part of the message as unread.

Closing as it's not a bug!

[tienifr]

@conorpendergrast
I don't agree with it. Even if it's not a bug, the behavior is inconsistent in different browsers. On Chrome or Desktop, selecting a part of the multiline text then right click -> copy would copy the selected only. Meanwhile in Safari, it would copy the whole text.

[conorpendergrast]

Oh that's very weird - I didn't spot that this was inconsistent in the OP (there's no platforms selected)

[conorpendergrast]

Reopening as this is an inconsistency that we should resolve either as copying the full message, or copying the selected part of the message

[conorpendergrast]

Interesting, I was doubly wrong! When you select text and then click Copy to Clipboard, that should copy only the selected text #2665

[conorpendergrast]

Updated the OP to include the platform!

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~01075913f2cc61e719

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[fedirjh]

@tienifr Thank you for the great explanation. However, this issue does not affect code blocks. Please check the video below. Do you have any explanation for that?

Screen.Recording.2023-04-25.at.5.18.18.PM.mov

[tienifr]

@fedirjh Thank you for the response, that’s a nice catch!

I’m not sure with the underlying principle of how Safari handle the blurring event, but after some more careful examination, here is my observation:

When we are copying a normal text on Safari, the flow would be

• Select text -> right click -> selection got cleared -> modal state is updated

In this situation, the modal selection value inside showContextMenu() function is ‘’ because the selection is already cleared when it reach that point, which will make it copy the whole message.

On the other hand, when we are copying a codeblock text, the flow would be

• Select text -> right click -> modal state is updated -> selection got cleared

In both of the situations, the window.getSelection() after everything is executed would be null.

Meanwhile, the window.getSelection() value is not cleared on Chrome

This is only happening of Safari, so IMO the root cause lay in some difference in the way Safari handle the blurring action, which may affect the order of the actual actions being executed. The order is different between when we select a pure text and when we select a code block.

[fedirjh]

The order is different between when we select a pure text and when we select a code block.

This is not addressing the why question clearly, I think we need more explanation about this difference.

[fedirjh]

After conducting additional research, I confirm that Safari sets the selection after setting the focus (as described in https://bugs.webkit.org/show_bug.cgi?id=27019#c1), which is in line with @tienifr's explanation here.

This is due to the fact WebKit sets selection after setting the focus.

@tienifr the solution you provided makes sense to me. However, it does not resolve all cases. For instance, when we select text from two messages, the selected text is not fully copied in this particular edge case.

Screen.Recording.2023-04-28.at.11.05.02.PM.mov

[tienifr]

@fedirjh Thank you for the input, and also the very helpful link!

I can confirm that this is happening on Safari (and not on Chrome). The copied text would be a single word that the pointer was hovered in when the right click event is triggered. I'm taking a deeper look into this.

[tienifr]

Apparently, the window.getSelection() result (which contains startOffset, startContainer, endOffset, endContainer) is different on Safari when we select across multiple messages (let's say message1,message2 for example) and right click. On Chrome, the startOffset and startContainer corresponds to message1, endOffset and endContainer correspond to message2. While on Safari, both start and end value for offset and container correspond to the one word that was clicked.

Still finding a proper way to address this.

[tienifr]

@fedirjh @conorpendergrast I'm still trying to fix the edge cases

[melvin-bot]

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

[tienifr]

After some investigation, I am still not able to find a resolution for this. The issue remains in the way Safari handle selection, which I have explained earlier.

I did some further digging on other messaging apps, and here is my sum up.

Action: Select text across 2 different messages -> Right click -> Copy text. Notice that we are selecting across different messages, not across lines in a single message.

Behavior of my proposal

• Chrome: the selected is copied (expected)
• Safari: the copied text is a single word being hovered before the right click event (*)

Behavior of other messaging apps

• Skype

  • Chrome: the selected is copied (expected)
  • Safari: the whole later message is copied, which does not include the text from the former text

• Slack

  • On Chrome: The text is copied, but with extra time information (not expected)
  • On Safari: same as (*)

• Linkedin

  • On Chrome: The text is copied, but with extra emojis from reaction menu (not expected)
  • On Safari: same as (*)

• Messenger

  • On Chrome: the text is copied, but with an extra “You have sent” text in the middle (not expected)
  • On Safari: same as (*)

Given that, I think this edge case should be addressed on the Webkit side, or on another issue, and it is out of scope in the current context (copying multiline text).

Thoughts?

[fedirjh]

Given that, I think this edge case should be addressed on the Webkit side

Thank you for investigating and providing an update @tienifr. I have tested Slack on Safari and can confirm that the edge cases are present there as well, which indicates that this bug is not within our app codebase. I agree with you that Webkit should address this edge case.

Given that, I think we should proceed with @tienifr's solution.

cc @jasperhuangg

🎀 👀 🎀 C+ reviewed

[conorpendergrast]

@jasperhuangg Waiting for your review to confirm and assign @tienifr

[melvin-bot]

📣 @tienifr You have been assigned to this job by @jasperhuangg!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[tienifr]

As this is a simple fix, the PR is ready for review: #18700.

[melvin-bot]

🎯 ⚡️ Woah @fedirjh / @tienifr, great job pushing this forwards! ⚡️

The pull request got merged within 2 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉

• when @tienifr got assigned: 2023-05-10 18:06:31 Z
• when the PR got merged: 2023-05-12 02:51:32 UTC

On to the next one 🚀

[conorpendergrast]

Offers sent via Upwork, will pay after the regression wait period

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.14-14 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix: Copy selection to clipboard is inconsistent between Chrome and Safari #18700

If no regressions arise, payment will be issued on 2023-05-23. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter - N/A, Applause
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@fedirjh] The PR that introduced the bug has been identified. Link to the PR:
• [@fedirjh] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@fedirjh] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@fedirjh] Determine if we should create a regression test for this bug.
• [@fedirjh] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@conorpendergrast] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/285701

[conorpendergrast]

Pay day, moving to Daily

[conorpendergrast]

Paid via Upwork.

@fedirjh Final step is the checklist!

[fedirjh]

• Link to the PR: Disable the focus trap for report page context menu #15375
• Link to comment: Disable the focus trap for report page context menu #15375 (comment)
• Link to discussion: https://expensify.slack.com/archives/C049HHMV9SM/p1684842526461499
• Regression Test

Regression Test Proposal

1. Open App on Safari (mac)
2. Send multiline message
3. Select one word of the msg, click on the selected text via right button: context menu is opened
4. Click on the Copy icon
5. Paste the text
6. Verify that only the selected text is copied

Do we agree 👍 or 👎

cc @conorpendergrast

[conorpendergrast]

I agree with those reproduction steps. Created the regression test request, and all done!