Use Selection.prototype.containsNode for better caret tracking

[tuespetre]

There are a lot of open issues so I am not sure what issues if any this would close, but here is what I've got:

Chrome

Always fires focus before selectionchange

Edge

Always fires focus before selectionchange

IE

Always fires focus after selectionchange

Firefox

Fires focus after selectionchange when focus was not in the document

Fires focus after selectionchange when the caret is placed after all nodes in the editor

Fires focus before selectionchange otherwise

The SelectionObserver was checking whether or not the editor was focused and using that to decide whether or not it could skip firing the selectionChange event for the Document. But thanks to the last two browsers on that list, we cannot actually be sure if the selectionchange DOM event is irrelevant just by checking whether we have focus. This pull request instead checks whether the resulting DOM selection includes all or part of the editor element, resulting in a better experience in Firefox and IE. Otherwise, every time you click back into the editor, your caret jumps back to where it was the last time the editor had focus. 👎

[coveralls]

Coverage remained the same at 100.0% when pulling 87d46784c79157d33f2d244b2b5b4f3dfac02f33 on tuespetre:master into ec70448ac3dca8314442e304bd3151fc695e07be on ckeditor:master.

[Reinmar]

Hi! Thanks for the PR! And congratulations for digging this up cause I think it's the first time I've seen a contribution touching such a system :)

I think that this issue sums it up: https://github.com/ckeditor/ckeditor5/issues/734

I've been reading the current implementation of the SelectionObserver and I don't understand why we abort when the editable has no focus. This seems unnecessary. Or rather – imprecise.

I'd try to decouple these two pieces of state (focus and selection) as much as possible. So, it seems that you chose the right direction.

Why do we need a check at all? It's because we listen to selectionchange on the entire document. Which means that selections made outside the editor will trigger the SelectionObserver. We need to filter them out.

I'm not sure, though, if your proposal is correct:

if ( !domSelection.containsNode( domElement, true ) ) {
    return;
}

Demo: https://jsfiddle.net/1948yv6j/5/

Fails when:

I'd rather check whether selection.anchorNode and selection.focusNode are both inside the editable element. So sth like:

domElement.contains( selection.anchorNode ) && domElement.contains( selection.focusNode ) 

It should check fine whether the entire selection is contained within the editor. Demo: https://jsfiddle.net/1948yv6j/8/

I'd check both selection ends because I'm afraid that some browsers may allow making a selection which starts outside and ends inside a contentEditable element. Especially that we need to consider the read-only mode.

WDYT about this?