DOM selection change will not be converted if the selection was placed outside of the editable element

[scofalik]

Suggested merge commit message (convention)

Fix: DOM selection change will not be converted if the selection was placed outside of the editable element. Closes ckeditor/ckeditor5#4199.

[Reinmar]

Tests don't pass.

[scofalik]

Weird. Works for me locally on FF v. 72.0.2:

(master 383e19d M1) ~/CKS/ckeditor5> yarn test --files=engine/view/observer/selectionobserver.js --browsers=Firefox
yarn run v1.19.1
$ node --max_old_space_size=4096 node_modules/@ckeditor/ckeditor5-dev-tests/bin/test.js --files=engine/view/observer/selectionobserver.js --browsers=Firefox
⚠ Console use is allowed. Use `--disallow-console-use` to disallow console use.
(node:31433) DeprecationWarning: Tapable.plugin is deprecated. Use new API on `.hooks` instead
ℹ ｢wdm｣: 
ℹ ｢wdm｣: Compiled successfully.

START:
ℹ ｢wdm｣: Compiling...
ℹ ｢wdm｣:    711 modules
ℹ ｢wdm｣: Compiled successfully.
21 01 2020 16:53:09.463:INFO [karma-server]: Karma v4.4.1 server started at http://0.0.0.0:9876/
21 01 2020 16:53:09.466:INFO [launcher]: Launching browsers Firefox with concurrency unlimited
21 01 2020 16:53:09.469:INFO [launcher]: Starting browser Firefox
21 01 2020 16:53:11.378:INFO [Firefox 72.0.0 (Mac OS X 10.15.0)]: Connected on socket IzF1Y_Y3Xv0bLmtBAAAA with id 2521783
  SelectionObserver
    ✔ should fire selectionChange when it is the only change
    ✔ should add only one listener to one document
    ✔ should not fire selectionChange on render
    ✔ should not fire if observer is disabled
    ✔ should not fire if the DOM selection was set outside editable
    ✔ should not enter infinite loop
    ✔ should not be treated as an infinite loop if selection is changed only few times
    ✔ should not be treated as an infinite loop if changes are not often
    ✔ should fire `selectionChangeDone` event after selection stop changing
    ✔ should not fire `selectionChangeDone` event when observer will be destroyed
    ✔ should re-render view if selections are similar if DOM selection is in incorrect place

Finished in 2.948 secs / 1.623 secs @ 16:53:15 GMT+0100 (Central European Standard Time)

SUMMARY:
✔ 11 tests completed
✨  Done in 10.13s.

[scofalik]

Okay, the build passed.

FF was throwing error when changing selection to a non-focused div. But I had to focus the browser window first to even see it failing.

[coveralls]

Coverage remained the same at 100.0% when pulling e069a9c on i/4199 into b2a8189 on master.

[Reinmar]

This change definitely requires a lot of manual testing of various scenarios. Could you write down some things that you think that we should have checked?

[scofalik]

The purpose of this PR is to have the selection not change when you click outside the editor editable element. This is the main thing to test. For example, if the first element is a heading, put a selection in the paragraph, click outside of the editor and then check if the value in heading dropdown changed. Check it in normal mode and in read-only mode.

Other than that, this change is a little difficult to test, because the model selection is invisible when you blur the editable. You can use CKE5 Inspector to help you with that. Put selection in the editor, then blur it (you can use Tab to have another use case) and then click around.

[scofalik]

This PR is ready for review and was tested on Cloud Features side.

[Reinmar]

I asked @Mgsy to think about what needs to be tested here (various cases, features with UI, different browsers, etc.) and check those things. I've got to say – I'm quite afraid of this change :D

[FilipTokarski]

[ Firefox ]

Steps:

• Select some text
• Click on the left/right side, outside of the editor

Expected: selection in model stays the same

Actual: selection in model changes

Note: it doesn't happen if you click above the editor

[FilipTokarski]

[ Chrome, Safari ]

Steps:

• set editor.isReadOnly = true;
• select some text
• click inside the selection

Expected: visual selection and model selection change

Actual: visual selection changes, you need to click again to change selection in model

[Mgsy]

We've tested it in different browsers and besides issues reported by @FilipTokarski, those changes look fine.

[scofalik]

#1814 (comment)

I don't know if this is something we consider really bad. It is true, that on FF you can make selection by "dragging" the mouse next to the editor. But, I guess, this is not our fault that FF sets the selection there? IDK. @Reinmar? If I had to guess, I'd say that it won't cause any bugs.

#1814 (comment)

This will be difficult / impossible to fix if we will use the solution I implemented. Unfortunately, when you click inside the selection, the selection is removed from the document (I mean DOM document). This is why it is not changed in the editor. After the second click, the DOM selection is set again.

[Reinmar]

I don't know if this is something we consider really bad. It is true, that on FF you can make selection by "dragging" the mouse next to the editor. But, I guess, this is not our fault that FF sets the selection there? IDK. @Reinmar? If I had to guess, I'd say that it won't cause any bugs.

It's an interesting finding. I recall this issue from a looong time ago :D But it indeed shouldn't be a big deal.

This will be difficult / impossible to fix if we will use the solution I implemented. Unfortunately, when you click inside the selection, the selection is removed from the document (I mean DOM document). This is why it is not changed in the editor. After the second click, the DOM selection is set again.

I'd say that it's not a big deal as well. The read only mode is not something you normally operate in. Additionally, those kind of issues are natural to self-correct by the user. You don't see the editor reacting to your action, you try 2nd time, perhaps differently.

---

Thanks @FilipTokarski @Mgsy @scofalik for all your input. I'll read the code again and merge this PR.