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

The first character of a word is duplicated to the right of the caret using Samsung keyboard using Firefox Mobile on Android #14567

Closed
dtdesign opened this issue Jul 11, 2023 · 21 comments · Fixed by #16289
Labels
browser:firefox 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. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@dtdesign
Copy link

dtdesign commented Jul 11, 2023

📝 Provide detailed reproduction steps (if any)

A user has reported that sometimes the first character of a word is duplicated and appears to the right of the caret. Any keypress is inserted to the left of the caret as expected.

The user has provided a screen recording of the issue and has explicitly confirmed that this is reproducible in the online demo of CKEditor. It was confirmed that the issue does not occur with Gboard or SwiftKey.

Screen_Recording_20230703_075830_Firefox.mp4

✔️ Expected result

Typing just works as expected.

❌ Actual result

The first character of a new word is duplicated and inserted to the right of the caret.

📃 Other details

  • Browser: Firefox Mobile 114
  • OS: Android 13
  • First affected CKEditor version: Unknown
  • Installed CKEditor plugins: Not relevant

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

@dtdesign dtdesign added the type:bug This issue reports a buggy (incorrect) behavior. label Jul 11, 2023
@Witoso Witoso added package:typing browser:firefox domain:typing/ime This issue reports a problem with standard typing & IME (typing method for CJK languages). squad:core Issue to be handled by the Core team. labels Jul 12, 2023
@mabryl
Copy link
Contributor

mabryl commented Aug 24, 2023

The issue is still reproducible, however it goes away when the predictive text option in the keyboard's settings is turned off.

It's the same case as the one mentioned here: #13247 (comment)

Issue not reproducible while using Gboard or SwiftKey.

@dtdesign
Copy link
Author

@Witoso We have received multiple reports from users that were affected by this issue. We suggested to them to disable the predictive text option and it was uniformly confirmed to resolve the issue for them.

This means that this is less of an issue of Firefox but rather a problem of that specific option of the Samsung keyboard. Samsung owns a significant potion of the smartphone market and this issue appears to be exclusively / primarily an issue with CKEditor.

@Witoso
Copy link
Member

Witoso commented Dec 27, 2023

@dtdesign we will be investigating this on our side in more depth (waiting for a proper hardware).

@ps915
Copy link

ps915 commented Jan 16, 2024

Same issue for my users.

Affected devices:

  • Galaxy S10+ / Firefox 121.1.0 / Android 12 / MS Swift Keyboard
  • Galaxy S10+ / Firefox 121.1.0 / Android 12 / Swype + Dragon Keyboard
  • Galaxy S23 / Firefox 121.1.0 / Android 14 / Samsung Standard Keyboard
  • LG Smartphone / Firefox / LG Standard Keyboard
  • LG Q60 / Firefox 121.1.0 sowie 120.0 / Android 10 / LG Keyboard ("Simple Keyboard" works fine)
Wortverdoppelung.mp4
Screen_Recording_20240116_123854_Firefox.Beta.mp4

@rpi-simonz
Copy link

Those duplicated characters bug happens even on my exotic smartphone OS:

Sony Xperia 10II -- Sailfish OS 4.5 -- Browser Opera in an Android 11 Virtual Machine.

(No Samsung or Samsung Keyboard involved here.)

@aze-dzn
Copy link

aze-dzn commented Jan 31, 2024

@Witoso So, how long until you get the hardware to pinpoint the issue and possibly find a solution? Waiting for a solution for months doesn't seem right, especially given Samsung's market share. And using a different keyboard as a workaround isn't winning users over.

@ps915
Copy link

ps915 commented Jan 31, 2024

Do you have any news about a possible solution? We are also waiting desperately for a bugfix (Woltlab Customer)!

@Witoso
Copy link
Member

Witoso commented Feb 1, 2024

We really appreciate all the comments, but we usually don't share details about the development stage. Typing is one of the hardest topics to debug, and as you see, even when we try to closely follow standards, browsers or keyboards break them. This won't be easy nor fast, also may not be able to provide a fix for all “exotic” or already unsupported OSes.  Thank you for your patience :)

@aze-dzn
Copy link

aze-dzn commented Feb 1, 2024

Understandable, I appreciate the long waiting period 100%. I can also fully understand that an editor doesn't live by typing correctly, without any strange behaviors, alone, and that you'd rather devote yourself to other things. I'll light a candle of hope that we get a fix this year; 6 months ain't long enough.

@ghost
Copy link

ghost commented Feb 2, 2024

@Witoso However, we are talking about the standard Samsung keyboard here. The standard should be the least that is covered.

@dtdesign
Copy link
Author

dtdesign commented Feb 3, 2024

This problem is a major issue and I spend half a day looking into it in order to find a pattern in the behavior. I was able to observe that the browser in some situations issues two beforeinput events for one keystroke. Going further I noticed a pattern in the browser’s behavior.

I have documented by observations before and mostly typed them while trying out different things and sometimes there was a gap of an hour between two paragraphs because chased some wild ideas. Debugging contenteditable certainly never gets boring.

Observed Behavior in the Editor

When I type This, the entire word is underlined including the initial T. At the moment I finish the first word by entering a space, This is no longer underlined. The editor now contains only This and pay attention to the trailing space here, it is important!

Next, I type t into the editor and it then contains This t, but the t is not underlined at all. If I type h now, the editor will contain Test tht and the th is being briefly underlined for a split second. I believe this is due to the selection being updated and the keyboard now notices that there is a character after the caret position and correctly assumes it to be placed inside a word.

The important bit is that I can backspace the second word, leaving only This , and typing again yields the exact same issue.

Comparing the Events Being Dispatched by the Browser

There are essentially four events that can be used to determine what happens in the editor. The most notable is beforeinput which is actively being used by CKEditor to detect the typed text. There are also compositionstart, compositionupdate and compositionend that, while being ignored by CKEditor on Android, provide yet another data point.

Monitoring beforeinput and logging the contents of data reveals this:

T
Th
Thi
This
This <-- The composition ends here
<-- This is a space
t
th
th <-- Composition ends here again? Also, this is triggered by the same keystroke as the invocation above!

This confirms that whatever causes the character to be duplicated is the result from an extra beforeinput event. So technically, this isn’t CKEditor at fault, unless …

Hypothesis: The Space Character Makes the Difference

At this point I had the suspicion that there must be something odd about the space being inserted, because the first character typed immediately after the space is not being underlined. As far as I understand it, the underlining is part of the composition process therefore its absence signals that no composition takes places. This causes an issue because of the assumptions made in input.ts about Android’s composition behavior:

// Typing in English on Android is firing composition events for the whole typed word.
// We need to check the target range text to only apply the difference.
if ( env.isAndroid ) {
const selectedText = Array.from( modelRanges[ 0 ].getItems() ).reduce( ( rangeText, node ) => {
return rangeText + ( node.is( '$textProxy' ) ? node.data : '' );
}, '' );
if ( selectedText ) {
if ( selectedText.length <= insertText.length ) {
if ( insertText.startsWith( selectedText ) ) {
insertText = insertText.substring( selectedText.length );
( modelRanges[ 0 ] as any ).start = modelRanges[ 0 ].start.getShiftedBy( selectedText.length );
}
} else {
if ( selectedText.startsWith( insertText ) ) {
// TODO this should be mapped as delete?
( modelRanges[ 0 ] as any ).start = modelRanges[ 0 ].start.getShiftedBy( insertText.length );
insertText = '';
}
}
}
}

In order to prove my hypothesis I chose a slightly different text with two consecutive spaces, I will now type This<space><space>this and using | to denote the caret position:

This|
This |
This  |
This  |tt
This  |hhtt
This  i|hhtt
This  is|hhtt

Yikes! This is even worse than before, we now have two characters being duplicated. Let’s try the same using three spaces:

This   |
This   t|
This   th|t
This   thi|t
This   this|t

That’s odd, we’re back to a single duplicated letter! Do one more try, but this time using four consecutive spaces to prove that two spaces is an outlier.

This    |
This    |tt
This    |hhtt
This    i|hhtt
This    is|hhtt

If you think that this looks like a pattern then you are correct, an odd number of spaces will duplicate a single letter to the right while an even number of spaces duplicates the first two letters twice to the right. Also, for an odd number of spaces the first character will also appear on the left so that you only end up with an extra letter. This is different to the even number of spaces that cause the first two characters to never appear at the start.

Cross-checking the Hypothesis

There is definitely a pattern here but I want to know if this issue is caused by a preceding space in general or just trailing spaces.

The obvious first thing to validate is to enter a single space first and then type a word (spoiler: It works just fine).

Next was trying two spaces to see if that makes any difference and … yes, it does.

|
 |
  |
  T|
  Th|T
  Thi|T
  This|T

I repeated this with three leading spaces and we’re back to the double duplication bug. Four leading spaces and it results in the single character being duplicated, five leading spaces and we get the double duplication bug again. Six leading spaces … you get the idea.

On the bright side, the behavior is identical to the previous observed issue but the number of spaces is consistently increased by one. My immediate thought was that this was caused by \u0020 (regular space) being used for the first space and \u00A0 (non-breaking space) for all subsequent ones but a quick check of the DOM reveals that a trailing space is always \u00A0.

The Aforementioned Workaround for Android in Detail

For the whole time I was running a modified version of input.ts with additional logging to provide me some insight into the internals, in particular the value of selectedText and which following branches were taken. I primarily used this to cross-check the beforeinput event data and the observed behavior on the screen.

I chose selectedtext because it has an impact on what the editor things is happening, the following is the value of selectedText for the input this (two leading spaces):

<empty string> <-- first space
<empty string> <-- second space
<empty string> <-- t (composion starts)
<empty string> <-- h
t <-- also h?
th <-- wait, this is also caused by h?

So we have three passes for – what should be – a single keystroke. insertText is set to h and then <empty string> for the second and third pass. The model range was shifted by 1 and 2 respectively.

To further investigate this I repeated the test with three leading spaces which results in the double duplication bug. Unfortunately, the value of selectedText makes even less sense here, revealing that something is really off:

<empty string> <-- 3 times, one for each space
<empty string> <-- t
<empty string> <-- t, again? The editor now shows two tt to the right of the caret.
<empty string> <-- h
<empty string> <-- h again.
<empty string> <-- i
<empty string> <-- s

It continues from here on, I can type as many characters as I want, selectedText remains a zero-length string. It’s also noteworthy that at no point the text is underlined so it appears to recognize that the user is typing in the middle of a word.

Final Thoughts

During my tests I used standalone test pages with contenteditable to see if I can reproduce any of this behavior outside of CKEditor but unfortunately I was unable to do so. I also do not see CKEditor doing anything obviously wrong here but that doesn’t mean that it isn’t causing it or at the very least triggering a browser / keyboard bug.

I have a strong suspicion that the spaces play a crucial role here, the pattern is too obvious at this point. My wild guess is that the selection is being updated to a state that throws off the browser and causes its heuristics to misinterpret the caret position. I did check getSelection().getRangeAt(0) manually to see if there is something odd, like being placed at the end of the text node vs being placed after the text node, but it always looked fine to me.

Maybe checking the state of the selection while beforeinput is being processed can shed some light onto what is happening here.

@dtdesign
Copy link
Author

dtdesign commented Feb 4, 2024

I had a few other ideas that I tried out to further pin point the issue. Since the actual DOM presented in the editor is managed by CKEditor, I cross-checked the HTML with a plain contenteditable to see if there is any difference in whitespace handling, but in both cases a trailing space is presented as \u00A0. This was to be expected and is also the case for a single whitespace in an empty line which is represented as <p>&nbsp;</p> for both.

Another idea was to check if blurring and focusing the editor again makes any difference, for example, typing Test and then moving the focus out and back again. The result remained unchanged, the same exact issue occurs.

The Duplication Takes Place after the First Keystroke

Everything points to an issue with the composition process and I was finally able to prove that in a slightly different way by typing the first character of the second word and then blurring the editor by moving the focus elsewhere. I’m using | to mark the caret position with its absence meaning that the editor is no longer focused.

Test |
Test t|
Test tt

This is remarkable because there was only one keystroke to produce the initial t, the second t was added when I blurred the editor. Blurring takes place by touching an empty space outside of the editor box which will also collapse the keyboard. The selectedText value is an empty string for both the t keystroke as well as the follow up beforeinput event which produces the duplicated character.

There is also this weird double duplication bug that kicks in when there are a multiple of two consecutive spaces. I tested the behavior for this case too and this is the result:

Test  |
Test  |tt
Test  tt

The double duplication bug immediately occurs but blurring the editor does not cause any further changes. This is to be expected because this is exactly what happened before. But this means that the initial duplication bug is the result of the end of a composition whereas the double duplication bug hits immediately. This could indicate that these two bugs are similar, yet may have slightly different causes.

Probing the Composition Events

From now on I will only focus the first case, the single letter duplication bug. Everything points to the root cause being the composition event so I set up a tracking for these events in parallel to the existing logging. The output below starts with the editor being in the following state: Test |.

// `t` is being hit
beforeinput -> selectedText = <empty string>
compositionstart -> data = <empty string>
compositionupdate -> data = "t"

// Editor is being blurred
beforeinput -> selected text = <empty string>
compositionend -> data = "t"

Once again I set up a small demo to cross-check my findings about the behavior of the composition events. The behavior is identical, the compositionend event is fired after the next keystroke or when the contenteditable is being blurred. This totally makes sense and is exactly what I did expect but I felt it was necessary to confirm that there is no divergence here.

The contenteditable shows the typed content while the composition is still in process. I was curious to see if compositionend would actually mutate the text node even if there is no visual change. For this purpose is set up a MutationObserver to verify this and I was able to confirm that the text node was changed both when the t was typed in and a second time when the blur occurs as a result of compositionend.

This reinforces my previous impression that this is entirely an issue with the selection handling because that would reasonably explain this behavior.

Verifying the Event Order

I did another check to see if the appearance and order of composition*, beforeinput and the mutation differs in any way, such as mutating the text content once too many. The observed order for both CKEditor and my test case identical, the output starts as Test |:

// `t` is being typed in
compositionstart -> data = <empty string>
compositionupdate -> data = "t"
beforeinput
MutationRecord

// blur occurs
beforeinput
compositionend -> data = "t"
MutationRecord

There is no difference between both cases so the only variable left is where CKEditor decides to place the typed in data. Considering the way the composition is being processed, the correct selection handling is crucial for the computation of the changed bits. It still bugs me that the composition is visually terminated after the first keystroke of the second word because it means that something must be going on that I am note aware of yet. A malformed or in other way incorrectly modified selection could possibly explain them both.

Well over an hour later my cup has ran out of coffee. Honestly, I didn’t expect the rabbit hole to be that deep …

@dtdesign
Copy link
Author

dtdesign commented Feb 4, 2024

I’m quite confident that it indeed is an issue with the selection. Surprisingly the debugger breakpoints have been proven to be super reliable which I really don’t take for granted when dealing with contenteditable. I have noticed the following behavior when placing the caret at the end of a word after it was somewhere else before. This is what is happening with the output starting with T.

T| <-- Caret is being placed here
|T| // <-- `h` is pressed, the selection now spans `T`
Th|

That range later matches the view selection that it passed to the insertText event handler in CKEditor. Whenever the duplication bug occurs, the provided selection is of zero length.

The change to the DOM selection only takes place when I stop editing by moving the caret anywhere else and then place it at the end of a word. The first keystroke will briefly trigger a selection of the characters in front of the caret up until the left boundary. This is easy to stop with a debugger, but can also be observed by the “Select All” popup becoming visible for a short moment.

It’s a bit annoying that the DOM selection constantly reports a collapsed selection but the behavior of the composition events suggest that there is some kind of hidden selection of the current word. In case CKEditor in some way is able to alter that selection and use it as its source of truth it would explain why both the browser and CKEditor produce the incorrect results.

At this point I’m confident that the internal selection stored in CKEditor is actually correct but the source of truth is not. That same source of truth is used by the browser to indicate the composition by underlining the word but even the browser (visually) reports that no composition in that regard is taking place.

@dtdesign
Copy link
Author

dtdesign commented Feb 4, 2024

I’ve found an interesting bit about the behavior of the spaces that has to do with how many spaces are in front of the caret and if the caret is at the boundary of the text node.

(I had to use to mark the caret position …)

Caret position Outcome
 ✔️
✔️
Single duplication
Double duplication bug to the right
Test ⋮ Single duplication
 Test ⋮ Double duplication bug to the right
Test ⋮ ✔️
Test ⋮ test ✔️
Test ⋮ test Double duplication bug to the right

Workaround for the Single Duplication Bug

One common pattern I’ve noticed is that when you place the caret at the end of a word and start to type, the composition will pick up the entire word as confirmed by the data exposed in compositionstart. This means that a possible workaround could involve capturing this data and comparing it to the selection that is stored internally. I’m not sure if that assumption actually holds up and it really isn’t fixing the root cause of it, but it might be worth a try?

I build a naive workaround that fixes the single duplication bug by adjusting selectedText when it appears to be incorrectly detected as empty. It not only prevents the single character duplication but the browser is also able to pick up the composition again.

diff --git a/packages/ckeditor5-typing/src/input.ts b/packages/ckeditor5-typing/src/input.ts
index 7716904fa0..41eadfcc0b 100644
--- a/packages/ckeditor5-typing/src/input.ts
+++ b/packages/ckeditor5-typing/src/input.ts
@@ -63,10 +63,29 @@ export default class Input extends Plugin {
 			// Typing in English on Android is firing composition events for the whole typed word.
 			// We need to check the target range text to only apply the difference.
 			if ( env.isAndroid ) {
-				const selectedText = Array.from( modelRanges[ 0 ].getItems() ).reduce( ( rangeText, node ) => {
+				let selectedText = Array.from( modelRanges[ 0 ].getItems() ).reduce( ( rangeText, node ) => {
 					return rangeText + ( node.is( '$textProxy' ) ? node.data : '' );
 				}, '' );
 
+				if ( selectedText === '' && viewSelection.isCollapsed && viewSelection.rangeCount === 1 ) {
+					const existingRange = viewSelection.getFirstRange()!;
+					if ( !existingRange.start.isAtStart ) {
+						const newPosition = view.createRange(
+							existingRange.start.getShiftedBy( -1 ),
+							existingRange.end
+						);
+
+						const newModelRange = editor.editing.mapper.toModelRange( newPosition );
+						selectedText = Array.from( newModelRange.getItems() ).reduce( ( rangeText, node ) => {
+							return rangeText + ( node.is( '$textProxy' ) ? node.data : '' );
+						}, '' );
+
+						if ( selectedText === ' ' || selectedText !== insertText ) {
+							selectedText = '';
+						}
+					}
+				}
+
 				if ( selectedText ) {
 					if ( selectedText.length <= insertText.length ) {
 						if ( insertText.startsWith( selectedText ) ) {

Maybe this is the solution after all? In that case, please feel free to steal it, the diff above is hereby released into the public domain! 🙂

@dtdesign
Copy link
Author

dtdesign commented Feb 5, 2024

Unfortunately, some more test cases showed that while the above workaround worked well for words, it broke the ability to type in two consecutive dots or spaces, or generally anything that is not a word character. While I am a bit disappointed that my attempt eventually did not hold up, I’m still surprised at the effectiveness of such a small change.

The primary weakness of my approach was that it was too simple. It simply shifted the selection by one character to the left and called it a day, which does not work if there is a non-word character. I’ve since iterated on my approach by tapping into compositionstart and compositionupdate to properly detect the text entered in order to compare it to what is stored in the model.

It works remarkably well, supporting multiple spaces and dots, properly handling compositions. There is only one caveat: For some reason it casually throws a model-nodelist-offset-out-of-bounds error in the table column plugin caused by the offset being off by one (for example, offset = 23 but offsetSize = 22).

For now I simply ignored it because the editor gracefully recovers and just continues. The error is reproducible by typing Test t followed by one more keystroke. My event logging outputs this:

// Situation: The editor contains `Test ` with the caret being at the end.
// The letter `t` is now being typed in.
compositionstart: data -> <empty string>
compositionupdate: data -> t
insertText: data.text -> t, selectedText is empty (my code resolves to a single space and reverts back to an empty string)
insertText: data.text -> t, selectedText is empty, but my code resolves it to `t` (which actually is correct)
*** Uncaught CKEditorError: model-nodelist-offset-out-of-bounds {"offset":7,"nodeList":[{"data":"Test t"}]}
// regular events continue, editor remains fully functional

Maybe someone with more insight into the internals of CKEditor can figure out what causes this off-by-one error. There is a limit to how far I can dig into the code within a few days! ;-)

(Mostly) Working Workaround / PoC Implementation

The code is a bit messy, for example, the two extra event listeners at the top aren’t ideal and certainly do not follow your general code style for persisting data between events. You might come across the regex check for the word character which is required because I’m only targeting the specific issue that occurs when the composition of a word previously failed.

Since it does not fix the original error but merely works around it, I wanted to keep my change as scoped as possible. Hard to believe it still took me 2-3 hours of testing and debugging to get here. It’s really annoying that Firefox does not allow me to edit the source on the go like Chrome does, having to wait 5 seconds for the debug build really adds up.

diff --git a/packages/ckeditor5-typing/src/input.ts b/packages/ckeditor5-typing/src/input.ts
index 7716904fa0..c2aa76489e 100644
--- a/packages/ckeditor5-typing/src/input.ts
+++ b/packages/ckeditor5-typing/src/input.ts
@@ -13,7 +13,7 @@ import { env } from '@ckeditor/ckeditor5-utils';
 import InsertTextCommand from './inserttextcommand.js';
 import InsertTextObserver, { type ViewDocumentInsertTextEvent } from './inserttextobserver.js';
 
-import type { Model } from '@ckeditor/ckeditor5-engine';
+import type { ViewDocumentCompositionStartEvent, Model, ViewDocumentCompositionUpdateEvent } from '@ckeditor/ckeditor5-engine';
 
 /**
  * Handles text input coming from the keyboard or other input methods.
@@ -44,6 +44,18 @@ export default class Input extends Plugin {
 		editor.commands.add( 'insertText', insertTextCommand );
 		editor.commands.add( 'input', insertTextCommand );
 
+		let compositionStartData: string | undefined = undefined;
+		if ( env.isAndroid ) {
+			this.listenTo<ViewDocumentCompositionStartEvent>( view.document, 'compositionstart', ( evt, data ) => {
+				compositionStartData = data.domEvent.data;
+			} );
+			this.listenTo<ViewDocumentCompositionUpdateEvent>( view.document, 'compositionupdate', ( evt, data ) => {
+				if ( compositionStartData === '' ) {
+					compositionStartData = data.domEvent.data;
+				}
+			} );
+		}
+
 		this.listenTo<ViewDocumentInsertTextEvent>( view.document, 'insertText', ( evt, data ) => {
 			// Rendering is disabled while composing so prevent events that will be rendered by the engine
 			// and should not be applied by the browser.
@@ -63,10 +75,37 @@ export default class Input extends Plugin {
 			// Typing in English on Android is firing composition events for the whole typed word.
 			// We need to check the target range text to only apply the difference.
 			if ( env.isAndroid ) {
-				const selectedText = Array.from( modelRanges[ 0 ].getItems() ).reduce( ( rangeText, node ) => {
+				let selectedText = Array.from( modelRanges[ 0 ].getItems() ).reduce( ( rangeText, node ) => {
 					return rangeText + ( node.is( '$textProxy' ) ? node.data : '' );
 				}, '' );
 
+				if ( typeof compositionStartData === 'string' && compositionStartData !== '' ) {
+					if (
+						selectedText === '' &&
+						viewSelection.isCollapsed &&
+						viewSelection.rangeCount === 1
+					) {
+						const existingRange = viewSelection.getFirstRange()!;
+						if ( !existingRange.start.isAtStart ) {
+							const newPosition = view.createRange(
+								existingRange.start.getShiftedBy( compositionStartData.length * -1 ),
+								existingRange.end
+							);
+
+							const newModelRange = editor.editing.mapper.toModelRange( newPosition );
+							selectedText = Array.from( newModelRange.getItems() ).reduce( ( rangeText, node ) => {
+								return rangeText + ( node.is( '$textProxy' ) ? node.data : '' );
+							}, '' );
+
+							if ( /^\w+$/.test( selectedText ) && selectedText === insertText ) {
+								compositionStartData = undefined;
+							} else {
+								selectedText = '';
+							}
+						}
+					}
+				}
+
 				if ( selectedText ) {
 					if ( selectedText.length <= insertText.length ) {
 						if ( insertText.startsWith( selectedText ) ) {

I hereby release this code into the public domain.

@dtdesign
Copy link
Author

dtdesign commented Feb 8, 2024

I did some further digging to investigate the “double duplication bug” which takes place after entering two consecutive spaces. After diving deeper into the batch processing and the selection handling I noticed that CKEditor appears to do just fine until the DOM selection suddenly changes. Let me explain:

  1. Given the input Test | (length=6) with | denoting the caret position.
  2. Entering t will cause the letter t to be inserted at offset 7.
  3. The model is updated and the caret position advances by one and now sits at the end Test t| (offset=8).
  4. A selectionchange event is dispatched that reports the caret to be at offset=7.
  5. The caret is now moved to the left: Test |t.
  6. compositionupdate is triggered once more followed by insertText.
  7. Like in step 2 and 3 the insertion takes place and the editor now contains Test t|t (the first t is the one to the right).
  8. One more time selectionchange is triggered and the caret is moved to offset=7 again.
  9. The editor now contains this: Test |tt.

Whatever is causing the update to the DOM selection is responsible for this entire mess. I set up a logger in DocumentSelection::setTo() to verify the offsets that are being reported and for each of those cycles the selection is updated twice. The first update properly updates the model document’s selection and the second triggers the selection reevaluation based on the mismatch with the reported DOM selection.

The problem that my workaround solves is also related to the selection, in particular that the internal selection does not keep track of the composition that started thus it assume the selection to be collapsed. This explains why my workaround above does not handle this case, because the selection is not where it should be. If the selectionchange (step 4 and 8) would not happen then my workaround would solve this issue too.

I added a switch to discard the insertText command to see what would happen and I am able to write the first word as usual, although it never appears in the model for obvious reasons. Any attempt to enter a space or punctuation will do nothing which again is to be expected and eventually the underline from the first word disappears. However, I am not able to type any further text, nothing will happen, not even the “simulated” input from the browser is visible.

I feel like I am missing some crucial here. When I start typing the first word, I get an underline from the very beginning (together with a compositionstart + compositionupdate) but for the second word I get no underline for the first letter. Once I hit the second letter the underline appears but from the perspective of the composition events there seem to be no difference between the two words.

Edit: There is one thing that I just noticed: selectionchange is triggered multiple times at once. It will fire exactly once per insertText when everything is working. But once I hit that “double duplication bug”, it will trigger three times in step 4 and 5 times in step 8. I logged out the values of getSelection().getRangeAt(0) for both startOffset and endOffset and these values are unchanged for each time it is triggered. For my test case the values go up to 6 and then stay at 6 as long s the incorrect behavior takes place.

@dtdesign
Copy link
Author

dtdesign commented Feb 9, 2024

The workaround in this issue is not required, it can be solved much easier with the fix outlined in #13994 (comment).

@winzig
Copy link

winzig commented Mar 26, 2024

@Witoso Is there going to be an official fix for this? I'm now experiencing this (or an extremely similar) issue in the latest Safari Technology Preview (Release 190), also related to predictive text.

Here is a demo of it happening on the CKEditor5 demo page:

screenshot.2024-03-26.at.15.16.07.mp4

I thought perhaps it's a bug in the tech preview, but I tried to reproduce it with the Quill demo editor, and it works fine there (and every other field I've tried this in on this version of Safari, outside of CKEditor):

screenshot.2024-03-26.at.15.14.41.mp4

@Witoso
Copy link
Member

Witoso commented Mar 27, 2024

@winzig thanks for the info! We will check this.

@dtdesign
Copy link
Author

I can reproduce this issue in Safari TP 190 and I’ve tested my fix from #13994 (comment) by changing the check for env.isAndroid to env.isAndroid || env.isSafari, but it did not improve the situation.

That means that while it is somewhat similar, this is in fact a different issue. Trying a few different things I believe that this is possibly similar to the behavior described in #15831 (which by the way lacks the proper labels). I did not perform any further investigations to confirm or disprove this hypothesis.

@Witoso I recommend moving the Safari issue into a dedicated tracking issue.

@niegowski
Copy link
Contributor

The work for this issue and others related is completed in the #16289, and we finalized the round of internal (successful) tests. I encourage everyone to test the PR if you have a chance. It should be merged in the following days, and will be a part of the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browser:firefox 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. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants