[HOLD for payment 2023-08-28] [$1000] Triple clicking text and copying to clipboard results in text copied containing button text #24156
Comments
|
Triggered auto assignment to @greg-schroeder ( |
Bug0 Triage Checklist (Main S/O)
|
ProposalPlease re-state the problem that we are trying to solve in this issue.The issue is that when triple-clicking a hidden message and copying it to the clipboard, the copied text includes parts of the message that are not intended to be copied. What is the root cause of that problem?The issue occurs because of how the text selection is currently handled in the code. It's set to copy the selected text over the message text. App/src/pages/home/report/ContextMenu/ContextMenuActions.js Lines 182 to 190 in d4cd138 What changes do you think we should make in order to solve the problem?To fix this, we need to modify how the selection is handled. Specifically, we should ensure that only the user-generated message text can be selected, not any additional UI text or elements. Based on preference, candition can be modified like |
ProposalPlease re-state the problem that we are trying to solve in this issue.Triple-click the hidden message explanation will also copy the Reveal Message button text even though it's not selectable. What is the root cause of that problem?This issue is very similar to #18343 and #17838. When we triple-click, it will select the text and the text near it, in this case, the reveal message button. The reveal message button text itself is not selectable, but it's the browser bug that makes the non-selectable text gets copied too. What changes do you think we should make in order to solve the problem?We can apply the same solution from both issues above, that is to add |
ProposalPlease re-state the problem that we are trying to solve in this issue.Triple click to text and copy to clipborad the text copied contains button text What is the root cause of that problem?
What changes do you think we should make in order to solve the problem?Add // Text.js
const propTypes = {
// ...
/** Should we make this selectable */
selectable: PropTypes.boolean,
};
const defaultProps = {
// ...
selectable: true,
};
// ReportActionItem.js
<Text
style={styles.buttonSmallText}
selectable={false}
>
{isHidden ? props.translate('moderation.revealMessage') : props.translate('moderation.hideMessage')}
</Text>The App/src/libs/SelectionScraper/index.js Line 103 in 28f4fc2 What alternative solutions did you explore? (Optional)When copying a message without the selection the button's text doesn't get copied. App/src/pages/home/report/ContextMenu/ContextMenuActions.js Lines 182 to 185 in 7b13f11 // Check if selection contains the whole message (different implementation)
if (selection >= message) {
selection = messageHtml;
} |
|
Job added to Upwork: https://www.upwork.com/jobs/~012ab20f2847f765d5 |
|
Current assignee @greg-schroeder is eligible for the External assigner, not assigning anyone new. |
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @narefyev91 ( |
|
Reproduced |
ProposalPlease re-state the problem that we are trying to solve in this issue.When selecting a reported message that has been hidden, the "Reveal Message" text and any emoji reactions are also getting copied. What is the root cause of that problem?Since the error is also reproducible when using Ctrl + C, I was able to rule out anything to do with the Copy action inside the Context Menu. This code overloads the Ctrl + C shortcut and uses the SelectionScraper to fill the clipboard. And after extensive digging into the code, I believe this line is the problem: App/src/libs/SelectionScraper/index.js Line 86 in aa5483d The SelectionScraper eliminates the data-selection-scrapper-hidden-element="true" items, but this is true only on hover. I still need to find where else is the true and false being manipulated. In the React code, it is manipulated by the following code, but I need to follow the chain to see where it is initially set and why: The initial implementation of UnreadActionIndicator is probably the root cause of where this problem first showed up. What changes do you think we should make in order to solve the problem?A simple and quick bug fix will be to remove A long-term solution is to identify where the selection-scrapper-hidden-element is being added, why and what all code is manipulating it. A catch-all removal is not a sufficient solution. What alternative solutions did you explore? (Optional)I looked at all the Context Menu code, the ExpensiMark code and the parsing code. Since the error was exactly reproducible when using the keyboard shortcut, it meant the context menu code was not the issue. |
|
📣 @swakeert! 📣
|
|
Contributor details |
|
✅ Contributor details stored successfully. Thank you for contributing to Expensify! |
|
Proposal from @bernhardoj looks good to me #24156 (comment) - we already have the same fixes for other situations which are closely to this one. Let's fix it in the same manner |
|
Triggered auto assignment to @jasperhuangg, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@narefyev91 Pardon me if this seems naive or if I'm asking something obvious. The issue arises when you select text and then copy it by right-clicking; it copies the selection instead of the original hidden message. The expected result is that it should copy the user's message. Hence, I suggested we prioritize copying the user's message, or if preferred, only do so in flagged cases. If I understand correctly, @bernhardoj suggests that users shouldn't be able to select text at all. So, is that what you're proposing: to prevent users from selecting text in the first place? |
|
The button text itself is already unselectable App/src/pages/home/report/ReportActionItem.js Lines 342 to 347 in 4e050d2 but you can still copy it by triple-clicking the text near it or dragging the selection over the button text |
|
@bernhardoj did you try using It is an issue with the selection function and what it decided to omit. EDIT: can't instead of can. |
|
Apologies for the delay! I agree that we should go with @bernhardoj's solution, it seems like it closely mirrors solutions we have implemented for this problem in other places. |
|
📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
|
📣 @dukenv0307 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app! |
|
PR is ready |
|
🎯 ⚡️ Woah @narefyev91 / @bernhardoj, great job pushing this forwards! ⚡️ The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉
On to the next one 🚀 |
|
|
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.55-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2023-08-28. 🎊 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.
For reference, here are some details about the assignees on this issue:
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
|
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:
|
|
|
Thanks @narefyev91 - today is payment date so working through that now. |
|
Issue Participants: Issue reported by: @dukenv0307 Was this issue merged in time to be eligible for the speed bonus? Yes Payment summary: Reporter: $250 |
|
Payments made, checklist complete. Closing. |
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:
Expected Result:
The message copied shouldn't contain reveal button text
Actual Result:
The message copied contains reveal button text
Workaround:
Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.50-1
Reproducible in staging?: y
Reproducible in production?: y
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
Screen.Recording.2023-08-01.at.17.19.37.mov
Recording.1445.mp4
Expensify/Expensify Issue URL:
Issue reported by: @dukenv0307
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690886867471159
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: