Merge pull request #1204 from ckeditor/t/ckeditor5/692

Fix: [Firefox] Added fix for typing space on the edge of inline elements. Closes ckeditor/ckeditor5#692.

src/view/domconverter.js

@@ -7,7 +7,7 @@
  * @module engine/view/domconverter
  */
 
-/* globals document, Node, NodeFilter */
+/* globals document, Node, NodeFilter, Text */
 
 import ViewText from './text';
 import ViewElement from './element';
@@ -956,10 +956,10 @@ export default class DomConverter {
 	 * @private
 	 */
 	_processDataFromDomText( node ) {
-		let data = getDataWithoutFiller( node );
+		let data = node.data;
 
 		if ( _hasDomParentOfType( node, this.preElements ) ) {
-			return data;
+			return getDataWithoutFiller( node );
 		}
 
 		// Change all consecutive whitespace characters (from the [ \n\t\r] set –
@@ -978,9 +978,16 @@ export default class DomConverter {
 		}
 
 		// If next text node does not exist remove space character from the end of this text node.
-		if ( !nextNode ) {
+		if ( !nextNode && !startsWithFiller( node ) ) {
 			data = data.replace( / $/, '' );
 		}
+
+		// At the beginning and end of a block element, Firefox inserts normal space + <br> instead of non-breaking space.
+		// This means that the text node starts/end with normal space instead of non-breaking space.
+		// This causes a problem because the normal space would be removed in `.replace` calls above. To prevent that,
+		// the inline filler is removed only after the data is initially processed (by the `.replace` above). See ckeditor5#692.
+		data = getDataWithoutFiller( new Text( data ) );
+
 		// At this point we should have removed all whitespaces from DOM text data.
 
 		// Now we have to change &nbsp; chars, that were in DOM text data because of rendering reasons, to spaces.

tests/tickets/ckeditor5-692.js

@@ -0,0 +1,87 @@
+/**
+ * @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+import MutationObserver from '../../src/view/observer/mutationobserver';
+import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor.js';
+import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
+import { getData as getModelData, setData as setModelData } from '../../src/dev-utils/model';
+import Bold from '@ckeditor/ckeditor5-basic-styles/src/bold.js';
+import { getData as getViewData } from '../../src/dev-utils/view';
+import { isInlineFiller } from '../../src/view/filler';
+import Input from '@ckeditor/ckeditor5-typing/src/input';
+
+/* globals document */
+
+describe( 'Bug ckeditor5#692', () => {
+	let editorElement, editor, mutationObserver, view, domEditor;
+
+	beforeEach( () => {
+		editorElement = document.createElement( 'div' );
+		document.body.appendChild( editorElement );
+
+		return ClassicTestEditor
+			.create( editorElement, {
+				plugins: [ Paragraph, Bold, Input ]
+			} )
+			.then( newEditor => {
+				editor = newEditor;
+				view = editor.editing.view;
+				mutationObserver = view.getObserver( MutationObserver );
+				domEditor = editor.ui.view.editableElement;
+			} );
+	} );
+
+	afterEach( () => {
+		document.body.removeChild( editorElement );
+
+		return editor.destroy();
+	} );
+
+	describe( 'DomConverter', () => {
+		// https://github.com/ckeditor/ckeditor5/issues/692 Scenario 1.
+		it( 'should handle space after inline filler at the end of container', () => {
+			setModelData( editor.model, '<paragraph>foo[]</paragraph>' );
+
+			// Create Bold attribute at the end of paragraph.
+			editor.execute( 'bold' );
+
+			expect( getModelData( editor.model ) ).to.equal( '<paragraph>foo<$text bold="true">[]</$text></paragraph>' );
+
+			const domParagraph = domEditor.childNodes[ 0 ];
+			const textNode = domParagraph.childNodes[ 1 ].childNodes[ 0 ];
+
+			expect( isInlineFiller( textNode ) ).to.be.true;
+
+			// Add space inside the strong's text node.
+			textNode.data += ' ';
+			mutationObserver.flush();
+
+			expect( getModelData( editor.model ) ).to.equal( '<paragraph>foo<$text bold="true"> []</$text></paragraph>' );
+			expect( getViewData( editor.editing.view ) ).to.equal( '<p>foo<strong> {}</strong></p>' );
+		} );
+
+		// https://github.com/ckeditor/ckeditor5/issues/692 Scenario 2.
+		it( 'should handle space after inline filler at the end of container', () => {
+			setModelData( editor.model, '<paragraph>[]foo</paragraph>' );
+
+			// Create Bold attribute at the end of paragraph.
+			editor.execute( 'bold' );
+
+			expect( getModelData( editor.model ) ).to.equal( '<paragraph><$text bold="true">[]</$text>foo</paragraph>' );
+
+			const domParagraph = domEditor.childNodes[ 0 ];
+			const textNode = domParagraph.childNodes[ 0 ].childNodes[ 0 ];
+
+			expect( isInlineFiller( textNode ) ).to.be.true;
+
+			// Add space inside the strong's text node.
+			textNode.data += ' ';
+			mutationObserver.flush();
+
+			expect( getModelData( editor.model ) ).to.equal( '<paragraph><$text bold="true"> []</$text>foo</paragraph>' );
+			expect( getViewData( editor.editing.view ) ).to.equal( '<p><strong> {}</strong>foo</p>' );
+		} );
+	} );
+} );

tests/view/domconverter/dom-to-view.js

@@ -525,6 +525,30 @@ describe( 'DomConverter', () => {
 			// See also whitespace-handling-integration.js.
 			//
 		} );
+
+		describe( 'clearing auto filler', () => {
+			it( 'should remove inline filler when converting dom to view', () => {
+				const text = document.createTextNode( INLINE_FILLER + 'foo' );
+				const view = converter.domToView( text );
+
+				expect( view.data ).to.equal( 'foo' );
+			} );
+
+			// See https://github.com/ckeditor/ckeditor5/issues/692.
+			it( 'should not remove space after inline filler if previous node nor next node does not exist', () => {
+				const text = document.createTextNode( INLINE_FILLER + ' ' );
+				const view = converter.domToView( text );
+
+				expect( view.data ).to.equal( ' ' );
+			} );
+
+			it( 'should convert non breaking space to normal space after inline filler', () => {
+				const text = document.createTextNode( INLINE_FILLER + '\u00A0' );
+				const view = converter.domToView( text );
+
+				expect( view.data ).to.equal( ' ' );
+			} );
+		} );
 	} );
 
 	describe( 'domChildrenToView', () => {

tests/view/observer/mutationobserver.js

@@ -269,6 +269,112 @@ describe( 'MutationObserver', () => {
 		expect( lastMutations[ 0 ].newText ).to.equal( 'xy' );
 	} );
 
+	// https://github.com/ckeditor/ckeditor5/issues/692 Scenario 1.
+	it( 'should handle space after inline filler at the end of container', () => {
+		const { view: viewContainer, selection } = parse(
+			'<container:p>' +
+				'foo' +
+				'<attribute:b>[]</attribute:b>' +
+			'</container:p>'
+		);
+
+		view.change( writer => {
+			viewRoot.appendChildren( viewContainer );
+			writer.setSelection( selection );
+		} );
+
+		// Appended container is third in the tree.
+		const container = domEditor.childNodes[ 2 ];
+		const inlineFiller = container.childNodes[ 1 ].childNodes[ 0 ];
+
+		inlineFiller.data += ' ';
+
+		mutationObserver.flush();
+
+		expect( lastMutations.length ).to.equal( 1 );
+		expect( lastMutations[ 0 ].type ).to.equal( 'children' );
+		expect( lastMutations[ 0 ].oldChildren.length ).to.equal( 0 );
+		expect( lastMutations[ 0 ].newChildren.length ).to.equal( 1 );
+		expect( lastMutations[ 0 ].newChildren[ 0 ].is( 'text' ) ).to.be.true;
+		expect( lastMutations[ 0 ].newChildren[ 0 ].data ).to.equal( ' ' );
+		expect( lastMutations[ 0 ].node ).to.equal( selection.getFirstPosition().parent );
+	} );
+
+	// https://github.com/ckeditor/ckeditor5/issues/692 Scenario 3.
+	it( 'should handle space after inline filler at the end of container #2', () => {
+		const { view: viewContainer, selection } = parse(
+			'<container:p>' +
+				'foo' +
+				'<attribute:b>bar</attribute:b>' +
+				'[]' +
+			'</container:p>'
+		);
+
+		view.change( writer => {
+			viewRoot.appendChildren( viewContainer );
+			writer.setSelection( selection );
+		} );
+
+		// Appended container is third in the tree.
+		const container = domEditor.childNodes[ 2 ];
+		const inlineFiller = container.childNodes[ 2 ];
+
+		inlineFiller.data += ' ';
+
+		mutationObserver.flush();
+
+		expect( lastMutations.length ).to.equal( 1 );
+		expect( lastMutations[ 0 ].type ).to.equal( 'children' );
+		expect( lastMutations[ 0 ].oldChildren.length ).to.equal( 2 );
+		expect( lastMutations[ 0 ].newChildren.length ).to.equal( 3 );
+
+		// Foo and attribute is removed and reinserted.
+		expect( lastMutations[ 0 ].oldChildren[ 0 ].is( 'text' ) ).to.be.true;
+		expect( lastMutations[ 0 ].oldChildren[ 0 ].data ).to.equal( 'foo' );
+		expect( lastMutations[ 0 ].newChildren[ 0 ].is( 'text' ) ).to.be.true;
+		expect( lastMutations[ 0 ].newChildren[ 0 ].data ).to.equal( 'foo' );
+
+		expect( lastMutations[ 0 ].oldChildren[ 1 ].is( 'attributeElement' ) ).to.be.true;
+		expect( lastMutations[ 0 ].oldChildren[ 1 ].name ).to.equal( 'b' );
+		expect( lastMutations[ 0 ].newChildren[ 1 ].is( 'attributeElement' ) ).to.be.true;
+		expect( lastMutations[ 0 ].newChildren[ 1 ].name ).to.equal( 'b' );
+
+		expect( lastMutations[ 0 ].newChildren[ 2 ].is( 'text' ) ).to.be.true;
+		expect( lastMutations[ 0 ].newChildren[ 2 ].data ).to.equal( ' ' );
+		expect( lastMutations[ 0 ].node ).to.equal( selection.getFirstPosition().parent );
+	} );
+
+	// https://github.com/ckeditor/ckeditor5/issues/692 Scenario 2.
+	it( 'should handle space after inline filler at the beginning of container', () => {
+		const { view: viewContainer, selection } = parse(
+			'<container:p>' +
+				'<attribute:b>[]</attribute:b>' +
+				'foo' +
+			'</container:p>'
+		);
+
+		view.change( writer => {
+			viewRoot.appendChildren( viewContainer );
+			writer.setSelection( selection );
+		} );
+
+		// Appended container is third in the tree.
+		const container = domEditor.childNodes[ 2 ];
+		const inlineFiller = container.childNodes[ 0 ].childNodes[ 0 ];
+
+		inlineFiller.data += ' ';
+
+		mutationObserver.flush();
+
+		expect( lastMutations.length ).to.equal( 1 );
+		expect( lastMutations[ 0 ].type ).to.equal( 'children' );
+		expect( lastMutations[ 0 ].oldChildren.length ).to.equal( 0 );
+		expect( lastMutations[ 0 ].newChildren.length ).to.equal( 1 );
+		expect( lastMutations[ 0 ].newChildren[ 0 ].is( 'text' ) ).to.be.true;
+		expect( lastMutations[ 0 ].newChildren[ 0 ].data ).to.equal( ' ' );
+		expect( lastMutations[ 0 ].node ).to.equal( selection.getFirstPosition().parent );
+	} );
+
 	it( 'should have no block filler in mutation', () => {
 		viewRoot.appendChildren( parse( '<container:p></container:p>' ) );
 
@@ -359,7 +465,7 @@ describe( 'MutationObserver', () => {
 
 		mutationObserver.flush();
 
-		// There was onlu P2 change. P1 must be ignored.
+		// There was only P2 change. P1 must be ignored.
 		const viewP2 = viewRoot.getChild( 1 );
 		expect( lastMutations.length ).to.equal( 1 );
 		expect( lastMutations[ 0 ].node ).to.equal( viewP2 );