Ver código fonte

Merge pull request #156 from ckeditor/t/ckeditor5/1083

Fix: Bogus `<br />` element inserted by a browser at the end of an element is now correctly handled. Closes ckeditor/ckeditor5#1083.
Piotrek Koszuliński 7 anos atrás
pai
commit
577c36ffe7

+ 20 - 9
packages/ckeditor5-typing/src/input.js

@@ -211,15 +211,26 @@ class MutationHandler {
 		const modelFromDomChildren = Array.from( modelFromCurrentDom.getChildren() );
 		const currentModelChildren = Array.from( currentModel.getChildren() );
 
-		// Skip situations when common ancestor has any elements (cause they are too hard).
-		if ( !hasOnlyTextNodes( modelFromDomChildren ) || !hasOnlyTextNodes( currentModelChildren ) ) {
+		// Remove the last `<softBreak>` from the end of `modelFromDomChildren` if there is no `<softBreak>` in current model.
+		// If the described scenario happened, it means that this is a bogus `<br />` added by a browser.
+		const lastDomChild = modelFromDomChildren[ modelFromDomChildren.length - 1 ];
+		const lastCurrentChild = currentModelChildren[ currentModelChildren.length - 1 ];
+
+		if ( lastDomChild && lastDomChild.is( 'softBreak' ) && lastCurrentChild && !lastCurrentChild.is( 'softBreak' ) ) {
+			modelFromDomChildren.pop();
+		}
+
+		// Skip situations when common ancestor has any container elements.
+		if ( !isSafeForTextMutation( modelFromDomChildren ) || !isSafeForTextMutation( currentModelChildren ) ) {
 			return;
 		}
 
-		// Replace &nbsp; inserted by the browser with normal space.
-		// See comment in `_handleTextMutation`.
-		const newText = modelFromDomChildren.map( item => item.data ).join( '' ).replace( /\u00A0/g, ' ' );
-		const oldText = currentModelChildren.map( item => item.data ).join( '' );
+		// Replace &nbsp; inserted by the browser with normal space. See comment in `_handleTextMutation`.
+		// Replace non-texts with any character. This is potentially dangerous but passes in manual tests. The thing is
+		// that we need to take care of proper indexes so we cannot simply remove non-text elements from the content.
+		// By inserting a character we keep all the real texts on their indexes.
+		const newText = modelFromDomChildren.map( item => item.is( 'text' ) ? item.data : '@' ).join( '' ).replace( /\u00A0/g, ' ' );
+		const oldText = currentModelChildren.map( item => item.is( 'text' ) ? item.data : '@' ).join( '' );
 
 		// Do nothing if mutations created same text.
 		if ( oldText === newText ) {
@@ -447,12 +458,12 @@ function containerChildrenMutated( mutations ) {
 	return false;
 }
 
-// Returns true if provided array contains only {@link module:engine/model/text~Text model text nodes}.
+// Returns true if provided array contains content that won't be problematic during diffing and text mutation handling.
 //
 // @param {Array.<module:engine/model/node~Node>} children
 // @returns {Boolean}
-function hasOnlyTextNodes( children ) {
-	return children.every( child => child.is( 'text' ) );
+function isSafeForTextMutation( children ) {
+	return children.every( child => child.is( 'text' ) || child.is( 'softBreak' ) );
 }
 
 // Calculates first change index and number of characters that should be inserted and deleted starting from that index.

+ 66 - 1
packages/ckeditor5-typing/tests/input.js

@@ -8,6 +8,7 @@ import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
 import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
 import List from '@ckeditor/ckeditor5-list/src/list';
 import Bold from '@ckeditor/ckeditor5-basic-styles/src/bold';
+import ShiftEnter from '@ckeditor/ckeditor5-enter/src/shiftenter';
 import Input from '../src/input';
 
 import Writer from '@ckeditor/ckeditor5-engine/src/model/writer';
@@ -36,7 +37,7 @@ describe( 'Input feature', () => {
 		const domElement = document.createElement( 'div' );
 		document.body.appendChild( domElement );
 
-		return ClassicTestEditor.create( domElement, { plugins: [ Input, Paragraph, Bold, List ] } )
+		return ClassicTestEditor.create( domElement, { plugins: [ Input, Paragraph, Bold, List, ShiftEnter ] } )
 			.then( newEditor => {
 				// Mock image feature.
 				newEditor.model.schema.register( 'image', { allowWhere: '$text' } );
@@ -465,6 +466,70 @@ describe( 'Input feature', () => {
 
 			expect( getViewData( view ) ).to.equal( '<p>{}Foo</p><ul><li>Bar</li><li>Baz</li></ul>' );
 		} );
+
+		it( 'should handle bogus br correctly', () => {
+			editor.setData( '<p><strong>Foo</strong></p>' );
+
+			editor.model.change( writer => {
+				writer.setSelection( editor.model.document.getRoot().getChild( 0 ), 'end' );
+				writer.removeSelectionAttribute( 'bold' );
+			} );
+
+			// We need to change the DOM content manually because typing algorithm actually does not check
+			// `newChildren` and `oldChildren` list but takes them from DOM and model.
+			const p = viewRoot.getChild( 0 );
+			const domP = editor.editing.view.domConverter.mapViewToDom( p );
+			domP.appendChild( document.createTextNode( ' ' ) );
+			domP.appendChild( document.createElement( 'br' ) );
+
+			viewDocument.fire( 'mutations', [
+				{
+					type: 'children',
+					oldChildren: [ viewRoot.getChild( 0 ).getChild( 0 ) ],
+					newChildren: [
+						new ViewElement( 'strong', null, new ViewText( 'Foo' ) ),
+						new ViewText( ' ' ),
+						new ViewElement( 'br' )
+					],
+					node: viewRoot.getChild( 0 )
+				}
+			] );
+
+			expect( getViewData( view ) ).to.equal( '<p><strong>Foo</strong> {}</p>' );
+		} );
+
+		it( 'should handle children mutation correctly if there are soft breaks in the mutated container', () => {
+			editor.setData( '<p><strong>Foo</strong><br /><strong>Bar</strong></p>' );
+
+			editor.model.change( writer => {
+				writer.setSelection( editor.model.document.getRoot().getChild( 0 ), 'end' );
+				writer.removeSelectionAttribute( 'bold' );
+			} );
+
+			// We need to change the DOM content manually because typing algorithm actually does not check
+			// `newChildren` and `oldChildren` list but takes them from DOM and model.
+			const p = viewRoot.getChild( 0 );
+			const domP = editor.editing.view.domConverter.mapViewToDom( p );
+			domP.appendChild( document.createTextNode( ' ' ) );
+			domP.appendChild( document.createElement( 'br' ) );
+
+			viewDocument.fire( 'mutations', [
+				{
+					type: 'children',
+					oldChildren: [ ...viewRoot.getChild( 0 ).getChildren() ],
+					newChildren: [
+						new ViewElement( 'strong', null, new ViewText( 'Foo' ) ),
+						new ViewElement( 'br' ),
+						new ViewElement( 'strong', null, new ViewText( 'Bar' ) ),
+						new ViewText( ' ' ),
+						new ViewElement( 'br' )
+					],
+					node: viewRoot.getChild( 0 )
+				}
+			] );
+
+			expect( getViewData( view ) ).to.equal( '<p><strong>Foo</strong><br></br><strong>Bar</strong> {}</p>' );
+		} );
 	} );
 
 	describe( 'keystroke handling', () => {