Merge pull request #10558 from ckeditor/ck/10430-selection-blocking-r…

…esearch

Fix (engine): Blocked DOM selection updates in `Renderer` when the selection is made using the mouse. Limited random glitching in Chrome when the user starts selection in a link (or a marker) at the beginning of the block. Closes #10562.

packages/ckeditor5-engine/package.json

@@ -30,6 +30,7 @@
     "@ckeditor/ckeditor5-basic-styles": "^30.0.0",
     "@ckeditor/ckeditor5-block-quote": "^30.0.0",
     "@ckeditor/ckeditor5-clipboard": "^30.0.0",
+    "@ckeditor/ckeditor5-cloud-services": "^30.0.0",
     "@ckeditor/ckeditor5-core": "^30.0.0",
     "@ckeditor/ckeditor5-editor-classic": "^30.0.0",
     "@ckeditor/ckeditor5-enter": "^30.0.0",
@@ -38,6 +39,7 @@
     "@ckeditor/ckeditor5-image": "^30.0.0",
     "@ckeditor/ckeditor5-link": "^30.0.0",
     "@ckeditor/ckeditor5-list": "^30.0.0",
+    "@ckeditor/ckeditor5-mention": "^30.0.0",
     "@ckeditor/ckeditor5-paragraph": "^30.0.0",
     "@ckeditor/ckeditor5-table": "^30.0.0",
     "@ckeditor/ckeditor5-theme-lark": "^30.0.0",

packages/ckeditor5-engine/src/view/document.js

@@ -80,6 +80,18 @@ export default class Document {
 		 */
 		this.set( 'isFocused', false );
 
+		/**
+		 * `true` while the user is making a selection in the document (e.g. holding the mouse button and moving the cursor).
+		 * When they stop selecting, the property goes back to `false`.
+		 *
+		 * This property is updated by the {@link module:engine/view/observer/selectionobserver~SelectionObserver}.
+		 *
+		 * @readonly
+		 * @observable
+		 * @member {Boolean} module:engine/view/document~Document#isSelecting
+		 */
+		this.set( 'isSelecting', false );
+
 		/**
 		 * True if composition is in progress inside the document.
 		 *

packages/ckeditor5-engine/src/view/domconverter.js

@@ -648,10 +648,10 @@ export default class DomConverter {
 	 * If structures are too different and it is not possible to find corresponding position then `null` will be returned.
 	 *
 	 * @param {Node} domParent DOM position parent.
-	 * @param {Number} domOffset DOM position offset.
+	 * @param {Number} [domOffset=0] DOM position offset. You can skip it when converting the inline filler node.
 	 * @returns {module:engine/view/position~Position} viewPosition View position.
 	 */
-	domPositionToView( domParent, domOffset ) {
+	domPositionToView( domParent, domOffset = 0 ) {
 		if ( this.isBlockFiller( domParent ) ) {
 			return this.domPositionToView( domParent.parentNode, indexOf( domParent ) );
 		}

packages/ckeditor5-engine/src/view/observer/selectionobserver.js

@@ -20,6 +20,8 @@ import { debounce } from 'lodash-es';
  * {@link module:engine/view/document~Document#event:selectionChange} event only if a selection change was the only change in the document
  * and the DOM selection is different then the view selection.
  *
+ * This observer also manages the {@link module:engine/view/document~Document#isSelecting} property of the view document.
+ *
  * Note that this observer is attached by the {@link module:engine/view/view~View} and is available by default.
  *
  * @see module:engine/view/observer/mutationobserver~MutationObserver
@@ -78,8 +80,26 @@ export default class SelectionObserver extends Observer {
 		 */
 		this._fireSelectionChangeDoneDebounced = debounce( data => this.document.fire( 'selectionChangeDone', data ), 200 );
 
+		/**
+		 * When called, starts clearing the {@link #_loopbackCounter} counter in intervals of time. When the number of selection
+		 * changes exceeds a certain limit within the interval of time, the observer will not fire `selectionChange` but warn about
+		 * possible infinite selection loop.
+		 *
+		 * @private
+		 * @member {Number} #_clearInfiniteLoopInterval
+		 */
 		this._clearInfiniteLoopInterval = setInterval( () => this._clearInfiniteLoop(), 1000 );
 
+		/**
+		 * Unlocks the `isSelecting` state of the view document in case the selection observer did not record this fact
+		 * correctly (for whatever the reason). It is a safeguard (paranoid check) that returns document to the normal state
+		 * after a certain period of time (debounced, postponed by each selectionchange event).
+		 *
+		 * @private
+		 * @method #_documentIsSelectingInactivityTimeoutDebounced
+		 */
+		this._documentIsSelectingInactivityTimeoutDebounced = debounce( () => ( this.document.isSelecting = false ), 5000 );
+
 		/**
 		 * Private property to check if the code does not enter infinite loop.
 		 *
@@ -100,10 +120,36 @@ export default class SelectionObserver extends Observer {
 			return;
 		}
 
+		const startDocumentIsSelecting = () => {
+			this.document.isSelecting = true;
+
+			// Let's activate the safety timeout each time the document enters the "is selecting" state.
+			this._documentIsSelectingInactivityTimeoutDebounced();
+		};
+
+		const endDocumentIsSelecting = () => {
+			this.document.isSelecting = false;
+
+			// The safety timeout can be canceled when the document leaves the "is selecting" state.
+			this._documentIsSelectingInactivityTimeoutDebounced.cancel();
+		};
+
 		this.listenTo( domDocument, 'selectionchange', ( evt, domEvent ) => {
 			this._handleSelectionChange( domEvent, domDocument );
+
+			// Defer the safety timeout when the selection changes (e.g. the user keeps extending the selection
+			// using their mouse).
+			this._documentIsSelectingInactivityTimeoutDebounced();
 		} );
 
+		// The document has the "is selecting" state while the user keeps making (extending) the selection
+		// (e.g. by holding the mouse button and moving the cursor). The state resets when they either released
+		// the mouse button or interrupted the process by pressing or releasing any key.
+		this.listenTo( domDocument, 'selectstart', startDocumentIsSelecting, { priority: 'highest' } );
+		this.listenTo( domDocument, 'mouseup', endDocumentIsSelecting, { priority: 'highest' } );
+		this.listenTo( domDocument, 'keydown', endDocumentIsSelecting, { priority: 'highest' } );
+		this.listenTo( domDocument, 'keyup', endDocumentIsSelecting, { priority: 'highest' } );
+
 		this._documents.add( domDocument );
 	}
 
@@ -115,6 +161,7 @@ export default class SelectionObserver extends Observer {
 
 		clearInterval( this._clearInfiniteLoopInterval );
 		this._fireSelectionChangeDoneDebounced.cancel();
+		this._documentIsSelectingInactivityTimeoutDebounced.cancel();
 	}
 
 	/**

packages/ckeditor5-engine/src/view/renderer.js

@@ -100,8 +100,32 @@ export default class Renderer {
 		 * this is set to `false`.
 		 *
 		 * @member {Boolean}
+		 * @observable
 		 */
-		this.isFocused = false;
+		this.set( 'isFocused', false );
+
+		/**
+		 * Indicates whether the user is making a selection in the document (e.g. holding the mouse button and moving the cursor).
+		 * When they stop selecting, the property goes back to `false`.
+		 *
+		 * Note: In some browsers, the renderer will stop rendering the selection and inline fillers while the user is making
+		 * a selection to avoid glitches in DOM selection (https://github.com/ckeditor/ckeditor5/issues/10562).
+		 *
+		 * @member {Boolean}
+		 * @observable
+		 */
+		this.set( 'isSelecting', false );
+
+		// Rendering the selection and inline filler manipulation should be postponed in Blink until the user finishes creating
+		// the selection in DOM to avoid accidental selection collapsing (https://github.com/ckeditor/ckeditor5/issues/10562).
+		// When the user stops, selecting, all pending changes should be rendered ASAP, though.
+		if ( env.isBlink ) {
+			this.on( 'change:isSelecting', () => {
+				if ( !this.isSelecting ) {
+					this.render();
+				}
+			} );
+		}
 
 		/**
 		 * The text node in which the inline filler was rendered.
@@ -172,29 +196,40 @@ export default class Renderer {
 	 */
 	render() {
 		let inlineFillerPosition;
+		const isInlineFillerRenderingPossible = env.isBlink ? !this.isSelecting : true;
 
 		// Refresh mappings.
 		for ( const element of this.markedChildren ) {
 			this._updateChildrenMappings( element );
 		}
 
-		// There was inline filler rendered in the DOM but it's not
-		// at the selection position any more, so we can remove it
-		// (cause even if it's needed, it must be placed in another location).
-		if ( this._inlineFiller && !this._isSelectionInInlineFiller() ) {
-			this._removeInlineFiller();
-		}
+		// Don't manipulate inline fillers while the selection is being made in Blink to prevent accidental
+		// DOM selection collapsing (https://github.com/ckeditor/ckeditor5/issues/10562).
+		if ( isInlineFillerRenderingPossible ) {
+			// There was inline filler rendered in the DOM but it's not
+			// at the selection position any more, so we can remove it
+			// (cause even if it's needed, it must be placed in another location).
+			if ( this._inlineFiller && !this._isSelectionInInlineFiller() ) {
+				this._removeInlineFiller();
+			}
 
-		// If we've got the filler, let's try to guess its position in the view.
-		if ( this._inlineFiller ) {
-			inlineFillerPosition = this._getInlineFillerPosition();
-		}
-		// Otherwise, if it's needed, create it at the selection position.
-		else if ( this._needsInlineFillerAtSelection() ) {
-			inlineFillerPosition = this.selection.getFirstPosition();
+			// If we've got the filler, let's try to guess its position in the view.
+			if ( this._inlineFiller ) {
+				inlineFillerPosition = this._getInlineFillerPosition();
+			}
+			// Otherwise, if it's needed, create it at the selection position.
+			else if ( this._needsInlineFillerAtSelection() ) {
+				inlineFillerPosition = this.selection.getFirstPosition();
 
-			// Do not use `markToSync` so it will be added even if the parent is already added.
-			this.markedChildren.add( inlineFillerPosition.parent );
+				// Do not use `markToSync` so it will be added even if the parent is already added.
+				this.markedChildren.add( inlineFillerPosition.parent );
+			}
+		}
+		// Paranoid check: we make sure the inline filler has any parent so it can be mapped to view position
+		// by DomConverter.
+		else if ( this._inlineFiller && this._inlineFiller.parentNode ) {
+			// While the user is making selection, preserve the inline filler at its original position.
+			inlineFillerPosition = this.domConverter.domPositionToView( this._inlineFiller );
 		}
 
 		for ( const element of this.markedAttributes ) {
@@ -211,26 +246,29 @@ export default class Renderer {
 			}
 		}
 
-		// Check whether the inline filler is required and where it really is in the DOM.
-		// At this point in most cases it will be in the DOM, but there are exceptions.
-		// For example, if the inline filler was deep in the created DOM structure, it will not be created.
-		// Similarly, if it was removed at the beginning of this function and then neither text nor children were updated,
-		// it will not be present.
-		// Fix those and similar scenarios.
-		if ( inlineFillerPosition ) {
-			const fillerDomPosition = this.domConverter.viewPositionToDom( inlineFillerPosition );
-			const domDocument = fillerDomPosition.parent.ownerDocument;
-
-			if ( !startsWithFiller( fillerDomPosition.parent ) ) {
-				// Filler has not been created at filler position. Create it now.
-				this._inlineFiller = addInlineFiller( domDocument, fillerDomPosition.parent, fillerDomPosition.offset );
+		// * Check whether the inline filler is required and where it really is in the DOM.
+		//   At this point in most cases it will be in the DOM, but there are exceptions.
+		//   For example, if the inline filler was deep in the created DOM structure, it will not be created.
+		//   Similarly, if it was removed at the beginning of this function and then neither text nor children were updated,
+		//   it will not be present. Fix those and similar scenarios.
+		// * Don't manipulate inline fillers while the selection is being made in Blink to prevent accidental
+		//   DOM selection collapsing (https://github.com/ckeditor/ckeditor5/issues/10562).
+		if ( isInlineFillerRenderingPossible ) {
+			if ( inlineFillerPosition ) {
+				const fillerDomPosition = this.domConverter.viewPositionToDom( inlineFillerPosition );
+				const domDocument = fillerDomPosition.parent.ownerDocument;
+
+				if ( !startsWithFiller( fillerDomPosition.parent ) ) {
+					// Filler has not been created at filler position. Create it now.
+					this._inlineFiller = addInlineFiller( domDocument, fillerDomPosition.parent, fillerDomPosition.offset );
+				} else {
+					// Filler has been found, save it.
+					this._inlineFiller = fillerDomPosition.parent;
+				}
 			} else {
-				// Filler has been found, save it.
-				this._inlineFiller = fillerDomPosition.parent;
+				// There is no filler needed.
+				this._inlineFiller = null;
 			}
-		} else {
-			// There is no filler needed.
-			this._inlineFiller = null;
 		}
 
 		// First focus the new editing host, then update the selection.
@@ -403,7 +441,7 @@ export default class Renderer {
 		}
 
 		if ( isInlineFiller( domFillerNode ) ) {
-			domFillerNode.parentNode.removeChild( domFillerNode );
+			domFillerNode.remove();
 		} else {
 			domFillerNode.data = domFillerNode.data.substr( INLINE_FILLER_LENGTH );
 		}
@@ -557,7 +595,7 @@ export default class Renderer {
 		const inlineFillerPosition = options.inlineFillerPosition;
 		const actualDomChildren = this.domConverter.mapViewToDom( viewElement ).childNodes;
 		const expectedDomChildren = Array.from(
-			this.domConverter.viewChildrenToDom( viewElement, domElement.ownerDocument, { bind: true, inlineFillerPosition } )
+			this.domConverter.viewChildrenToDom( viewElement, domElement.ownerDocument, { bind: true } )
 		);
 
 		// Inline filler element has to be created as it is present in the DOM, but not in the view. It is required
@@ -698,6 +736,14 @@ export default class Renderer {
 	 * @private
 	 */
 	_updateSelection() {
+		// Block updating DOM selection in Blink while the user is selecting to prevent accidental selection collapsing.
+		// Note: Structural changes in DOM must trigger selection rendering, though. Nodes the selection was anchored
+		// to may disappear in DOM which would break the selection (e.g. in real-time collaboration scenarios).
+		// https://github.com/ckeditor/ckeditor5/issues/10562
+		if ( env.isBlink && this.isSelecting && !this.markedChildren.size ) {
+			return;
+		}
+
 		// If there is no selection - remove DOM and fake selections.
 		if ( this.selection.rangeCount === 0 ) {
 			this._removeDomSelection();

packages/ckeditor5-engine/src/view/view.js

@@ -116,7 +116,7 @@ export default class View {
 		 * @type {module:engine/view/renderer~Renderer}
 		 */
 		this._renderer = new Renderer( this.domConverter, this.document.selection );
-		this._renderer.bind( 'isFocused' ).to( this.document );
+		this._renderer.bind( 'isFocused', 'isSelecting' ).to( this.document );
 
 		/**
 		 * A DOM root attributes cache. It saves the initial values of DOM root attributes before the DOM element