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: Caret moves to a wrong position when uploading an image via toolbar #15684

Conversation

AndrewPrigorshnev
Copy link
Contributor

@AndrewPrigorshnev AndrewPrigorshnev commented Jan 23, 2022

When uploading an image, we change the uploading placeholder several times. Every time, we correct the position of the cursor after replacing. But we schedule repositioning of cursor to the afterRender queue in Ember Run Loop. As a result, sometimes we replace the placeholder several times but correct the cursor position only once at the end.

It just cannot work correctly with scheduling, we'll always be dealing with cumulative error. Removing scheduling fixes the problem.

Sadly, I cannot make the test work, I skipped it for now, going to give it another try later.

@AndrewPrigorshnev AndrewPrigorshnev force-pushed the fix/caret-moves-to-a-wrong-position-when-uploading-an-image-via-toolbar branch 2 times, most recently from 6554cf1 to 51a6e42 Compare February 3, 2022 18:09
@AndrewPrigorshnev AndrewPrigorshnev force-pushed the fix/caret-moves-to-a-wrong-position-when-uploading-an-image-via-toolbar branch from 51a6e42 to 498071e Compare February 3, 2022 20:15
@AndrewPrigorshnev AndrewPrigorshnev marked this pull request as ready for review February 3, 2022 22:00
@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/caret-moves-to-end-of-line-when-uploading-an-image-via-toolbar/214822/8

@@ -477,7 +477,7 @@ export default Component.extend(TextareaTextManipulation, {
key: "#",
afterComplete: (value) => {
this.set("value", value);
return this._focusTextArea();
return schedule("afterRender", this, this._focusTextArea);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need the return here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need it, you're right.

@ZogStriP
Copy link
Member

ZogStriP commented Feb 4, 2022

Sadly, I cannot make the test work, I skipped it for now, going to give it another try later.

Maybe @CvX or @jjaffeux can help you with that?

@AndrewPrigorshnev
Copy link
Contributor Author

AndrewPrigorshnev commented Feb 4, 2022

Maybe @CvX or @jjaffeux can help you with that?

It's not like there is a roadblock that is too hard to deal with. It just needs a bit more time to play with, when I'm testing it I see that the bug is clearly solved but something unusual is happening in the test environment.

I wanted to merge it to dogfood it a bit and then backport to the release. What if I deal with the test early next week, and ping @CvX or @jjaffeux in case it takes too much time?

@AndrewPrigorshnev AndrewPrigorshnev merged commit 778abb0 into main Feb 4, 2022
@AndrewPrigorshnev AndrewPrigorshnev deleted the fix/caret-moves-to-a-wrong-position-when-uploading-an-image-via-toolbar branch February 4, 2022 14:26
@AndrewPrigorshnev
Copy link
Contributor Author

Turned out that this fixes the bug only when running under Ember CLI. The PR for the legacy environment is upcoming.

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