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.

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.

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', () => {