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

Fix: Focus at the end of the pasted contents instead of the beginning #6427

Closed
wants to merge 1 commit into from

Conversation

hackerbirds
Copy link
Contributor

@hackerbirds hackerbirds commented May 21, 2023

Contributor checklist:

  • My contribution is not related to translations.
  • My commits are in nice logical chunks with good commit messages
  • My changes are rebased on the latest main branch
  • A yarn ready run passes successfully (more about tests here)
  • My changes are ready to be shipped to users

Description

When pasting a long text into the composition input, it does not scroll down to the end of the pasted text--it stays at the beginning instead. This PR changes this behaviour so that the input quill will automatically focus at the end of the text instead (i.e. scroll to the bottom of the pasted text)

I found that the problem emerged because the code calls this.quill.focus() before updating the contents of the quill, not after, so it would scroll to where the cursor was before the pasted text was being added, and not where it is after pasting the text. Simply calling this.quill.focus() after updating the contents seems to solve the problem.

This fixes https://community.signalusers.org/t/when-pasting-mutliple-lines-jump-scroll-input-field-to-last-pasted-line/51677

Edit:

Here is a visual explanation of the change:
Before pasting (notice that the cursor is between two words):
beforepasting

After pasting, before the fix:
afterpastingnofix

After pasting with the fix:
afterpastingfix

@hackerbirds
Copy link
Contributor Author

By the way, I found no side effects to having it focus() after the if statement that potentially returns the function early. I tried edge cases like having no text at all or copying images and found that everything was working as intended (and still focusing).

However if you think this could be a problem, we can perhaps just call focus() twice, once in the beginning and again at the end. Not the prettiest solution but it would work. Let me know!

@josh-signal josh-signal self-requested a review May 22, 2023 17:48
@josh-signal josh-signal self-assigned this May 22, 2023
@scottnonnenberg-signal
Copy link
Contributor

Thanks for looking into this! I'm going to close this, though, because we have a different solution in the setSelection() call further down in that handler.

@scottnonnenberg-signal
Copy link
Contributor

Nevermind, that was a different issue. Reopening. :0)

@hackerbirds
Copy link
Contributor Author

Thanks for looking into this! I'm going to close this, though, because we have a different solution in the setSelection() call further down in that handler.

Yeah, looks like that change broke the fix I proposed for some reason. I'll try to look for a newer solution that works and rebase it to latest if I do.

@hackerbirds
Copy link
Contributor Author

hackerbirds commented May 23, 2023

I looked at the generated quill.js (node-modules/quill/dist/quill.js) and the onPaste implementation in it focuses twice: once in the beginning like it already was, and a second time inside the setTimeout(){}, so I decided to do that since the rest of the code pretty much matched what quill.js was doing. The changes/rebase I just pushed should work now.

@hackerbirds
Copy link
Contributor Author

Oh, looks like it was added, no wonder the rebase didn't do anything 😅 Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants