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

[Android/IME] Selecting a column/row clears its content #12369

Closed
Mgsy opened this issue Aug 31, 2022 · 5 comments
Closed

[Android/IME] Selecting a column/row clears its content #12369

Mgsy opened this issue Aug 31, 2022 · 5 comments
Assignees
Labels
browser:android 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. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@Mgsy
Copy link
Member

Mgsy commented Aug 31, 2022

📝 Provide detailed reproduction steps (if any)

  1. Go to the all features manual test.
  2. Put the caret in a table cell.
  3. Use the table toolbar and click on the Select column button.

✔️ Expected result

The column is selected.

❌ Actual result

The column content disappears.

📃 Other details

The same scenario applies to table rows.

Branch: ck/11438-beforeinput-ime-research-vol1.1-android
Language: English
Keyboards: GBoard, Android keyboard
Device: Nexus 10 emulator@Android v12


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

@Mgsy Mgsy added type:bug This issue reports a buggy (incorrect) behavior. browser:android 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 Aug 31, 2022
@Reinmar
Copy link
Member

Reinmar commented Aug 31, 2022

I can reproduce this in the all features manual test, but it doesn't happen always. It seems to depend on the content of a table.

E.g. I can only reproduce this in the first column here:

We observed that Chrome fires compositionstart and compositionend events right after clicking the "Select column" button. Probably, the content gets cleared on compositionstart by our handler.

@Reinmar
Copy link
Member

Reinmar commented Sep 7, 2022

Fix for this issue may also resolve #12383.

@Reinmar
Copy link
Member

Reinmar commented Sep 7, 2022

Idea: For Android only, replace the compositionstart listener with a keydown:229 listener.

The current listener that calls deleteContent() is executed before the moment when we change the isComposing flag to true. The keydown:229 is fired before compositionstart so we'll not affect the order here.

@Reinmar
Copy link
Member

Reinmar commented Sep 7, 2022

Something like this seems to work (of course, we still need the compositionstart for other browsers and do the new thing only on Android).

diff --git a/packages/ckeditor5-typing/src/input.js b/packages/ckeditor5-typing/src/input.js
index a7a644bd93..32f3f46853 100644
--- a/packages/ckeditor5-typing/src/input.js
+++ b/packages/ckeditor5-typing/src/input.js
@@ -97,19 +97,20 @@ export default class Input extends Plugin {

 		// Note: The priority must precede the CompositionObserver handler to call it before
 		// the renderer is blocked, because we want to render this change.
-		this.listenTo( view.document, 'compositionstart', () => {
+		this.listenTo( view.document, 'keydown', ( evt, data ) => {
 			if ( modelSelection.isCollapsed ) {
 				return;
 			}

-			// @if CK_DEBUG_TYPING // if ( window.logCKETyping ) {
-			// @if CK_DEBUG_TYPING // 	console.log( '%c[Input]%c Composition start -> model.deleteContent()',
-			// @if CK_DEBUG_TYPING // 		'font-weight: bold; color: green;', '',
-			// @if CK_DEBUG_TYPING // 		`[${ modelSelection.getFirstPosition().path }]-[${ modelSelection.getLastPosition().path }]`
-			// @if CK_DEBUG_TYPING // 	);
-			// @if CK_DEBUG_TYPING // }
-
-			model.deleteContent( modelSelection );
+			if ( data.keyCode === 229 ) {
+				// @if CK_DEBUG_TYPING // if ( window.logCKETyping ) {
+				// @if CK_DEBUG_TYPING // 	console.log( '%c[Input]%c Composition start -> model.deleteContent()',
+				// @if CK_DEBUG_TYPING // 		'font-weight: bold; color: green;', '',
+				// @if CK_DEBUG_TYPING // 		`[${ modelSelection.getFirstPosition().path }]-[${ modelSelection.getLastPosition().path }]`
+				// @if CK_DEBUG_TYPING // 	);
+				// @if CK_DEBUG_TYPING // }
+				model.deleteContent( modelSelection );
+			}
 		} );
 	}
 }

Make sure to add a comment pointing to this issue cause this will be WTH in the future.

@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 Sep 7, 2022
@niegowski niegowski self-assigned this Sep 7, 2022
@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 Sep 7, 2022
@Mgsy
Copy link
Member Author

Mgsy commented Sep 8, 2022

Fixed on ck/11438-beforeinput-ime-research-vol1.1-android branch.

@Mgsy Mgsy closed this as completed Sep 8, 2022
@Mgsy Mgsy added this to the iteration 57 milestone Sep 8, 2022
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Sep 8, 2022
@Reinmar Reinmar modified the milestones: iteration 57, upcoming Sep 13, 2022
@Reinmar Reinmar modified the milestones: upcoming, iteration 58 Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browser:android 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. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

No branches or pull requests

4 participants