[HOLD for payment 2022-11-29] [$1000] BUG: The text inside the composer input and the menu on the settings page got highlighted when triple-click on the last report chat reported by @mollfpr

[kavimuru]

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:

1. Go to the composer input and type something
2. Triple-click to highlight the text of the last report chat
3. Click the account avatar to open the settings modal in the rightDocked side modal

Expected Result:

Only the intended text selection is highlighted.

Actual Result:

The intended text select, text inside the composer input, and the menu on the settings page are highlighted.

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web
• Desktop App

Version Number: 1.2.18-2
Reproducible in staging?: y
Reproducible in production?: y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Recording.733.mp4

Screen.Recording.2022-10-19.at.21.10.57.mov

Upwork URL: https://www.upwork.com/jobs/~019c6881ba683194f2
Issue reported by: @mollfpr
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1666188981167469

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @trjExpensify (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[trjExpensify]

I can reproduce this reliably on v1.2.18-4. Marking as external.

[trjExpensify]

Job on Upwork here: https://www.upwork.com/jobs/~019c6881ba683194f2

[hungvu193]

Proposal
Disable the text selection highlighting can help us fix this problem.

Solution
Apply this style to the ScreenWrapper of InitialSettingPage:

- <ScreenWrapper >
+  <ScreenWrapper style={[styles.settingsPageWrapper]}>
                <HeaderWithCloseButton
                    title={this.props.translate('common.settings')}
                    onCloseButtonPress={() => Navigation.dismissModal(true)}
                />

+settingsPageWrapper: {
+      MozUserSelect: "none",
+      WebkitUserSelect: "none",
+      MsUserSelect: "none",
+}

We also need to update this style to other screen in this page too (Workspace, profile, Preference....)

Screen.Recording.2022-10-22.at.19.09.39.mov

[melvin-bot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[melvin-bot]

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

[melvin-bot]

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

[thesahindia]

@hungvu193, the text inside the composer still gets selected. I think your solution is a workaround. We need to find the root cause.

[wildan-m]

Proposal

Root cause

I think it's related to how chrome behaves with a triple click. They have a related bug with status won't fix
https://bugs.chromium.org/p/chromium/issues/detail?id=575136

Workaround

My proposed solution is also a workaround, but let me know if it's acceptable.

The idea is to override the selection with the desired contents.

We can wrap ReportActionItemFragment with a div and listen triple-click events to programmatically re-select the text contents.

git diff src/pages/home/report/ReportActionItemMessage.js
diff --git a/src/pages/home/report/ReportActionItemMessage.js b/src/pages/home/report/ReportActionItemMessage.js
index d226ac9ced..af88ac4e5e 100644
--- a/src/pages/home/report/ReportActionItemMessage.js
+++ b/src/pages/home/report/ReportActionItemMessage.js
@@ -26,18 +26,33 @@ const defaultProps = {
     style: [],
 };
 
+const selectElementContents = (el) => {
+    const sel = window.getSelection();
+    sel.removeAllRanges();
+    const range = document.createRange();
+    range.selectNodeContents(el);
+    sel.addRange(range);
+};
+
+const onClick = (e) => {
+    if (e.detail !== 3) { return; }
+    selectElementContents(e.target);
+};
+
 const ReportActionItemMessage = props => (
     <View style={[styles.chatItemMessage, ...props.style]}>
         {_.map(_.compact(props.action.previousMessage || props.action.message), (fragment, index) => (
-            <ReportActionItemFragment
-                key={`actionFragment-${props.action.sequenceNumber}-${index}`}
-                fragment={fragment}
-                isAttachment={props.action.isAttachment}
-                attachmentInfo={props.action.attachmentInfo}
-                source={lodashGet(props.action, 'originalMessage.source')}
-                loading={props.action.isLoading}
-                style={props.style}
-            />
+            <div role="presentation" onClick={onClick}>
+                <ReportActionItemFragment
+                    key={`actionFragment-${props.action.sequenceNumber}-${index}`}
+                    fragment={fragment}
+                    isAttachment={props.action.isAttachment}
+                    attachmentInfo={props.action.attachmentInfo}
+                    source={lodashGet(props.action, 'originalMessage.source')}
+                    loading={props.action.isLoading}
+                    style={props.style}
+                />
+            </div>
         ))}
     </View>
 );

I know this solution will still have the wrong selection at first, but it will immediately correct the text selection. Please have a look at the video.

Recording.42.mp4

[thesahindia]

I think it's related to how chrome behaves with a triple click. They have a related bug with status won't fix

@wildan-m, the issue can be seen on other browsers as well.

Waiting for more proposals.

[wildan-m]

Proposal (Updated)

@thesahindia, you are right, I can also reproduce it in Safari.

I still believe that it's caused by the different triple-click behavior of browsers
At this time, behavior of default action of triple-click isn't specified the W3C spec
Other repos also experience similar issues:
facebook/lexical#2660
ckeditor/ckeditor4#484
ckeditor/ckeditor4#3118

I've resolved glitches in my previous solution by changing the onClick event to onMouseDown and adding preventDefault if the subsequent click is more than two.

App/src/pages/home/report/ReportActionItemMessage/index.web.js

Lines 10 to 40 in 7392208

	const selectElementContents = (el) => {
	const sel = window.getSelection();
	sel.removeAllRanges();
	const range = document.createRange();
	range.selectNodeContents(el);
	sel.addRange(range);
	};
	const onMouseDown = (e) => {
	if (e.detail < 3) { return; }
	e.preventDefault();
	selectElementContents(e.target);
	};
	const ReportActionItemMessage = props => (
	<View style={[styles.chatItemMessage, ...props.style]}>
	{_.map(_.compact(props.action.previousMessage || props.action.message), (fragment, index) => (
	<div key={`view-${props.action.sequenceNumber}-${index}`} role="presentation" onMouseDown={onMouseDown}>
	<ReportActionItemFragment
	key={`reportAction-${props.action.sequenceNumber}-${index}`}
	fragment={fragment}
	isAttachment={props.action.isAttachment}
	attachmentInfo={props.action.attachmentInfo}
	source={lodashGet(props.action, 'originalMessage.source')}
	loading={props.action.isLoading}
	style={props.style}
	/>
	</div>
	))}
	</View>
	);

These changes will be implemented for the web and desktop only.

Complete comparison:
7392208

No more flickering:

Recording.43.mp4

[thesahindia]

I don't prefer adding another wrapper, I wanna wait to see if someone can find the root cause and propose a more simpler solution.

[trjExpensify]

@thesahindia presumably you believe this is a regression, right?

I still believe that it's caused by the different triple-click behavior of browsers

@wildan-m, we see the issue on the desktop app as well (not just in the Web browser), so how can that be the case?

[thesahindia]

What I was trying to understand is why user-select: none is causing this issue just to be sure about the root cause, I tested this at codepen and now I don't have doubts about it.

@wildan-m, can you please test that your solution will work on all platforms?

[wildan-m]

@thesahindia
I've found chromium code that causes and might fix this behavior:
chromium/chromium@31a47fc

It's committed 16 days ago. It might be merged into chrome but I'm not sure when.

Forgot to add WebkitUserSelect: auto for Safari:

git diff src/styles/styles.js
diff --git a/src/styles/styles.js b/src/styles/styles.js
index f2d7cff023..ff418ef535 100644
--- a/src/styles/styles.js
+++ b/src/styles/styles.js
@@ -1236,6 +1236,8 @@ const styles = {
     appContent: {
         backgroundColor: themeColors.appBG,
         overflow: 'hidden',
+        userSelect: 'auto',
+        WebkitUserSelect: 'auto',
     },
 
     appContentHeader: {

I've tested it on the web (chrome, FF, Safari), and desktop and the result is OK.
In Android, it doesn't create any defects thus far. Haven't tested it on ios yet, but I think it will be OK too.

Note:
There is a newly reported issue that prevents us to use Cmd+C in safari. Temporarily we can use the edit menu to copy/paste

[wildan-m]

@thesahindia just tested it on iOS. The result is OK

[thesahindia]

The codepen example has the same issue so it's confirmed that user-select: 'none' is causing this as mentioned here
I like @wildan-m's proposal, the styles will be passed at ScreenWrapper

App/src/pages/home/ReportScreen.js

Lines 235 to 236 in e09d228

	<ScreenWrapper
	style={screenWrapperStyle}

C+ reviewed 🎀👀🎀

cc: @chiragsalian

[chiragsalian]

Proposal LGTM, feel free to create the PR @wildan-m.

[melvin-bot]

📣 @wildan-m You have been assigned to this job by @chiragsalian!
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 📖

[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:

• @trjExpensify A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here: N/A. Minor edge case that isn't worth adding a regression test for.
• @thesahindia @chiragsalian The PR that introduced the bug has been identified. Link to the PR:
• @trjExpensify 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:
• @trjExpensify A discussion in #contributor-plus 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:
• @trjExpensify Payment has been made to the issue reporter (if applicable)
• @trjExpensify Payment has been made to the contributor that fixed the issue (if applicable)
• @trjExpensify Payment has been made to the contributor+ that helped on the issue (if applicable)

[trjExpensify]

Alrighty, time to settle up! Payments due:

$1000 - @wildan-m (C) - offer sent
$1000 - @thesahindia (C+) - settled 👍
$500 - @mollfpr for reporting this issue & #11921 - settled 👍

[trjExpensify]

As for the checklist:

1. We don't need to add a manual regression test for this minor edge case
2. Where the regression was introduced is known (i.e the drawer navigation lib upgrade)
3. The engineer that spearheaded the upgrade is aware, as we had a discussion about it in Slack already.

[trjExpensify]

Alrighty, @wildan-m settled up! Closing, thanks everyone! 🎉

[thesahindia]

@trjExpensify, I think it's also eligible for the 50% bonus 😄

[trjExpensify]

Oh, bloody hell! 😅 Sent you both contracts for that.

[trjExpensify]

Cool, all sorted.