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

Editor has a reverse typing effect on slower machines #14569

Closed
Witoso opened this issue Jul 12, 2023 · 19 comments · Fixed by #14648
Closed

Editor has a reverse typing effect on slower machines #14569

Witoso opened this issue Jul 12, 2023 · 19 comments · Fixed by #14648
Assignees
Labels
domain:performance This issue reports a problem or potential improvement regarding editor performance. domain:typing/ime This issue reports a problem with standard typing & IME (typing method for CJK languages). package:typing squad:core Issue to be handled by the Core team. support:1 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@Witoso
Copy link
Member

Witoso commented Jul 12, 2023

📝 Provide detailed reproduction steps (if any)

First reported in the #13926.

It's not that the typing is "reversed", but more like the cursor is "stuck" and isn't updating -- so text just keeps getting added behind the cursor. Seems to be something with setting the DOM selection. The behavior seems to be more obvious when the JS thread is very busy and the user is typing during this time. Funny enough, when things settle slightly, the cursor is able to update again, and the user doesn't really need to do anything.

I was able to actually repro it a couple times in the inline editor demo here: https://ckeditor.com/docs/ckeditor5/latest/examples/builds/inline-editor.html

Simulating throttling of CPU by 4x/6x on DevTools, and loading the page before the content has settled allowed me to briefly reproduce the issue at some point.

✔️ Expected result

There's no reverse typing effect.

❌ Actual result

On slower machines, the text keeps getting added behind the cursor.

📃 Other details

  • Browser: …
  • OS: …
  • First affected CKEditor version: …
  • Installed CKEditor plugins: …

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@Witoso Witoso added type:bug This issue reports a buggy (incorrect) behavior. package:typing squad:core Issue to be handled by the Core team. domain:performance This issue reports a problem or potential improvement regarding editor performance. domain:typing/ime This issue reports a problem with standard typing & IME (typing method for CJK languages). labels Jul 12, 2023
@urbanspr1nter
Copy link
Contributor

@Witoso - Other than being used as a document editor, our scenario actually uses CKE5 as an effective editing engine for messaging purposes. Therefore the volume of documents being generated by CKE5 is very, very high. 😊 It's quite a good confirmation on the resilience of the editor overall. Great job!

There are a few problems which we have been able to workaround, however the one that that seems very fundamental to which I typically cannot patch over/write code to workaround is this reverse typing issue. it happens quite a bit for our specific application using CKE5.

The symptoms are quite consistent when it does happen:

  • The JS rendering thread is very busy when the user begins typing
  • As soon as typing and render of the character happens, the selection does not update, and causes the cursor to "stick". What the user sees is "reverse typing"

When we started seeing this occur more often is around late Feb 2023 -- probably shortly after the beforeinput migration. I know that happened around January.

I see that DOM selection is updated on render in renderer.ts. I've many times tweaked the code to try to nail down an easy to reproduce scenario without having to rely on timing tricks and system throttling.

Is the process in updating the DOM selection different from when characters get rendered into the contenteditable?

@urbanspr1nter
Copy link
Contributor

As far as what constitutes a "slower machine" - We've been able to indicate occurrences of this happening on Core i5 8000U series chips. So these are not exactly slow, and not totally old either... but could be slow enough under certain power scenarios such as being on battery and "silent mode".

For automated tests, we space out our keystrokes to get around this. Otherwise, text will be completely "backwards" if just left for the browser test runner to input keys into the editable.

@Reinmar
Copy link
Member

Reinmar commented Jul 13, 2023

I was trying to reproduce this. With CPU slowed down 6x, recording the timeline (which adds its own overhead), and in the mentioned demo (https://ckeditor.com/docs/ckeditor5/latest/examples/builds/inline-editor.html). To no avail. 

I was generating keystrokes by moving my fingers over the keyboard (like: "qwertyuiopasdfghjklzxcvbnm"). 

Even though the handlers took ~70ms each and the text appeared with sever delay, it's not reversed in any way.

I still see the typing performance as an issue. But I wonder if there's anything there that triggers the "reversing" (which increases the severity of the issue for sure.

@urbanspr1nter
Copy link
Contributor

@Reinmar - that's about what we observe, however the issue seems to be appearing whenever the editor appears to be initializing.

@urbanspr1nter
Copy link
Contributor

Also one more thing, the issue seems to occur whenever the editor starts out empty.

@lslowikowska lslowikowska added the support:1 An issue reported by a commercially licensed client. label Jul 18, 2023
@urbanspr1nter
Copy link
Contributor

urbanspr1nter commented Jul 19, 2023

@Witoso , @Reinmar - For documentation in the future, I'll put some details here to share with the community...

This one is hard to reproduce, but I have been able to take some performance profiles in the behavior which occurs. Almost always that the problem comes up whenever the editable becomes focused, and even more when the editor starts out as empty.

An example scenario where I intend to type testtest, it actually becomes like testtset, where the selection [] moves like this in time:

[]t
[]et
[]set
[]tset
t[]tset
tes[]tset
test[]tset

The keystrokes in which the cursor does not move has a call stack that looks like this:
image

The keystrokes in which the cursor does move, has a call stack that looks like this:
image

Notice the pattern here: _updateSelection->_updateDomSelection->_domSelectionNeedsUpdate->domSelectionToView->Browser Layout. This is what allows the selection to update. However the first few keystrokes do not have this pattern. What happened to this final call?

I have been able to reproduce this at will by modifying focusobserver.ts here: https://github.com/ckeditor/ckeditor5/blob/755159dbc1bfd9853ed912268fa2aed2cd9a0714/packages/ckeditor5-engine/src/view/observer/focusobserver.ts#L62C1-L65C12

It seems like for a moment (very brief), the rendering is locked up or at least DOM selection update until the callback for the setTimeout finally happens. I believe, possibly that under high load in the JS thread, where the event queue is slammed for moments (like in our scenario), the event queue can run deep, and the setTimeout may not fire at the specified duration.

At some point, then the reverse typing occurs until at some point where it magically corrects itself.

In this case, for us, something like 50ms may not be happening. To test this, I cranked it up to 10,000ms and was able to repro everything at will until the 10s expiration.

I was able to modify the code to use requestAnimationFrame instead, and the problem totally went away!

this._renderTimeoutId = requestAnimationFrame( () => {
  this.flush();
  view.change( () => {} );
});

Question now is whether this will produce unintended side effects, or break what this was trying to fix to begin with, which seemed to be scenarios involving Image captions and Safari...

@urbanspr1nter
Copy link
Contributor

urbanspr1nter commented Jul 21, 2023

@Witoso , @Reinmar - scratch that -- it didn't totally fix it, and just lessened the occurrence significantly. I assume now that we really do need to wait for the selection event to happen first too... :-\

However, I am fairly certain it is that piece of focus code that is causing a lot of the trouble.

@lslowikowska lslowikowska added support:3 An issue reported by a commercially licensed client. support:1 An issue reported by a commercially licensed client. and removed support:1 An issue reported by a commercially licensed client. support:3 An issue reported by a commercially licensed client. labels Jul 24, 2023
@CKEditorBot CKEditorBot added the status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. label Jul 24, 2023
@niegowski niegowski self-assigned this Jul 24, 2023
@CKEditorBot CKEditorBot added status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Jul 24, 2023
@niegowski
Copy link
Contributor

I was able to reproduce this issue by making the CPU busy with a loop like this:

function tick() {
	const start = performance.now();

	while ( performance.now() - start < 1000 ) {
		// blocking CPU
	}

	setTimeout( tick, 0 );
}

Looks like the document.isFocused is not updated before the text is inserted in the editable. I'm looking for a solution.

@niegowski
Copy link
Contributor

So the issue was happening because the FocusObserver was not flushed in cases when the DOM selection was matching the view document selection. This caused the isFocused flag to stay false until the timeout was reached. Could you please confirm that changes from #14648 fixed this issue?

@urbanspr1nter
Copy link
Contributor

Hey @niegowski , good news! Let me try this right now, and will follow up.

@urbanspr1nter
Copy link
Contributor

@niegowski - Tried out the changes, this looks like it works from my repro case. :)

Thank you so much for being so quick to this.. 🙇‍♂️

@urbanspr1nter
Copy link
Contributor

urbanspr1nter commented Jul 24, 2023

@niegowski ...... Never mind. This one still comes up... I just didn't throttle hard enough. 🙁.. However the behavior is a bit different... seems like it does get out of the reverse typing effect faster.

@urbanspr1nter
Copy link
Contributor

For sure it does feel better, though. At some point I was able to type a longer string in a sticky cursor state, now it seems to be behaving better after the change. There is some reverse typing but it does correct itself. However, I am not sure if it is just me telling myself this... From what I'm understanding, based on the PR, the focus observer was in a "locking state" whenever selection didn't change in the case of something like an ALT+TAB switch from the browser to some app and back. (This is a case where I found the issue to reproduce quite easily on a "throttled" machine) --- The change seems like it is fixing something consistent with what I've seen.

However, I can't just seem to shake off the fact that I still get a little bit of reverse typing and I'm not sure for some, how tolerable that would be. (I'm just a little scarred from experiencing this type of nightmare for months now 🤣) - IMO, the expected experience from such a scenario should be that the characters are blocked from rendering if the UI thread is super busy... But that was my expectation.

I do think this fix is needed with the way it is right now. It does seem to provide some relief, but I'm wondering if there's more that can be done?

@niegowski
Copy link
Contributor

the focus observer was in a "locking state" whenever selection didn't change in the case of something like an ALT+TAB switch from the browser to some app and back.

This scenario helped me a lot to discover the solution. Now I updated the above PR to flush the FocusObserver if there is some typing in the editable. @urbanspr1nter could you please retest this PR?

@urbanspr1nter
Copy link
Contributor

@niegowski , Yes! Flushing the FocusObserver in InsertTextObserver seems a lot better! :)

I will continue to test more, but this is already significantly better.

@urbanspr1nter
Copy link
Contributor

Hey @niegowski - I did some more testing, I believe your latest iteration is the solution. 👍

For our specific integration, we have a different behavior for Mac and Windows. The issue is easily reproducible from Windows side once a pattern is found, but for Mac, it seems to come up easier (esp with the ALT(CMD)+TAB scenario).

Based on my testing and just overall review of the code path related to my scenario testing, with the latest commit in your PR, everything seems happy and behaving. :)

I wish I could approve the PR, even if it is just for morale. 🎉

I understand that this issue was really hard to reproduce in the first place, so to say it is completely solved needs some really good confidence 😂. Let's assume this is solved for now, and if not, it may just be a case-by-case basis going forward as we discover other interesting scenarios. Otherwise, this is an incredible improvement.

@Witoso
Copy link
Member Author

Witoso commented Jul 26, 2023

Great to hear that @urbanspr1nter 😁 Kudos @niegowski!

@PRYNSHU
Copy link

PRYNSHU commented Apr 9, 2024

Is the Reverse typing effect issue resolved?

@urbanspr1nter
Copy link
Contributor

This specific one is, but there are other cases to consider, and this PR considers it: #16066

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:performance This issue reports a problem or potential improvement regarding editor performance. domain:typing/ime This issue reports a problem with standard typing & IME (typing method for CJK languages). package:typing squad:core Issue to be handled by the Core team. support:1 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
7 participants