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

Composition breaks when typing a Korean phonetic radical after typing space key on Android Chrome #13693

Closed
byeokim opened this issue Mar 16, 2023 · 10 comments · Fixed by #16289
Labels
domain:typing/ime This issue reports a problem with standard typing & IME (typing method for CJK languages). support:2 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior. type:regression This issue reports a bug that was not present in the previous releases.

Comments

@byeokim
Copy link

byeokim commented Mar 16, 2023

📝 Provide detailed reproduction steps (if any)

  1. Launch any CKEditor, e.g. this demo, on Android Chrome
  2. Put caret at the start of any empty line
  3. Use GBoard keyboard
  4. Add keyboard language: select Korean then 2-Bulsik
  5. Change keyboard language to Korean
  6. Type
  7. Type
  8. Type space
  9. Type
  10. Type
screenrecording.webm

✔️ Expected result

가 가

❌ Actual result

가 ㄱㅏ

❓ Possible solution

Not updating Text node when composing, i.e. after compositionstart event and before compositionend event, by adding the following code into renderer.ts:

// Update the existing text node data. Note that replace action is generated only for Android for now.
else if ( action === 'replace' ) {
// @if CK_DEBUG_TYPING // if ( ( window as any ).logCKETyping ) {
// @if CK_DEBUG_TYPING // console.group( '%c[Renderer]%c Update text node',
// @if CK_DEBUG_TYPING // 'color: green;font-weight: bold', ''
// @if CK_DEBUG_TYPING // );
// @if CK_DEBUG_TYPING // }
updateTextNode( actualDomChildren[ i ] as DomText, ( expectedDomChildren[ i ] as DomText ).data );
i++;
// @if CK_DEBUG_TYPING // if ( ( window as any ).logCKETyping ) {
// @if CK_DEBUG_TYPING // console.groupEnd();
// @if CK_DEBUG_TYPING // }
} else if ( action === 'equal' ) {

else if ( action === 'replace' ) {
+ if (!this.isComposing) {
    updateTextNode( actualDomChildren[ i ] as DomText, ( expectedDomChildren[ i ] as DomText ).data );
+ }
  i++;

On Android Chrome, when a character is typed and the typing triggers a compositionstart event, IME seems to put the character into the Text node sometime after compositionupdate event. Meanwhile CKEditor also effectively puts the character into the node by invoking updateTextNode. So there would be two same characters. MutationObserver detects the DOM mutation and invokes another updateTextNode. Because of that invocation one of the two characters is removed. This can be confirmed by uncommenting CK_DEBUG_TYPING console.logs. I suspect that which character is removed affects whether the current composition should break, i.e. IME seems to be keeping track of which character instance is involved in the current composition.

The suggested solution will prevent the character duplication. IME will put the character while CKEditor won't. Thus there would be no removal of the duplicated character. Neither be composition-breaking.

📃 Other details

  • Browser: Chrome 91.0
  • OS: Android 12
  • First affected CKEditor version: 36.0.1
  • Installed CKEditor plugins: Essentials, Paragraph

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

@byeokim byeokim added the type:bug This issue reports a buggy (incorrect) behavior. label Mar 16, 2023
@tkmin
Copy link

tkmin commented Mar 20, 2023

bug: typing Korean on Safari at v34 was resolved in v35,
but the Android bug described above was not occured when the version was 34.

@Almyk
Copy link

Almyk commented Apr 4, 2023

CKEditor version 35.4.0 is also affected by this bug.

@Witoso Witoso added type:regression This issue reports a bug that was not present in the previous releases. domain:typing/ime This issue reports a problem with standard typing & IME (typing method for CJK languages). labels Apr 5, 2023
@bungabear
Copy link

bungabear commented May 3, 2023

Your solution seems works well at 36.0.1

@tiger-noncelab
Copy link

👍🙏

@tkmin
Copy link

tkmin commented May 8, 2023

@noncelab-tiger
@bungabear

could you please tell me which method is used to build the editor?
I used the online builder and the renderer.ts file cannot be found in my project.

Thanks in advanced! 🙇‍♂️

Screen Shot 2023-05-08 at 2 55 32 PM

@bungabear
Copy link

@noncelab-tiger @bungabear

could you please tell me which method is used to build the editor? I used the online builder and the renderer.ts file cannot be found in my project.

Thanks in advanced! 🙇‍♂️

Screen Shot 2023-05-08 at 2 55 32 PM

You should edit node_modules/@ckeditor/ckeditor5-engine/src/view/renderer.js file.

@tiger-noncelab
Copy link

I used the online builder. I modified the code in the path below. The code is minified so it may be hard to find.
In my case, it looked like this.

node_modules/ckeditor5-custom-build/build/ckeditor.js

if (t === "insert") {
    bc(n, c, r[c]);
    c++
} else if (t === "replace") {
    Uf(o[c], r[c].data); // this code
    c++
} else if (t === "equal") {
    this._markDescendantTextToSync(this.domConverter.domToView(r[c]));
    c++
}

@dancinlife
Copy link

Very helpful!

@justplayboard
Copy link

justplayboard commented Mar 27, 2024

I used the online builder. I modified the code in the path below. The code is minified so it may be hard to find. In my case, it looked like this.

node_modules/ckeditor5-custom-build/build/ckeditor.js

if (t === "insert") {
    bc(n, c, r[c]);
    c++
} else if (t === "replace") {
    Uf(o[c], r[c].data); // this code
    c++
} else if (t === "equal") {
    this._markDescendantTextToSync(this.domConverter.domToView(r[c]));
    c++
}

If the following error occurs, correct it as follows:

Uncaught Error: Failed to execute 'setStart' on 'Range': The offset 2 is larger than the node's length (1).
    at Dm.viewRangeToDom (ckeditor.js:13564:1)
    at wb.scrollToTheSelection (ckeditor.js:15308:1)
    at e.<anonymous> (ckeditor.js:32234:1)
    at e.fire (ckeditor.js:3605:1)
    at Af (ckeditor.js:11685:1)
    at xf.fire (ckeditor.js:11638:1)
    at e.<anonymous> (ckeditor.js:32156:1)
    at e.fire (ckeditor.js:3605:1)
    at Af (ckeditor.js:11685:1)
    at xf.fire (ckeditor.js:11638:1)

solution:

node_modules/ckeditor5-custom-build/build/ckeditor.js

    if (n === o) {
        yield this._getBlockFiller()
    }
}
viewRangeToDom(t) {
    const e = this.viewPositionToDom(t.start);
    const n = this.viewPositionToDom(t.end);
    const o = this._domDocument.createRange();
	if(e.offset > 0 || n.offset > 0){    // new code line
		o.setStart(e.parent, e.offset - 1);    // new code line
		o.setEnd(n.parent, n.offset - 1);    // new code line
	} else {    // new code line
		o.setStart(e.parent, e.offset);
		o.setEnd(n.parent, n.offset);
	}    // new code line
        return o
}
viewPositionToDom(t) {
    const e = t.parent;
    if (e.is("$text")) {
        const n = this.findCorrespondingDomText(e);

@aldonace-wu aldonace-wu added the support:2 An issue reported by a commercially licensed client. label Apr 4, 2024
@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.

@CKEditorBot CKEditorBot added this to the iteration 77 milestone Jul 3, 2024
@pomek pomek modified the milestones: iteration 77, iteration 76 Jul 4, 2024
eliandoran pushed a commit to TriliumNext/trilium-ckeditor5 that referenced this issue Nov 8, 2024
Fix (typing, engine): Predictive text should not get doubled while typing. Closes ckeditor#16106.

Fix (engine): The reverse typing effect should not happen after the focus change. Closes ckeditor#14702. Thanks, @urbanspr1nter!

Fix (engine, typing): Typing on Android should avoid modifying DOM while composing. Closes ckeditor#13994. Closes ckeditor#14707. Closes ckeditor#13850. Closes ckeditor#13693. Closes ckeditor#14567. Closes: ckeditor#11569.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:typing/ime This issue reports a problem with standard typing & IME (typing method for CJK languages). support:2 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior. type:regression This issue reports a bug that was not present in the previous releases.
Projects
None yet
Development

Successfully merging a pull request may close this issue.