Disable inline filling character removal during composition

src/view/renderer.js

@@ -98,13 +98,20 @@ export default class Renderer {
 		this._inlineFiller = null;
 
 		/**
-		 * Indicates if the view document is focused and selection can be rendered. Selection will not be rendered if
+		 * Indicates whether the view document is focused and selection can be rendered. Selection will not be rendered if
 		 * this is set to `false`.
 		 *
 		 * @member {Boolean}
 		 */
 		this.isFocused = false;
 
+		/**
+		 * Indicates whether text composition takes place in the document.
+		 *
+		 * @member {Boolean}
+		 */
+		this.isComposing = false;
+
 		/**
 		 * DOM element containing fake selection.
 		 *
@@ -180,23 +187,43 @@ export default class Renderer {
 	render() {
 		let inlineFillerPosition;
 
-		// 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();
+			if ( !this.isComposing && !this._isSelectionInInlineFiller( false ) ) {
+				// There was inline filler rendered in the DOM but it's not at the selection position any more, so we can remove
+				// it if not during composition (cause even if it's needed, it must be placed in another location).
+				this._removeInlineFiller();
+			} else if ( this.isComposing ) {
+				// When selection has 0 ranges, `isCollapsed` returns false, but here
+				// we are only interested in non-collapsed selection (so with at least 1 range).
+				if ( !this.selection.isCollapsed && this.selection.rangeCount > 0 ) {
+					// During composition selection may be extended and its start/end moved to a different text
+					// node (using 'shift + up' in Chrome on Windows during composition). In such situations
+					// filler should not be moved or deleted (because it is possible to continue composing).
+					inlineFillerPosition = this._getExistingInlineFillerPosition();
+				} else if ( !this._isValidCompositionSelection() ) {
+					// Check for situations like:
+					//
+					// 		<p>text{}</p><p>FILLERtext</p> or <p>{}</p><p>FILLERtext</p>
+					//
+					// when collapsed selection was moved to a different node (without inline filler) during composition.
+					// It means `compositionend` event was not fired properly and inline filler should be removed.
+					this._removeInlineFiller();
+				}
+			}
 		}
-		// 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 );
+		if ( !inlineFillerPosition ) {
+			// 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 );
+			}
 		}
 
 		for ( const element of this.markedAttributes ) {
@@ -212,27 +239,33 @@ export default class Renderer {
 				this._updateText( node, { inlineFillerPosition } );
 			}
 		}
-
 		// 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.
+		this._inlineFiller = null; // Reset filler first.
 		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 = this._addInlineFiller( domDocument, fillerDomPosition.parent, fillerDomPosition.offset );
-			} else {
-				// Filler has been found, save it.
-				this._inlineFiller = fillerDomPosition.parent;
+			// `fillerDomPosition` may be null during composition in situations like:
+			//
+			//		<p>Foo Bar<b>FILLER{}</b></p> -> 'Shift' + 'Arrow up' -> <p>{Foo Bar}</p>
+			//
+			// because `inlineFillerPosition` points to `AttributeElement` (`strong`) which was removed during rendering.
+			// It means inline filler is already removed from DOM so `this._inlineFiller` should be null.
+			if ( fillerDomPosition ) {
+				const domDocument = fillerDomPosition.parent.ownerDocument;
+
+				if ( !startsWithFiller( fillerDomPosition.parent ) ) {
+					// Filler has not been created at filler position. Create it now.
+					this._inlineFiller = this._addInlineFiller( domDocument, fillerDomPosition.parent, fillerDomPosition.offset );
+				} else {
+					// Filler has been found, save it.
+					this._inlineFiller = fillerDomPosition.parent;
+				}
 			}
-		} else {
-			// There is no filler needed.
-			this._inlineFiller = null;
 		}
 
 		this._updateSelection();
@@ -280,7 +313,7 @@ export default class Renderer {
 	 * Gets the position of the inline filler based on the current selection.
 	 * Here, we assume that we know that the filler is needed and
 	 * {@link #_isSelectionInInlineFiller is at the selection position}, and, since it's needed,
-	 * it's somewhere at the selection postion.
+	 * it's somewhere at the selection position.
 	 *
 	 * Note: we cannot restore the filler position based on the filler's DOM text node, because
 	 * when this method is called (before rendering) the bindings will often be broken. View to DOM
@@ -299,15 +332,37 @@ export default class Renderer {
 		}
 	}
 
+	/**
+	 * Gets the position of the inline filler based on the filler's DOM text node.
+	 *
+	 * Note: Here, we assume that {@link #_inlineFiller inline filler} exists. In most cases
+	 * {@link #_getInlineFillerPosition} should be used to get inline filler position which
+	 * will be valid after rendering. This method is used only in special cases (for example
+	 * when we know only selection changed so bindings will not get outdated).
+	 *
+	 * @private
+	 * @returns {module:engine/view/position~Position}
+	 */
+	_getExistingInlineFillerPosition() {
+		let fillerPosition = this.domConverter.domPositionToView( this._inlineFiller, 0 );
+		if ( fillerPosition.parent.is( 'text' ) ) {
+			const textNode = fillerPosition.parent;
+			fillerPosition = new ViewPosition( textNode.parent, textNode.index );
+		}
+		return fillerPosition;
+	}
+
 	/**
 	 * Returns `true` if the selection hasn't left the inline filler's text node.
 	 * If it is `true` it means that the filler had been added for a reason and the selection does not
 	 * left the filler's text node. E.g. the user can be in the middle of a composition so it should not be touched.
 	 *
 	 * @private
+	 * @param {Boolean} [withText=true] Whether to check if selection is inside text node containing inline filler only
+	 * or additional text is also allowed.
 	 * @returns {Boolean} True if the inline filler and selection are in the same place.
 	 */
-	_isSelectionInInlineFiller() {
+	_isSelectionInInlineFiller( withText = true ) {
 		if ( this.selection.rangeCount != 1 || !this.selection.isCollapsed ) {
 			return false;
 		}
@@ -324,8 +379,44 @@ export default class Renderer {
 		const selectionPosition = this.selection.getFirstPosition();
 		const position = this.domConverter.viewPositionToDom( selectionPosition );
 
-		if ( position && isText( position.parent ) && startsWithFiller( position.parent ) ) {
+		if ( position && isText( position.parent ) ) {
+			return withText ? startsWithFiller( position.parent ) : isInlineFiller( position.parent );
+		}
+
+		return false;
+	}
+
+	/**
+	 * Checks if the selection is in the same node in which current composition
+	 * was started based on inline filler position.
+	 *
+	 * Note: we assume that inline filler exists and composition takes place.
+	 *
+	 * @private
+	 * @returns {Boolean}
+	 */
+	_isValidCompositionSelection() {
+		if ( this.selection.rangeCount != 1 ) {
+			return false;
+		}
+
+		const position = this.domConverter.viewPositionToDom( this.selection.getFirstPosition() );
+
+		if ( !position ) {
+			// Position is not mapped properly when new inline element is inserted:
+			//
+			//		<p>foo<b>{}</b></p> -> <p>foo<b><i>{}</i></b></p>
+			//
+			// or new character in the empty inline element is inserted:
+			//
+			//		<p>foo<b>{}</b></p> -> <p>foo<b>a{}</b></p>
+			//
+			// so we assume it is valid position during composition.
 			return true;
+		} else if ( position && isText( position.parent ) ) {
+			// If during composition there is an inline filler present, but in another
+			// node it means the composition was started in that node and the selection is in different one.
+			return this._isSelectionInInlineFiller();
 		}
 
 		return false;

src/view/view.js

@@ -85,6 +85,7 @@ export default class View {
 		 */
 		this._renderer = new Renderer( this.domConverter, this.document.selection );
 		this._renderer.bind( 'isFocused' ).to( this.document );
+		this._renderer.bind( 'isComposing' ).to( this.document );
 
 		/**
 		 * Roots of the DOM tree. Map on the `HTMLElement`s with roots names as keys.

tests/view/manual/inline-filler-removal.html

@@ -0,0 +1,20 @@
+<button id="composition-toggle" style="margin-bottom: 25px;">Toggle composition</button>
+
+<div id="editor">
+	<h2>Heading 1</h2>
+	<p>Paragraph</p>
+	<p></p>
+	<p><strong>Bold</strong> <i>Italic</i> <a href="https://ckeditor.com">Link</a></p>
+	<ul>
+		<li>UL List item 1</li>
+		<li>UL List item 2</li>
+	</ul>
+	<blockquote>
+	<p>Quote</p>
+	<ul>
+		<li>Quoted UL List item 1</li>
+		<li>Quoted UL List item 2</li>
+	</ul>
+	<p>Quote</p>
+	</blockquote>
+</div>

tests/view/manual/inline-filler-removal.js

@@ -0,0 +1,46 @@
+/**
+ * @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+/* globals console, window, document */
+
+import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor';
+
+import ArticlePluginSet from '@ckeditor/ckeditor5-core/tests/_utils/articlepluginset';
+
+ClassicEditor
+	.create( document.querySelector( '#editor' ), {
+		plugins: [ ArticlePluginSet ],
+		toolbar: [ 'bold', 'italic', 'link', 'bulletedList', 'blockQuote', 'undo', 'redo' ]
+	} )
+	.then( editor => {
+		window.editor = editor;
+
+		let isComposing = true;
+
+		// Prevent stealing the focus.
+		document.querySelector( '#composition-toggle' ).addEventListener( 'mousedown', evt => {
+			evt.preventDefault();
+		} );
+
+		document.querySelector( '#composition-toggle' ).addEventListener( 'click', evt => {
+			editor.editing.view._renderer.isComposing = isComposing;
+			console.log( `Composition is ${ isComposing ? 'on' : 'off' } (toggle button).` );
+			isComposing = !isComposing;
+
+			// Prevent stealing the focus.
+			evt.preventDefault();
+		} );
+
+		editor.editing.view.document.on( 'compositionstart', () => {
+			console.log( 'Composition is on (native event).' );
+		} );
+
+		editor.editing.view.document.on( 'compositionend', () => {
+			console.log( 'Composition is off (native event).' );
+		} );
+	} )
+	.catch( err => {
+		console.error( err.stack );
+	} );

tests/view/manual/inline-filler-removal.md

@@ -0,0 +1,13 @@
+# Inline filler removal
+
+Check whether inline filler is properly removed when typing inside empty inline elements:
+
+* By default inline filler should be removed when anything is typed inside empty inline element.
+* During composition, inline filler should not be removed until composition ends.
+
+You may emulate composition by clicking **Toggle composition** or use e.g. Spanish or Japanese keyboard
+(the composition state is logged in the browser dev console).
+
+**Notice:** While emulating composition (or if you are able to create situation in which `compositionend` is not fired properly),
+inline filler should behave as described above in the inline elements, but should not be inserted in other non-empty elements.
+Also there should by always maximum one inline filler in the whole content.