Web - Composer - Cursor positioned at the start if pasting text which contains a new line #42892

moonstar-95515 · 2024-05-31T03:15:31Z

Details

Web - Composer - Cursor positioned at the start if pasting text which contains a new line

Fixed Issues

$ #42216
PROPOSAL: #42216 (comment)

Tests

Navigate to staging.new.expensify.com
Open a chat and type a one line text and create an empty new line
Select all using keyboard ctrl + A and cut the text
Paste it inside the compose box

Verify the cursor is positioned at the end of the pasted text

Verify that no errors appear in the JS console

Offline tests

QA Steps

Navigate to staging.new.expensify.com
Open a chat and type a one line text and create an empty new line
Select all using keyboard ctrl + A and cut the text
Paste it inside the compose box

Verify the cursor is positioned at the end of the pasted text

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

android.mp4

Android: mWeb Chrome

android.-chrome.mp4

iOS: Native

ios-native.mp4

iOS: mWeb Safari

bandicam.2024-06-11.17-34-50-999.mp4

MacOS: Chrome / Safari

web.mp4

MacOS: Desktop

bandicam.2024-06-11.17-03-26-854.mp4

melvin-bot · 2024-05-31T03:15:36Z

@eVoloshchak Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

github-actions · 2024-05-31T03:15:47Z

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

moonstar-95515 · 2024-05-31T03:16:14Z

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

eVoloshchak · 2024-06-03T16:05:15Z

src/hooks/useHtmlPaste/index.ts

@@ -16,8 +16,9 @@ const insertAtCaret = (target: HTMLElement, text: string) => {
        range.insertNode(node);

        // Move caret to the end of the newly inserted text node.
-        range.setStart(node, node.length);
-        range.setEnd(node, node.length);
+        const offset = (text.length && text[text.length - 1] === '\n' ? node.length - 1 : node.length);


Suggested change

const offset = (text.length && text[text.length - 1] === '\n' ? node.length - 1 : node.length);

const offset = text?.endsWith('\n') ? node.length - 1 : node.length;

eVoloshchak · 2024-06-03T16:07:19Z

src/hooks/useHtmlPaste/index.ts

@@ -16,8 +16,9 @@ const insertAtCaret = (target: HTMLElement, text: string) => {
        range.insertNode(node);

        // Move caret to the end of the newly inserted text node.
-        range.setStart(node, node.length);
-        range.setEnd(node, node.length);
+        const offset = (text.length && text[text.length - 1] === '\n' ? node.length - 1 : node.length);


Could you add a comment explaining why this logic is needed and include a link to the issue?

moonstar-95515 · 2024-06-03T16:26:46Z

Hi, @eVoloshchak
I've pushed updates.

eVoloshchak · 2024-06-04T15:04:50Z

src/hooks/useHtmlPaste/index.ts

@@ -16,8 +16,12 @@ const insertAtCaret = (target: HTMLElement, text: string) => {
        range.insertNode(node);

        // Move caret to the end of the newly inserted text node.
-        range.setStart(node, node.length);
-        range.setEnd(node, node.length);
+        // we have to set offset as `node.length - 1` when text has empty line at the end


Suggested change

// we have to set offset as `node.length - 1` when text has empty line at the end

// Correctly calculate text length since newline character does not create a new line in HTML

// https://github.com/Expensify/App/issues/42216

eVoloshchak · 2024-06-04T15:09:39Z

@moonstar-95515, looks awesome, there are a couple of steps left to finish up the author checklist:

include screen recordings for all of the platforms
include the final step in Tests section: "Verify the cursor is positioned at the end of the pasted text"
Include QA Steps (should be the same as regular Tests)

moonstar-95515 · 2024-06-04T15:16:45Z

@eVoloshchak
I tried to login in local server. But it constantly get this error. Is it working fine on your side?

eVoloshchak · 2024-06-04T15:30:03Z

@moonstar-95515, works fine for me (a little slow though)
Try pulling the latest main and clearing cookies and site data

moonstar-95515 · 2024-06-10T16:51:03Z

@eVoloshchak
Sorry for delay but I still have problem with login.

eVoloshchak · 2024-06-10T17:02:55Z

@moonstar-95515, try doing this

Screen.Recording.2024-06-10.at.19.01.27.mov

moonstar-95515 · 2024-06-10T22:07:27Z

@eVoloshchak
Thank you. I tried this but it doesn't help.

eVoloshchak · 2024-06-11T12:37:30Z

@moonstar-95515, did you pull the latest changes from main?
It should be working, just verified

moonstar-95515 · 2024-06-11T14:50:24Z

@eVoloshchak
Thank you. I merged latest main to my working branch and it works.
I've uploaded videos for ios and desktop.

eVoloshchak · 2024-06-11T14:57:15Z

I merged latest main to my working branch and it works

@moonstar-95515, could you push this commit so I can test this PR with the latest changes?
Also, screen recordings for Android: Native and Android: mWeb Chrome are missing

moonstar-95515 · 2024-06-12T06:27:03Z

@eVoloshchak
I've pushed updates.
For android, I have some problems as well. Is there any possibility to go further with current status?
Thank you in advance.

eVoloshchak · 2024-06-13T13:56:11Z

Is there any possibility to go further with current status?

It is much preferred if we both double-check all of the platforms, let's try to get this working on Android for you. What problem are you having?

moonstar-95515 · 2024-06-13T15:36:32Z

@eVoloshchak

It is much preferred if we both double-check all of the platform

Thank you I got it.
But unfortunately, my MacOS is borken unexpectedly and it seems it's difficult to make it work soon.
Sorry for inconvenience.

eVoloshchak · 2024-06-13T15:39:46Z

@moonstar-95515, check out our TESTING_MACOS_AND_IOS.md guide

kpadmanabhan · 2024-06-14T19:43:46Z

I was working on this issue initially. When my PR was in progress, proposal for this PR was picked up. Based on @jayeshmangwani comment, ideally this PR and proposal should have been withdrawn. My PR is on HOLD for quite some days now.

However, as @moonstar-95515 also suggested a good alternative solution for the issue, but now that they are stuck with continuing, I am ok with taking this forward in whichever way Expensify sees it logical as per code of conduct.

@jayeshmangwani / @roryabraham / @eVoloshchak / @slafortune / @mallenexpensify : kindly suggest way forward.

moonstar-95515 · 2024-06-17T14:36:54Z

I'm sorry that I have some issues with recording test video but I'm still working on this issue.

@kpadmanabhan
I really appreciate your effort in your PR but I think your PR is hold due to regression.
I've almost completed this PR.

kpadmanabhan · 2024-06-17T15:01:30Z

@moonstar-95515 : thanks for the update. Will await completion of your PR.

eVoloshchak · 2024-06-24T12:30:50Z

@moonstar-95515, were you able to get your development environment working?

moonstar-95515 · 2024-06-25T00:28:28Z

@eVoloshchak
Sorry for delay.
I'm building android environment.
But I think I tried every solution but I was not able to run emulator.
Is it possible to run android version on VM?

eVoloshchak · 2024-06-27T17:27:45Z

Is it possible to run android version on VM?

@moonstar-95515, it is possible
Also, TESTING_MACOS_AND_IOS.md tells how you can use a macOS instance (real mac that you use through remote desktop)

Could you share which problems you're having exactly?

moonstar-95515 · 2024-06-30T02:23:37Z

Thank you. @eVoloshchak
I'm not able to run android emulator on VMware.

jayeshmangwani · 2024-07-02T06:37:15Z

But I think I tried every solution but I was not able to run emulator.

@moonstar-95515 What error did you encounter?

mallenexpensify · 2024-07-03T00:23:58Z

@moonstar-95515 per CONTRIBUTING.md, it's a requirement to be able to test on Android

Make sure you can test on all platforms
Expensify requires that you can test the app on iOS, MacOS, Android, Web, and mWeb.

This PR has been open for over a month, I'd love to get it wrapped ASAP

moonstar-95515 · 2024-07-05T05:53:19Z

Thank you everybody.
Finally, I was able to build android on my new Mac.

@eVoloshchak
I've uploaded all videos. Please check.

eVoloshchak · 2024-07-07T22:14:58Z

@moonstar-95515, looks like this is already resolved on staging, could you double-check please?

Screen.Recording.2024-07-08.at.00.06.04.mov

mallenexpensify · 2024-07-08T22:48:51Z

Also confirming I am unable to reproduce on staging.new.expensify.com . Anyone know what PR might have fixed this?

jayeshmangwani · 2024-07-11T10:24:21Z

@eVoloshchak @mallenexpensify I tested this today, and the issue is happening again, even though it was working fine two days ago. It is working fine in production, but the issue is happening on staging

mallenexpensify · 2024-07-12T00:33:48Z

I'm experiencing a different bug on staging (and not production), when I paste text with a blank line underneath, it shows being greyed out and I'm not able to type 'return' to send the message

2024-07-11_17-28-34.mp4

kpadmanabhan · 2024-07-13T22:31:39Z

I'm experiencing a different bug on staging (and not production), when I paste text with a blank line underneath, it shows being greyed out and I'm not able to type 'return' to send the message

I tested on main HEAD without applying any local fixes. the core problem is fixed.

@jayeshmangwani, the approach taken here is to trim off the trailing new-line. This was my first proposal, which is on hold for almost 2 months now. But these changes from someone else has reached production bypassing all regressions. Referring to this.

@eVoloshchak : code in main HEAD trims off newlines from inside code block irrespective of how much ever you copy. refererring to this.

@mallenexpensify : due to extra long waiting times with issues on HOLD, I see that others are fixing code in the same area and the same is merged to main with known regressions (due to which my PR was put on hold). Kin dly suggest how to proceed here.

mallenexpensify · 2024-07-15T21:36:48Z

@kpadmanabhan commented on the issue you're assigned, can you follow up there plz? thx
#39971 (comment)

eVoloshchak · 2024-09-15T08:27:02Z

@moonstar-95515, since the issue was already resolved, could you close this PR please?

fix issue

04db0ff

moonstar-95515 requested a review from a team as a code owner May 31, 2024 03:15

melvin-bot bot removed the request for review from a team May 31, 2024 03:15

melvin-bot bot requested a review from eVoloshchak May 31, 2024 03:15

eVoloshchak suggested changes Jun 3, 2024

View reviewed changes

add comment

23bddd1

eVoloshchak reviewed Jun 4, 2024

View reviewed changes

update comment

59770b5

Merge branch 'main' into issue-42216

e311a24

Christinadobrzyn mentioned this pull request Jun 5, 2024

[Pay meow][$250] Web - Composer - Cursor positioned at the start if pasting text which contains a new line #42216

Closed

6 tasks

jayeshmangwani mentioned this pull request Jun 6, 2024

[HOLD for PR #42892] trim leading and trailing CR LF during paste into composer #41265

Closed

63 tasks

kpadmanabhan mentioned this pull request Jun 6, 2024

[HOLD PR #42892][$250] Text pasted from Slack won't send or disappears in NewDot. #39971

Closed

6 tasks

jayeshmangwani mentioned this pull request Jun 10, 2024

[$250] Markdown - Space in the Code Block is removed after Manually copying and pasting #43331

Closed

6 tasks

Merge branch 'main' into issue-42216

470f24e

Merge branch 'main' into issue-42216

9c70fad

	const offset = (text.length && text[text.length - 1] === '\n' ? node.length - 1 : node.length);
	const offset = text?.endsWith('\n') ? node.length - 1 : node.length;

	// we have to set offset as `node.length - 1` when text has empty line at the end
	// Correctly calculate text length since newline character does not create a new line in HTML
	// https://github.com/Expensify/App/issues/42216

Web - Composer - Cursor positioned at the start if pasting text which contains a new line #42892

Are you sure you want to change the base?

Web - Composer - Cursor positioned at the start if pasting text which contains a new line #42892

Conversation

moonstar-95515 commented May 31, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented May 31, 2024

github-actions bot commented May 31, 2024 • edited Loading

moonstar-95515 commented May 31, 2024

eVoloshchak Jun 3, 2024

Choose a reason for hiding this comment

eVoloshchak Jun 3, 2024

Choose a reason for hiding this comment

moonstar-95515 commented Jun 3, 2024

eVoloshchak Jun 4, 2024

Choose a reason for hiding this comment

eVoloshchak commented Jun 4, 2024

moonstar-95515 commented Jun 4, 2024

eVoloshchak commented Jun 4, 2024

moonstar-95515 commented Jun 10, 2024

eVoloshchak commented Jun 10, 2024

moonstar-95515 commented Jun 10, 2024

eVoloshchak commented Jun 11, 2024

moonstar-95515 commented Jun 11, 2024

eVoloshchak commented Jun 11, 2024 • edited Loading

moonstar-95515 commented Jun 12, 2024

eVoloshchak commented Jun 13, 2024

moonstar-95515 commented Jun 13, 2024

eVoloshchak commented Jun 13, 2024

kpadmanabhan commented Jun 14, 2024

moonstar-95515 commented Jun 17, 2024

kpadmanabhan commented Jun 17, 2024

eVoloshchak commented Jun 24, 2024

moonstar-95515 commented Jun 25, 2024

eVoloshchak commented Jun 27, 2024

moonstar-95515 commented Jun 30, 2024

jayeshmangwani commented Jul 2, 2024

mallenexpensify commented Jul 3, 2024

moonstar-95515 commented Jul 5, 2024

eVoloshchak commented Jul 7, 2024

mallenexpensify commented Jul 8, 2024

jayeshmangwani commented Jul 11, 2024

mallenexpensify commented Jul 12, 2024

kpadmanabhan commented Jul 13, 2024

mallenexpensify commented Jul 15, 2024

eVoloshchak commented Sep 15, 2024

moonstar-95515 commented May 31, 2024 •

edited

Loading

github-actions bot commented May 31, 2024 •

edited

Loading

eVoloshchak commented Jun 11, 2024 •

edited

Loading