Calling writer.setSelection on an invalid selection breaks the editor

[adamerose]

📝 Provide detailed reproduction steps (if any)

1. Make a selection in the CKE5 editor and run let s = editor.model.document.selection.getFirstRange()
2. Remove text or lines such that the selection location is no longer valid (this bug won't happen if you delete the line with selected text only to have another equally long line take its place)
3. Run editor.model.change((writer) => {writer.setSelection(s)})
4. CKE will throw error Uncaught CKEditorError: model-nodelist-offset-out-of-bounds and remain broken until reloaded

✔️ Expected result

The writer.setSelection call should just throw an error and abort

❌ Actual result

The editor becomes buggy until reloaded - any time you try clicking or selecting the cursor snaps back to the failed selection location.

https://www.screencast.com/t/bUHzZVnvQ2Q

ckeditorerror.js:104 Uncaught CKEditorError: model-nodelist-offset-out-of-bounds {"offset":7,"nodeList":[{"data":"sfaafs"}]}
Read more: https://ckeditor.com/docs/ckeditor5/latest/framework/guides/support/error-codes.html#error-model-nodelist-offset-out-of-bounds
    at NodeList.offsetToIndex (nodelist.js:161:1)
    at Element.offsetToIndex (element.js:186:1)
    at getTextNodeAtPosition (position.js:1100:1)
    at get textNode [as textNode] (position.js:218:1)
    at validateTextNodePosition (document.js:473:1)
    at Document._validateSelectionRange (document.js:382:1)
    at LiveSelection.<anonymous> (documentselection.js:666:1)
    at LiveSelection.fire (emittermixin.js:199:1)
    at LiveSelection._setRanges (selection.js:471:1)
    at LiveSelection.setTo (selection.js:376:1)

[dufipl]

Hello @adamerose,

Thank you for reporting your issue. It looks like the error is correct behaviour at this moment and it's related to the incorrect use of API. Such error often requires restarting CKEditor5/using watchdog.

Could you please describe to me your use case? I would like to understand what effect you are trying to achieve. :)

[adamerose]

Could you please describe to me your use case? I would like to understand what effect you are trying to achieve. :)

I am working on a VS Code extension using CKEditor5 as a custom text editor for editing markdown files. A snippet that can lead to this error is shown below. When a markdown document is open in the CKEditor5 editor and the user modifies it elsewhere and saves, the CKEditor5 contents gets updated to reflect the latest file contents. I do this using setData and writer.setSelection.

const selection = editor.model.document.selection.getFirstRange();
editor.setData(text);
editor.model.change((writer) => {
	writer.setSelection(selection);
});

I created a function to work around this error by validating the selection before entering the writer block which solves the problem on my end, but it relies on an undocumented function editor.model.document._validateSelectionRange. You can see the workaround and the full repo here: https://github.com/adamerose/vscode-markdown-editor/blob/a1e4ffa1e80a876b4ff2774ae9d7f1aa666441fd/src/markdownEditorInitScript.js#L48

It looks like the error is correct behaviour at this moment and it's related to the incorrect use of API.

I would think the correct behavior when incorrectly using the API should be throwing an error, not breaking the editor.

[arkflpc]

@adamerose, can you provide the specific data to reproduce the exception? I mean the value of text in your code and the state of the editor before you call getFirstRange?

[Reinmar]

We discussed this during an internal meeting. Conclusions:

• setSelection() should validate the input before changing anything in the model. Ideally, if you put it in try-catch, it should not break your editor.
• There are dozens of other places where we should add more validation. Again, in a way that if an error is thrown, the method did nothing. We can't of course update all these places at once.
• Optionally: We can also expose the validateSelectionRange() as it's useful.
• Question: Should we in any way differentiate critical (non-recoverable) and recoverable errors? How would e.g. @adamerose know whether it's fine to wrap setSelection() with try-catch?
  • This would be valuable.
  • But definitely a lot of work to understand, document and cover with tests the recoverable errors.
  • Useful for Watchdog too.

[Reinmar]

I am working on a VS Code extension using CKEditor5

What a cool idea :heart_eyes: Is the plugin usable already? I'd love to install it.

[Reinmar]

Let's try this:

• Touch only DocumentSelection. The Selection must allow "virtual" positions.
• Add early validation with a clear error message and documentation. For now, we won't be differentiating recoverable and non-recoverable errors (possible followup).
• There's a risk that many tests will start to fail. If too many (2h+), let's abort this task.

The followups (other methods that should have better validation, other ideas) are covered in #4260.

[urbanspr1nter]

We discussed this during an internal meeting. Conclusions:

• setSelection() should validate the input before changing anything in the model. Ideally, if you put it in try-catch, it should not break your editor.

• There are dozens of other places where we should add more validation. Again, in a way that if an error is thrown, the method did nothing. We can't of course update all these places at once.

• Optionally: We can also expose the validateSelectionRange() as it's useful.

• Question: Should we in any way differentiate critical (non-recoverable) and recoverable errors? How would e.g. @adamerose know whether it's fine to wrap setSelection() with try-catch?

  • This would be valuable.
  • But definitely a lot of work to understand, document and cover with tests the recoverable errors.
  • Useful for Watchdog too.

On the point of differentiating the critical recoverable vs non-recoverable errors from the editor, this would be immensely helpful. For our integration, we'd like to minimize the amount of editor crashes, as it is hard to detect when it has happened on for the user. (Our integration is quite complex which leads to the Editor watchdog being difficult to implement)

I came across this post a month ago, and implemented try-catch around setSelection where it made sense, and doing so has helped tremendously. Would be very useful to know what other types of operations can be "hardened" through wrapping a try-catch around them... :)

[adamerose]

Thanks for the fix everyone!

What a cool idea 😍 Is the plugin usable already? I'd love to install it.

There's still much to improve but I finally got around to publishing it. It would not have been a viable project if I didn't have CKE5 to do all the heavy lifting!