I/5840: Safe fixes to enforcing restricted editing restrictions

[jodator]

Suggested merge commit message (convention)

Fix: Restricted editing boundaries should not be crossed by delete content and input command. Closes ckeditor/ckeditor5#5840.

---

Additional information

• Safe subset of changes from the I/5840: Enforce restricted editing restrictions #5:

1. Deleting content using model.deleteContent() used by delete command with a "word" modifier (ctrl + del)
   Fixed by changing selection argument passed to deleteContent method (using decorator)
2. Inserting content using model.insertContent() which was triggered by native spell check (in Firefox)
   Fixed by a decorator for InputCommand.execute() that blocks execute() for ranges that crosses one exception marker.

[Reinmar]

@Mgsy, could you check how it works? Please take into consideration that it's just part of the changes needed to finish this.

[Reinmar]

From a code perspective, I've got just this one comment.

[Reinmar]

I don't think that you should be modifying the selection passed to the command. Instead, override args[ 0 ] with a new selection instance.

Also, DocumentSelection doesn't even have setTo(). Please add a test for that – I think it would crash.

[Mgsy]

Steps to reproduce

1. Enter the restricted editing mode.
2. Select some part of the exception marker.
3. Press Spacebar / Enter.

Current result

The editor crashes.

GIF

Error

Uncaught TypeError: selection.setTo is not a function
    at Model.<anonymous> (restrictededitingmodeediting.js:334)
    at Model.fire (emittermixin.js:209)
    at Model.<computed> [as deleteContent] (observablemixin.js:259)
    at Object.callback (injectunsafekeystrokeshandling.js:114)
    at Model._runPendingChanges (model.js:784)
    at Model.enqueueChange (model.js:226)
    at deleteSelectionContent (injectunsafekeystrokeshandling.js:113)
    at handleUnsafeKeystroke (injectunsafekeystrokeshandling.js:84)
    at Document.view.document.on.priority (injectunsafekeystrokeshandling.js:30)
    at Document.fire (emittermixin.js:209)

Browser: All browsers

[jodator]

Uncaught TypeError: selection.setTo is not a function

Guess you're right @Reinmar : #7 (comment) 😉

[Mgsy]

@FilipTokarski, can you test this PR in Firefox and Safari? I'll take Chrome and Edge.

[jodator]

@Reinmar the problem with args[ 0 ] is that the DeleteCommand uses the selection instance that it passes to the model.deleteContent() to set selection after deleting something. Normally it would work OK. But if you remove a word that is on exception marker boundary we neeed to modify selection to only part of a word that is inside exception.

This works with arg[ 0 ] replacement as we don't delete too much.

The problem is that DeleteCommand will use original selection to re-set selection (this is used for some edge-cases in undo and paragraph merging/spliting AFAICS). In that prticular case the selection will be ugly and set to random position or even might break the editor with model-nodelist-offset-out-of-bounds.

That's why this hack on hack must use selection.setTo() for most cases and arg[ 0 ] substitution when document.selection is passed to model.deleteContent().

[FilipTokarski]

When using spellchecker some words are moved outside the exception marker. It seems to happen more often when spellchecker is suggesting longer words and happens both at the begginning and at the end of the exception marker.

Steps to reproduce:

1. Enter restricted mode
2. Type some word incorrectly
3. Select a word from spellchecker suggestions

NOTE: It happens only on some words, seems a bit random.

[jodator]

NOTE: It happens only on some words, seems a bit random.

I've noticed similar thing on the left side of the marker. I've hoped that the right sign will behave the same 😞 I'll look into it.

[jodator]

@FilipTokarski / @Mgsy - could one of you re-test the current solution? I think that I've fixed the spell-checking bug on the exception field boundary. It looks like a problematic case was when the spellchecking occurred on the boundary:

Foo [bar uguabuga] baz

fixing "ugabuga" to "bugaboo" (whatever Chrome thinks that it should be 🤷‍♂️) was
creating a state like this:

Foo [bar ]ugabuga baz

The second example in the screencast is basically the same bug but RE "fixed" the restricted editing exception only by 1 character and not by the whole change.

[Reinmar]

@jodator, I actually changed my mind regarding overriding the selection. Since this is deleteContent() you do expect the output selection to be different than the input selection. So, ignore my previous comment which clearly didn't make sense.

We should, however, make sure to not change the behaviour of whether the reference is kept. IDK how it works right now but if we operate on the exact same instance of selection that was passed to deleteContent() then we need to preserve that.

Does it make more sense now?

[Reinmar]

BTW, I'd like to add that if we'll struggle with handling those commands by overriding the selection, we may consider reverting "invalid" changes in post fixers. By invalid, I mean changes that violate the restrictions. Reverting them will not create the best UX, but should be easier and solve the issue. After all, the scenarios that we're fixing right now are not the most common ones so they don't have to be perfect as long as the restrictions are enforced.

[FilipTokarski]

I checked it on all browsers and it seems to be working ok now

[jodator]

@Mgsy can we ask you guys to check this one more time. The change is minimal but can have other side effects. Hopefully, this is the last iteration for this PR 🤞.

[FilipTokarski]

LGTM

[Mgsy]

LGTM 👌

[Reinmar]

Thanks, guys!