Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
1 of 6 tasks
kbecciv opened this issue May 15, 2024 · 79 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented May 15, 2024

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


Version Number: 1.4.74-0
Reproducible in staging?: y
Reproducible in production?: y
Issue reported by: Applause - Internal Team

Action Performed:

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

Expected Result:

The cursor gets positioned at the end of the pasted text, no console error

Actual Result:

The cursor gets positioned at the start of the pasted text if the pasted text contains a new empty line, console error is showing

Workaround:

n/a

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6481716_1715782332852.bandicam_2024-05-15_17-07-29-894.mp4
image

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01cc45b59ee9c4ae02
  • Upwork Job ID: 1792451156946927616
  • Last Price Increase: 2024-08-08
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels May 15, 2024
Copy link

melvin-bot bot commented May 15, 2024

Triggered auto assignment to @Christinadobrzyn (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@kbecciv
Copy link
Author

kbecciv commented May 15, 2024

We think that this bug might be related to #wave-collect - Release 1

@kbecciv
Copy link
Author

kbecciv commented May 15, 2024

@Christinadobrzyn FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

@Christinadobrzyn
Copy link
Contributor

Very similar to #42112 - asking if the fix will be the same

@MOONSTAR-TOP-DEV
Copy link
Contributor

Proposal

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

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

What is the root cause of that problem?

We are setting cursor position in below part when pasting.

range.setStart(node, node.length);
range.setEnd(node, node.length);

When pasted text contains empty line, the value of node.length is greater than it's actually length because the length of line break at the end(\n\n) will be caculated as 2.
So console error occurs and cursor is not set because we are trying to set cursor to non-exist position.

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

We have to set range with node.length - 1 when it contains empty line at the end.

What alternative solutions did you explore? (Optional)

Copy link

melvin-bot bot commented May 16, 2024

📣 @moonstar-95515! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@Christinadobrzyn
Copy link
Contributor

Doesn't seem to be related to #42112 as that is resolved. I can still reproduce this - adding external

@Christinadobrzyn Christinadobrzyn added the External Added to denote the issue can be worked on by a contributor label May 20, 2024
@melvin-bot melvin-bot bot changed the title Web - Composer - Cursor positioned at the start if pasting text which contains a new line [$250] Web - Composer - Cursor positioned at the start if pasting text which contains a new line May 20, 2024
Copy link

melvin-bot bot commented May 20, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01cc45b59ee9c4ae02

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label May 20, 2024
Copy link

melvin-bot bot commented May 20, 2024

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

@rakhaxor
Copy link

rakhaxor commented May 20, 2024

Proposal

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

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

What is the root cause of that problem?

In the text param, newline character(s) at the end (if present) are wrongly interpreted by the cursorUtils library because node.length is calculated including the \n but \n is not inserted in the document node which makes the range go out of bounds.

const insertAtCaret = (target: HTMLElement, text: string) => {

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

Creating a new text variable with stripped newline characters at the end or modifying reusing the same text would work fine.

const plainText = text.replace(/\n+$/, '');
And use this in all the places we are using text:

const node = document.createTextNode(text);
insertByCommand(text);

What alternative solutions did you explore? (Optional)

None

@RickDeb2004
Copy link

RickDeb2004 commented May 20, 2024

Proposal

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

when pasting text containing a new empty line into the chat compose box, the cursor incorrectly positions itself at the start of the text instead of the end and a console error occurs.

What is the root cause of that problem?

The root cause of the problem is that the cursor positioning logic in the insertAtCaret function incorrectly handles the insertion of text containing new lines. Specifically, the cursor is not being moved to the correct position (the end of the newly inserted text) when the text includes empty new lines, which causes the cursor to appear at the start of the text. Additionally, there is no proper error handling to catch and log any potential issues during the paste operation, leading to console errors.

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

a)
original code :

range.setStart(node, node.length);
       range.setEnd(node, node.length);
       selection.removeAllRanges();
       selection.addRange(range);

modified code:

 range.setStartAfter(node);
        range.setEndAfter(node);
        selection.removeAllRanges();
        selection.addRange(range);
 

b)
original code :

target.dispatchEvent(new Event('paste', {bubbles: true}));
       // Dispatch input event to trigger Markdown Input to parse the new text
       target.dispatchEvent(new Event('input', {bubbles: true}));
   } else {
       insertByCommand(text);
   };

modified code:

 target.dispatchEvent(new Event('input', {bubbles: true}));
    } else {
        insertByCommand(text);
    }
 

c)
original code :

const paste = useCallback((text: string) => {
       try {
           const textInputHTMLElement = textInputRef.current as HTMLElement;
           if (textInputHTMLElement?.hasAttribute('contenteditable')) {
               insertAtCaret(textInputHTMLElement, text);
           } else {
               insertByCommand(text);
           }

           // Pointer will go out of sight when a large paragraph is pasted on the web. Refocusing the input keeps the cursor in view.
           textInputRef.current?.blur();
           textInputRef.current?.focus();
           // eslint-disable-next-line no-empty
       } catch (e) {}
       // We only need to set the callback once.
       // eslint-disable-next-line react-hooks/exhaustive-deps
   }, []);

modified code:

 const paste = useCallback((text: string) => {
        try {
            const textInputHTMLElement = textInputRef.current as HTMLElement;
            if (textInputHTMLElement?.hasAttribute('contenteditable')) {
                insertAtCaret(textInputHTMLElement, text);
            } else {
                insertByCommand(text);
            }

            // Pointer will go out of sight when a large paragraph is pasted on the web. Refocusing the input keeps the cursor in view.
            textInputRef.current?.blur();
            textInputRef.current?.focus();
        } catch (e) {
            // ** Added console error log here **
            console.error('Error during paste handling:', e);
        }
    }, []);

 

Summary of Changes

1.Cursor Positioning Fix:

Updated the insertAtCaret function to use range.setStartAfter(node) and range.setEndAfter(node) to correctly position the cursor at the end of the newly inserted text, especially when new lines are involved.

2.Error Handling:

Added console.error logging within the paste function to handle and log any exceptions that occur during the paste operation, making it easier to debug issues.
These changes ensure that the cursor is positioned correctly after pasting text with new lines and improve error visibility during paste operations.

What alternative solutions did you explore? (Optional)

None

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/~017738fb94de003574

Copy link

melvin-bot bot commented May 20, 2024

📣 @RickDeb2004! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@Christinadobrzyn
Copy link
Contributor

hi @eVoloshchak can you take a peek at these proposals when you have a moment? TY!

@eVoloshchak
Copy link
Contributor

We have to set range with node.length - 1 when it contains empty line at the end.
@moonstar-95515, that works for this specific case of testing steps, but wouldn't work if pasted text end with 3 (or more) new lines

I think we should proceed with @rakhaxor's proposal, it's clean and straightforward. It does cut off all of the new lines at the end of the pasted text, but that's what the backend does when you send the message, so this can be considered a small improvement

🎀👀🎀 C+ reviewed!

Copy link

melvin-bot bot commented May 21, 2024

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

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label May 21, 2024
Copy link

melvin-bot bot commented May 21, 2024

📣 @rakhaxor You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

@MOONSTAR-TOP-DEV
Copy link
Contributor

MOONSTAR-TOP-DEV commented May 21, 2024

@eVoloshchak

@moonstar-95515, that works for this specific case of testing steps, but wouldn't work if pasted text end with 3 (or more) new lines

My proposal works when it has 3 or more new lines at the end, as the text will be hello 111\n\n\n\n when it has 3 new lines.
Only last new line is presented with two \n.

@MOONSTAR-TOP-DEV
Copy link
Contributor

MOONSTAR-TOP-DEV commented May 21, 2024

@eVoloshchak cc: @Christinadobrzyn @AndrewGable

It does cut off all of the new lines at the end of the pasted text, but that's what the backend does when you send the message, so this can be considered a small improvement

I can't sure if I understood correctly, but I think only new lines at the end of the text will be cut on backend side.
If we copy text with new lines in the middle of the compose box and paste it again, new lines will be deleted. I think this is not what we want.

test.mp4

@melvin-bot melvin-bot bot added the Overdue label Aug 12, 2024
Copy link

melvin-bot bot commented Aug 12, 2024

@AndrewGable, @eVoloshchak, @rakhaxor, @Christinadobrzyn, @moonstar-95515 Whoops! This issue is 2 days overdue. Let's get this updated quick!

@Christinadobrzyn
Copy link
Contributor

Hi @moonstar-95515 sorry for the delay here - I just created a new offer in this Upwork job post - https://www.upwork.com/nx/wm/offer/103512886

Can you see if you can accept that for payment? Thank you!

@melvin-bot melvin-bot bot removed the Overdue label Aug 13, 2024
@MOONSTAR-TOP-DEV
Copy link
Contributor

@Christinadobrzyn
Thank you.
I've accepted offer.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Daily KSv2 labels Aug 14, 2024
@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Aug 15, 2024

Thanks @moonstar-95515!

I'm a little lost on the payments due for this. I think this is the summary, can we get a review.

Payouts due:

@MOONSTAR-TOP-DEV
Copy link
Contributor

@Christinadobrzyn
Thank you for your summary.
@ rakhaxor was mis-assigned at the first time and not unassigned yet.

@mallenexpensify
Copy link
Contributor

@rakhaxor had an accepted proposal and was initially assigned/hired so $125 seems fair, even with @moonstar-95515 working on the PR/issue later.

@MOONSTAR-TOP-DEV
Copy link
Contributor

@mallenexpensify
Thank you for your explanation.

Of course, I know that I have no face to say anything because my solution has not been reflected in production.
But I still have some unclear points.
Can contributors be compensated even if it is due to a misjudgement of C+ as long as they are assigned to an issue?

@rakhaxor
Copy link

Hello @Christinadobrzyn, Thank you for the offer. Here’s my upwork: https://www.upwork.com/freelancers/~01f94219e6101472c1

@MOONSTAR-TOP-DEV
Copy link
Contributor

@mallenexpensify , @Christinadobrzyn
Sorry for bothering again.
Of course, I've already agreed 50% of compensation here.
But I think it's not fair considering my work and @rakhaxor's work in this issue.

Here's summary:

  • My proposal was the first one and perfect.
  • But then @rakhaxor 's proposal was selected due to misjudgment. He worked on this issue until it was assigned to me.
  • Anyway, my proposal was selected here.
  • Then, I've created PR and I worked on the PR for over a month.

I think I put much more effort in this issue and the same compensation is not fair.
Could you please reconsider about compensation?

@rakhaxor
Copy link

@moonstar-95515 What’s your problem with me man? I have done all the tests and made screen recordings. You were assigned later on, I did all the work there and was asking to submit PR. I didn’t even comment even after doing all the work and not being able to submit it until now when I was asked to submit my profile link. Anyways, I just need you to not talk about me anymore. Thank you

@MOONSTAR-TOP-DEV
Copy link
Contributor

@rakhaxor
I don't have any problem with you. I'm just talking about my rights.
Do you really think you have right to be compensated?
I've commented just after you were assigned and you may know that your proposal was wrong. I can't understand why you kept working.
But I'm not saying you shouldn't be compensated, I'm just saying that I put more effort and there must be difference in compensation.
That's all. Thank you.

@melvin-bot melvin-bot bot added the Overdue label Aug 19, 2024
@mallenexpensify
Copy link
Contributor

The decision to compensate @rakhaxor is an edge case, based on the fact they were assigned to the issue and hired. It was the responsibility of Expensify to hire/assign. Since this action is considered a source-of-truth and gives the green light for the contributor to invest more time into the issue (assuming compensation), we feel it's fair to compensate. Since their proposal was initially accepted, we decided on 50%.

@moonstar-95515 , your compensation on this issue, and the value associated with it, is completely unrelated to the compensation for @rakhaxor. Please keep in mind our documented code of conduct when communicating with others, both internal and with fellow contributors.

We pride ourselves as a company and as individual employees on our core qualities: Talent, Ambition, & Humility. The environment we have developed uses these qualities to adhere to our two golden rules of Expensify:

  1. Get Shit Done
  2. Don’t Ruin it for Everyone Else

Thanks

Copy link

melvin-bot bot commented Aug 19, 2024

@AndrewGable, @eVoloshchak, @rakhaxor, @Christinadobrzyn, @moonstar-95515 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@Christinadobrzyn
Copy link
Contributor

Quick follow up - based on a review (as Matt outlined) we are going to continue with this payment summary.

@eVoloshchak can you accept this Upwork offer - https://www.upwork.com/nx/wm/offer/103544327
@rakhaxor can you accept this Upwork offer - https://www.upwork.com/nx/wm/offer/103593092

TY!

@melvin-bot melvin-bot bot removed the Overdue label Aug 19, 2024
@rakhaxor
Copy link

Hello @Christinadobrzyn! I have accepted the offer on Upwork. Thank you

@eVoloshchak
Copy link
Contributor

@Christinadobrzyn, declined the Upwork offer, requested the payment on NewDot
Thanks!

@Christinadobrzyn
Copy link
Contributor

Awesome! Thanks! The payment summary is here.

Feel free to submit your NewDot payment @eVoloshchak. Also, I don't see that it was asked but do we need a regression test for this?

@JmillsExpensify
Copy link

$250 approved for @eVoloshchak

@Christinadobrzyn
Copy link
Contributor

@eVoloshchak can you let us know about a regression test? Then I think this is good to close!

@eVoloshchak
Copy link
Contributor

Yes, a regression test would be useful and easy to perform

Regression Test Proposal

  1. Navigate to staging.new.expensify.com
  2. Open a chat and type a one-line text and create a few empty lines
test



  1. Select all using keyboard ctrl + A and cut the text
  2. Paste it inside the composer
  3. Verify the cursor is positioned at the end of the pasted text

Do we agree 👍 or 👎

@Christinadobrzyn
Copy link
Contributor

Regression test is created - https://github.com/Expensify/Expensify/issues/422290

Closing!

Copy link

melvin-bot bot commented Aug 21, 2024

@AndrewGable @Christinadobrzyn Be sure to fill out the Contact List!

@github-project-automation github-project-automation bot moved this from Polish to Done in [#whatsnext] #wave-collect Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
No open projects
Status: Done
Development

No branches or pull requests

10 participants