IME/typing bug-fixing

[niegowski]

Suggested merge commit message (convention)

Fix (typing, engine): Predictive text should not get doubled while typing. Closes #16106.

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

Fix (engine, typing): Typing on Android should avoid modifying DOM while composing. Closes #13994. Closes #14707. Closes #13850. Closes #13693. Closes #14567. Closes: #11569.

TODO

---

Additional information

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

[coderabbitai[bot]]

Walkthrough

The recent updates to the CKEditor 5 codebase involve refactoring event handlers into private methods for better encapsulation and maintainability, adding new event listeners for improved handling of focus and composition events, and refining the logic for managing text insertion and selection changes. These changes aim to address specific issues related to predictive text and reverse typing effects in Safari.

Changes

Files/Paths	Change Summary
packages/ckeditor5-engine/src/view/observer/focusobserver.ts	Refactored focus and blur event handlers into private methods _handleFocus() and _handleBlur(). Added a new event handler for beforeinput. Centralized timeout handling into _clearTimeout().
packages/ckeditor5-engine/src/view/observer/selectionobserver.ts	Imported ViewDocumentCompositionStartEvent. Adjusted _handleSelectionChange to accept domDocument only. Added a new event listener for composition start.
packages/ckeditor5-engine/tests/view/observer/focusobserver.js	Added test cases for isFocused behavior under different conditions.
packages/ckeditor5-engine/tests/view/observer/selectionobserver.js	Imported priorities from @ckeditor/ckeditor5-utils. Added a test case for selectionChange event during composition start.
packages/ckeditor5-typing/src/input.ts	Modified import statements for types. Adjusted handling of model ranges based on view selection. Refined event listeners for keydown and compositionstart events.
packages/ckeditor5-typing/src/inserttextobserver.ts	Revised logic for inserting composed text. Made selection property optional in InsertTextEventData interface.
packages/ckeditor5-typing/tests/input.js	Imported DomEventData. Modified tests for model document selection and updating model selection during composition events.
packages/ckeditor5-typing/tests/inserttextobserver.js	Modified assertion related to selection property, checking if it is undefined instead of view.document.selection.

Assessment against linked issues

Objective	Addressed	Explanation
Fix predictive text "double typing" effect in Safari (Issue #16106)	✅
Fix reverse typing effect after focus change in Safari (Issue #14702)	✅

---

Recent Review Details

Configuration used: CodeRabbit UI Review profile: CHILL

Commits

Files that changed from the base of the PR and between ac70a6047407795b27dada389e54c48f1ebd7f94 and 3945477baee30716fb27f1b644895fa05f9cf02e.

Files selected for processing (8)

• packages/ckeditor5-engine/src/view/observer/focusobserver.ts (3 hunks)
• packages/ckeditor5-engine/src/view/observer/selectionobserver.ts (5 hunks)
• packages/ckeditor5-engine/tests/view/observer/focusobserver.js (1 hunks)
• packages/ckeditor5-engine/tests/view/observer/selectionobserver.js (2 hunks)
• packages/ckeditor5-typing/src/input.ts (4 hunks)
• packages/ckeditor5-typing/src/inserttextobserver.ts (2 hunks)
• packages/ckeditor5-typing/tests/input.js (3 hunks)
• packages/ckeditor5-typing/tests/inserttextobserver.js (1 hunks)

Files skipped from review due to trivial changes (1)

• packages/ckeditor5-engine/tests/view/observer/focusobserver.js

Additional comments not posted (33)

packages/ckeditor5-engine/src/view/observer/focusobserver.ts (5)

52-59: Ensure the beforeinput event listener does not interfere with other high-priority listeners.

Verification successful

---

No conflicts found with the beforeinput event listener priority.

Scripts Executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Description: Verify the priority of the `beforeinput` event listener.

# Test: Search for other `beforeinput` event listeners. Expect: No conflicts with highest priority.
rg --type typescript $'beforeinput'

Length of output: 69

---

---

Script:

#!/bin/bash
# Description: Verify the priority of the `beforeinput` event listener.

# Test: Search for other `beforeinput` event listeners in TypeScript files. Expect: No conflicts with highest priority.
rg 'beforeinput' --glob '*.ts'

Length of output: 2650

---

---

Script:

#!/bin/bash
# Description: Verify the priority of the `beforeinput` event listener.

# Test: Search for `beforeinput` event listeners with a specified priority in TypeScript files. Expect: No conflicts with highest priority.
rg 'beforeinput' --glob '*.ts' | rg 'priority'

Length of output: 44

---

83-85: LGTM! The destroy method correctly clears the timeout.

---

90-107: LGTM! The _handleFocus method handles focus changes effectively.

---

112-123: LGTM! The _handleBlur method correctly updates the focus state.

---

128-131: LGTM! The _clearTimeout method effectively clears pending timeouts.

packages/ckeditor5-typing/src/inserttextobserver.ts (3)

115-120: Ensure the compositionend event listener does not interfere with other low-priority listeners.

Verification successful

---

compositionend Event Listener Priority Verification

The compositionend event listener in inserttextobserver.ts is set with a low priority. This ensures it does not interfere with other low-priority listeners.

• viewDocument.on<ViewDocumentCompositionEndEvent>( 'compositionend', ( evt, { data, domEvent } ) => { ... }, { priority: 'low' } );

No conflicts with other low-priority listeners were found.

Scripts Executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Description: Verify the priority of the `compositionend` event listener.

# Test: Search for other `compositionend` event listeners. Expect: No conflicts with lowest priority.
rg --type typescript $'compositionend'

Length of output: 72

---

---

Script:

#!/bin/bash
# Description: Verify the priority of the `compositionend` event listener.

# Find all TypeScript files and search for `compositionend` event listeners.
fd -e ts -x rg 'compositionend' {}

Length of output: 719

---

---

Script:

#!/bin/bash
# Description: Verify the priority of the `compositionend` event listener.

# Search for `compositionend` event listeners and their priorities in TypeScript files.
fd -e ts -x rg 'on<.*compositionend' {}

Length of output: 220

---

---

Script:

#!/bin/bash
# Description: Verify the priority of the `compositionend` event listener.

# Search for `compositionend` event listeners with context lines to check their priorities.
fd -e ts -x rg 'on<.*compositionend' -C 3 {}

Length of output: 884

---

122-122: No changes needed for the observe method.

---

Line range hint 124-124: No changes needed for the stopObserving method.

packages/ckeditor5-typing/src/input.ts (2)

123-126: Ensure the keydown event listener does not interfere with other listeners.

---

Line range hint 153-167: LGTM! The deleteSelectionContent function correctly handles content deletion.

packages/ckeditor5-typing/tests/inserttextobserver.js (14)

Line range hint 28-35: LGTM! The test case correctly verifies the initialization of the observer.

---

Line range hint 37-42: LGTM! The test case correctly verifies the behavior when the observer is disabled for beforeinput event.

---

Line range hint 44-49: LGTM! The test case correctly verifies the behavior when the observer is disabled for composition event.

---

Line range hint 51-55: LGTM! The test case correctly verifies that other input types are ignored.

---

Line range hint 57-67: LGTM! The test case correctly verifies the propagation behavior of the beforeinput event.

---

Line range hint 69-78: LGTM! The test case correctly verifies the propagation behavior of the beforeinput event.

---

Line range hint 80-95: LGTM! The test case correctly verifies that preventDefault is not called for the beforeinput event.

---

Line range hint 97-111: LGTM! The test case correctly verifies the handling of insertText input type.

---

Line range hint 113-127: LGTM! The test case correctly verifies the handling of insertReplacementText input type.

---

Line range hint 129-135: LGTM! The test case correctly verifies the handling of compositionend event.

---

Line range hint 137-143: LGTM! The test case correctly verifies the handling of empty compositionend event.

---

Line range hint 145-161: LGTM! The test case correctly verifies that the focus observer is flushed to enable selection rendering.

---

Line range hint 163-217: LGTM! The test case correctly verifies the behavior in Android environment.

---

Line range hint 219-221: LGTM! The test case correctly verifies the implementation of stopObserving method.

packages/ckeditor5-engine/src/view/observer/selectionobserver.ts (3)

22-22: The import statement for ViewDocumentCompositionStartEvent looks good and is necessary for the new event listener.

---

132-132: The _handleSelectionChange method signature change simplifies the code and aligns with the new event handling logic.

---

192-198: The addition of the compositionstart event listener to handle selection changes synchronously is a crucial fix for the reverse typing effect issue.

packages/ckeditor5-typing/tests/input.js (5)

6-6: Global declarations for document and window are necessary for the tests to run in a browser environment.

---

11-11: Importing DomEventData is necessary for creating instances used in the tests.

---

195-215: The setup and initial assertions correctly ensure that the model selection is updated to match the DOM selection during compositionstart.

---

216-227: The handling of the compositionend event and the assertions ensure that the insertText command is called with the expected text and selection.

---

195-227: The test case is well-structured and covers the necessary scenarios for handling composition events, ensuring the correct behavior of the insertText command.

packages/ckeditor5-engine/tests/view/observer/selectionobserver.js (1)

196-236: The new test case for firing selectionChange synchronously on the compositionstart event looks good. It ensures that the event is fired with the correct data and at the correct priority.

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[winzig]

@niegowski What's the ETA on when this will be released, what version of CKEditor would it be in?

[Witoso]

We are starting preparation for the release that is scheduled for the next week. Most likely it will be 43.0.0.