fix: scroll to right position on text insertion #9062

b1tjoy · 2022-05-18T01:18:06Z

Details

Text Insertion point goes out of the sight on typing in the middle of the paragraph. This PR fix the issue. And in order to fix #8663, we have to delete one line of code, which was add to fix #7351 previous, so this PR alse fix that one.

Why move textInput.blur() and textInput.focus() from ReportActionCompose.js to Composer/index.js?

Because these two lines was add to fix #7351, but the issue only affects Web, mWeb and Desktop platform, and only in pasting text copied from HTML page scenario, if we keep the code in ReportActionCompose.js, it will cause other issues, such as:

Virtual keyboard keeps collapse and pop out on Android native when we type text in input field
Input field keeps flash on Web when we type text

So I moved these codes to Composer/index.js:handlePasteHTML().

Fixed Issues

$ #8663

Tests

Test case for #8663:

Open Expensify account
Select any chat room in your list
Write multiple lines of text, 20+ lines for web, 10+ lines for mWeb
Type some words on the 4th-5th line
Verify text insertion point does not move out of sight

Test case for #7351:

Open Expensify account
Select any chat room in your list
Paste a large paragraph copy from HTML page, 20+ lines for web, 10+ lines for mWeb
Verify the scroll bar scroll down to the end of input field and text insertion point remains in sight

Test case for paste on selection, a more complex scenario:

Open Expensify account
Select any chat room in your list
Write multiple lines of text, 20+ lines for web, 10+ lines for mWeb
Copy a large parapragh from HTML page
Select some text from chat input field and paste
Verify the text insertion point remains in sight

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Test case for #8663:

Open Expensify account
Select any chat room in your list
Write multiple lines of text, 20+ lines for web, 10+ lines for mWeb
Type some words on the 4th-5th line
Verify text insertion point does not move out of sight

Test case for #7351:

Open Expensify account
Select any chat room in your list
Paste a large paragraph copy from HTML page, 20+ lines for web, 10+ lines for mWeb
Verify the scroll bar scroll down to the end of input field and text insertion point remains in sight

Test case for paste on selection, a more complex scenario:

Open Expensify account
Select any chat room in your list
Write multiple lines of text, 20+ lines for web, 10+ lines for mWeb
Copy a large parapragh from HTML page
Select some text from chat input field and paste
Verify the text insertion point remains in sight

Verify that no errors appear in the JS console

Screenshots

Web

Test case for #8663

Screen.Recording.2022-05-22.at.10.49.11-MacOS-Chrome-1.mp4

Test case for #7351

Screen.Recording.2022-05-22.at.10.50.23-MacOS-Chrome-2.mp4

Test case for paste on selection, a more complex scenario

Screen.Recording.2022-05-22.at.10.52.02-MacOS-Chrome-3.mp4

Mobile Web

Test case for #8663 Android/Chrome

Screenshot-8663-Android-chrome-1.mp4

Test case for #7351 Android/Chrome

Screenshot-8663-Android-chrome-2.mp4

Test case for paste on selection, a more complex scenario Android/Chrome

Screenshot-8663-Android-chrome-3.mp4

Test case for #8663 iOS/Safari

Simulator.Screen.Recording.-.iPhone.13.-.2022-05-22.at.11.37.26-iOS-safari-1.mp4

Test case for #7351 iOS/Safari

Simulator.Screen.Recording.-.iPhone.13.-.2022-05-22.at.11.39.18-iOS-safari-2.mp4

Test case for paste on selection, a more complex scenario iOS/Safari

Simulator.Screen.Recording.-.iPhone.13.-.2022-05-22.at.11.40.46-iOS-safari-3.mp4

Desktop

Test case for #8663

Screen.Recording.2022-05-22.at.10.55.34-MacOS-Desktop-1.mp4

Test case for #7351

Screen.Recording.2022-05-22.at.10.58.01-MacOS-Desktop-2.mp4

Test case for paste on selection, a more complex scenario

Screen.Recording.2022-05-22.at.10.59.00-MacOS-Desktop-3.mp4

iOS

Test case for #8663

Simulator.Screen.Recording.-.iPhone.13.-.2022-05-22.at.11.06.48-iOS-native-1.mp4

Test case for #7351

Simulator.Screen.Recording.-.iPhone.13.-.2022-05-22.at.11.10.35-iOS-native-2.mp4

Test case for paste on selection, a more complex scenario

Simulator.Screen.Recording.-.iPhone.13.-.2022-05-22.at.11.12.47-iOS-native-3.mp4

Android

Test case for #8663

Screenshot-20220522084200-Android-native-1.mp4

Test case for #7351

Screenshot-20220522084700-Android-native-2.mp4

Test case for paste on selection, a more complex scenario

Screenshot-20220522085300-Android-native-3.mp4

github-actions · 2022-05-18T01:18:25Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

b1tjoy · 2022-05-22T14:46:29Z

I have read the CLA Document and I hereby sign the CLA

parasharrajat

Thanks for the detailed PR.

parasharrajat · 2022-05-23T16:17:32Z

src/components/Composer/index.js

@@ -244,6 +244,8 @@ class Composer extends React.Component {
        try {
            document.execCommand('insertText', false, markdownText);
            this.updateNumberOfLines();
+            this.textInput.blur();


A comment will be useful.

Comment added.

parasharrajat · 2022-05-24T16:27:00Z

The review of this PR will be delayed. I need to run a few tests. The current solution is also a hack to fix the cursor. I will try to find a better solution to the problem.

b1tjoy · 2022-05-25T12:52:43Z

The review of this PR will be delayed. I need to run a few tests. The current solution is also a hack to fix the cursor. I will try to find a better solution to the problem.

@parasharrajat I agree with you, if we use a hack to fix one issue at the current moment, it may lead to another issue in the future. Always find the root cause.

First I thought it was related to react-native-web, but after some digging, it may be problem with Chrome. I write snippets with basic JS to show how document.execCommand('insertText') works:

Text Insertion Example

If you run with Chrome, when you paste a large paragraph, the text insertion point goes out of sight. But if you run the example with Firefox, everything works fine.

parasharrajat · 2022-06-01T21:26:50Z

Thanks for waiting. I will try to steal some time from my schedule to do some research on this issue.

parasharrajat · 2022-06-05T20:03:47Z

No update yet. But I will share some update very soon. On my main list.

parasharrajat · 2022-06-10T16:57:32Z

@b1tjoy Thank you for your patience. I was looking into your other proposals. And it seems that this #8663 (comment) could be the next potential solution.

You mentioned that this is working fine. Was there any issues with it?

b1tjoy · 2022-06-11T16:07:23Z

@parasharrajat When we select text from bottom to top, then paste the content copy from html page, the text insertion point moves out of sight.

Screencast.from.06-11-2022.11.49.27.PM.mp4

parasharrajat · 2022-06-13T14:06:00Z

Ok, I am trying to determine the expected behavior when we paste text in textArea. There are a few case possible for pasting. I will try to list them down and test your solution over it.

b1tjoy · 2022-06-21T04:37:39Z

Ok, I am trying to determine the expected behavior when we paste text in textArea. There are a few case possible for pasting. I will try to list them down and test your solution over it.

@parasharrajat Possible cases for pasting I thought of:

textArea is empty, paste text more than 20 lines
textArea not empty, no selection in textArea, paste in the beginning, middle or end of textArea
textArea not empty, select partial text from top to bottom in the beginning, middle or end of textArea, then paste
textArea not empty, select partial text from bottom to top in the beginning, middle or end of textArea, then paste

I don't want to be bothersome and I know you are pretty busy, could you please take some time to review this PR for me? As a new contributor, I am not allowed to work on other issues for now, although I have submitted good proposal. Thanks a lot! :)

parasharrajat · 2022-06-21T20:09:25Z

Sorry for the delay. I just kind of lost this one. Adding it to my list now. I will get to the bottom of it in a day or two.

Anyways, you should be good for new tasks. You can ping again on the same issue.

parasharrajat · 2022-06-24T21:21:09Z

Ok, I tested your #8663 (comment) proposal and it seems to be working. Could you do the following and run a few tests? Otherwise, I can go with the existing solution.

Add a state update callback as second param and add these statements to the callback.

 const heightToBottom = this.textInput.scrollHeight - paddingTopAndBottom - this.textInput.scrollTop;
                this.textInput.scrollTop = this.textInput.scrollHeight - paddingTopAndBottom - heightToBottom;

Now run your tests.

parasharrajat · 2022-06-24T21:47:32Z

Also, please merge main.

b1tjoy · 2022-06-26T14:29:52Z

I don't think it's going to work. The main idea of #8663 (comment) is to store the height from the insertion point to the last line of textArea before document.execCommand('insertText'), after text insertion we can calculate scrollTop by the stored value and manually scroll the textArea to the appropriate position. This works fine for the following cases:

textArea is empty, paste text more than 20 lines
textArea not empty, no selection in textArea, paste in the beginning, middle or end of textArea
textArea not empty, select partial text from top to bottom in the beginning, middle or end of textArea, then paste

If we have 40 lines text for example, line height is 20, maxLines = 16, select from line 11 to 30, and paste 30 lines text:

Before insert:
scrollHeight = 800
selection lines (select from top to bottom) = [11, 30]
insertion point line = 30
visible area lines = [15, 30]
upper lines not visible = [1, 14]
lower lines not visible = [31, 40]
scrollTop = 14 * 20 = 280
heightToBottom = 800 - 280 = 520

After insert:
scrollHeight = 1000
scrollTop = 1000 - 520 = 480 = 24 * 20
upper lines not visible = [1, 24]
visible area lines = [25, 40]
lower lines not visible = [41, 50]
insertion point line = 40 (because we replace line 11-30 with 30 lines text)

Insertion point is in the visible area and everything works fine.

But in some edge case it doesn't work:

textArea not empty, select partial text from bottom to top (more than 16 lines and make scroll bar goes backward) in textArea, then paste

If we have 40 lines text for example, line height is 20, maxLines = 16, select from line 30 to 11, and paste 30 lines text:

Before insert:
scrollHeight = 800
selection lines (select from bottom to top) = [30, 11]
insertion point line = 30
visible area lines = [11, 26]
upper lines not visible = [1, 10]
lower lines not visible = [27, 40]
scrollTop = 10 * 20 = 200
heightToBottom = 800 - 200 = 600

After insert:
scrollHeight = 1000
scrollTop = 1000 - 600 = 400 = 20 * 20
upper lines not visible = [1, 20]
visible area lines = [21, 36]
lower lines not visible = [37, 50]
insertion point line = 40 (because we replace line 11-30 with 30 lines text)

Insertion point is not in the visible area.

I think we can combine these two proposals, use the calculation one to handle regular case, and use blur() & focus() one to handle select backward case. We can add a variable to track if user scroll backward, if user scroll backward and selection is not empty, call blur() & focus() to keep insertion point insight, otherwise calculate scrollTop and manually scroll to appropriate position. How about that?

parasharrajat · 2022-06-27T22:25:43Z

A slight difference from your proposal is that I am not tracking the scroll before paste. Instead, I am relying on the reset of numberOfLines to 1.

Anyways, I see your analysis. Testing the current solution in this PR. I would say that if are not able to set a manual scroll position then this solution is fine. I will open the discussion on slack to get more eyes before moving ahead with the PR.

It would be done by tomorrow.

parasharrajat · 2022-06-27T22:26:03Z

Meanwhile please merge main into this PR.

b1tjoy · 2022-07-01T15:45:49Z

A slight difference from your proposal is that I am not tracking the scroll before paste. Instead, I am relying on the reset of numberOfLines to 1.

@parasharrajat After run a few tests, I think that reset numberOfLines to 1 does not make textarea scroll to appropriate position. Do you get the same result?

parasharrajat · 2022-07-01T17:54:20Z

Yeah, I saw that the current solution of the PR has better results. I will open the discussion today.

parasharrajat

@b1tjoy Please merge main into this.

parasharrajat · 2022-07-04T17:10:10Z

src/components/Composer/index.js

@@ -239,6 +239,10 @@ class Composer extends React.Component {
        try {
            document.execCommand('insertText', false, markdownText);
            this.updateNumberOfLines();
+            // Manually make text input scroll to appropriate position, otherwise the text insertion
+            // point will goes out of sight when we paste a large paragraph on Chrome or Desktop


Suggested change

// point will goes out of sight when we paste a large paragraph on Chrome or Desktop

// Pointer will go out of sight when a large paragraph is pasted on the web. Refocusing the input keeps the cursor in view.

parasharrajat · 2022-07-04T17:10:41Z

src/components/Composer/index.js

@@ -239,6 +239,10 @@ class Composer extends React.Component {
        try {
            document.execCommand('insertText', false, markdownText);
            this.updateNumberOfLines();
+            // Manually make text input scroll to appropriate position, otherwise the text insertion


Suggested change

// Manually make text input scroll to appropriate position, otherwise the text insertion

parasharrajat

LGTM.

cc: @pecanoro

PR Reviewer Checklist

OSBotify · 2022-07-05T16:52:35Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-07-08T01:34:03Z

🚀 Deployed to staging by @pecanoro in version: 1.1.81-0 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	failure ❌

OSBotify · 2022-07-13T01:20:18Z

🚀 Deployed to production by @roryabraham in version: 1.1.82-5 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

fix: scroll to right position on text insertion

efea0bb

move text input blur and focus code to Composer

d97bb0b

b1tjoy marked this pull request as ready for review May 22, 2022 14:45

b1tjoy requested a review from a team as a code owner May 22, 2022 14:45

melvin-bot bot requested review from parasharrajat and pecanoro and removed request for a team May 22, 2022 14:45

parasharrajat suggested changes May 23, 2022

View reviewed changes

add comment for new code

f84d164

Merge branch 'Expensify:main' into fix/8663-scroll-on-insertion

780d767

parasharrajat suggested changes Jul 4, 2022

View reviewed changes

Merge branch 'Expensify:main' into fix/8663-scroll-on-insertion

9f33935

update comment and fix lint error

e5ad3f7

b1tjoy requested a review from parasharrajat July 5, 2022 14:45

parasharrajat approved these changes Jul 5, 2022

View reviewed changes

pecanoro approved these changes Jul 5, 2022

View reviewed changes

pecanoro merged commit 4774898 into Expensify:main Jul 5, 2022

roryabraham mentioned this pull request Jul 8, 2022

Deploy Checklist: New Expensify 2022-07-07 #9781

Closed

b1tjoy mentioned this pull request Jul 8, 2022

[HOLD for payment 2023-04-03] [HOLD for payment 2022-08-11] [$500] Web - Chat - Avatar icon is flickers when the message is sent for the first time #9183

Closed

parasharrajat mentioned this pull request Jan 9, 2023

perf: fix (comment) input performance #11684

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: scroll to right position on text insertion #9062

fix: scroll to right position on text insertion #9062

b1tjoy commented May 18, 2022 •

edited

Loading

github-actions bot commented May 18, 2022 •

edited

Loading

b1tjoy commented May 22, 2022

parasharrajat left a comment

parasharrajat May 23, 2022

b1tjoy May 24, 2022

parasharrajat commented May 24, 2022

b1tjoy commented May 25, 2022 •

edited

Loading

parasharrajat commented Jun 1, 2022

parasharrajat commented Jun 5, 2022 •

edited

Loading

parasharrajat commented Jun 10, 2022

b1tjoy commented Jun 11, 2022

parasharrajat commented Jun 13, 2022

b1tjoy commented Jun 21, 2022

parasharrajat commented Jun 21, 2022

parasharrajat commented Jun 24, 2022

parasharrajat commented Jun 24, 2022

b1tjoy commented Jun 26, 2022

parasharrajat commented Jun 27, 2022

parasharrajat commented Jun 27, 2022

b1tjoy commented Jul 1, 2022

parasharrajat commented Jul 1, 2022

parasharrajat left a comment

parasharrajat Jul 4, 2022

parasharrajat Jul 4, 2022

parasharrajat left a comment

OSBotify commented Jul 5, 2022

OSBotify commented Jul 8, 2022

OSBotify commented Jul 13, 2022

	// point will goes out of sight when we paste a large paragraph on Chrome or Desktop
	// Pointer will go out of sight when a large paragraph is pasted on the web. Refocusing the input keeps the cursor in view.

fix: scroll to right position on text insertion #9062

fix: scroll to right position on text insertion #9062

Conversation

b1tjoy commented May 18, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

github-actions bot commented May 18, 2022 • edited Loading

b1tjoy commented May 22, 2022

parasharrajat left a comment

Choose a reason for hiding this comment

parasharrajat May 23, 2022

Choose a reason for hiding this comment

b1tjoy May 24, 2022

Choose a reason for hiding this comment

parasharrajat commented May 24, 2022

b1tjoy commented May 25, 2022 • edited Loading

parasharrajat commented Jun 1, 2022

parasharrajat commented Jun 5, 2022 • edited Loading

parasharrajat commented Jun 10, 2022

b1tjoy commented Jun 11, 2022

parasharrajat commented Jun 13, 2022

b1tjoy commented Jun 21, 2022

parasharrajat commented Jun 21, 2022

parasharrajat commented Jun 24, 2022

parasharrajat commented Jun 24, 2022

b1tjoy commented Jun 26, 2022

parasharrajat commented Jun 27, 2022

parasharrajat commented Jun 27, 2022

b1tjoy commented Jul 1, 2022

parasharrajat commented Jul 1, 2022

parasharrajat left a comment

Choose a reason for hiding this comment

parasharrajat Jul 4, 2022

Choose a reason for hiding this comment

parasharrajat Jul 4, 2022

Choose a reason for hiding this comment

parasharrajat left a comment

Choose a reason for hiding this comment

PR Reviewer Checklist

OSBotify commented Jul 5, 2022

OSBotify commented Jul 8, 2022

OSBotify commented Jul 13, 2022

b1tjoy commented May 18, 2022 •

edited

Loading

github-actions bot commented May 18, 2022 •

edited

Loading

b1tjoy commented May 25, 2022 •

edited

Loading

parasharrajat commented Jun 5, 2022 •

edited

Loading