Merge pull request #1073 from ckeditor/t/1072

Fix: AttributeElement with bogus <br /> will now be placed after all UI elements which will fix how those elements are rendered. Closes #1072.

Author: Piotr Jasiun

src/conversion/model-selection-to-view-converters.js

@@ -200,12 +200,36 @@ function wrapCollapsedSelectionPosition( modelSelection, viewSelection, viewElem
 	}
 
 	let viewPosition = viewSelection.getFirstPosition();
+
+	// This hack is supposed to place attribute element *after* all ui elements if the attribute element would be
+	// the only non-ui child and thus receive a block filler.
+	// This is needed to properly render ui elements. Block filler is a <br /> element. If it is placed before
+	// UI element, the ui element will most probably be incorrectly rendered (in next line). #1072.
+	if ( shouldPushAttributeElement( viewPosition.parent ) ) {
+		viewPosition = viewPosition.getLastMatchingPosition( value => value.item.is( 'uiElement' ) );
+	}
+	// End of hack.
+
 	viewPosition = viewWriter.wrapPosition( viewPosition, viewElement );
 
 	viewSelection.removeAllRanges();
 	viewSelection.addRange( new ViewRange( viewPosition, viewPosition ) );
 }
 
+function shouldPushAttributeElement( parent ) {
+	if ( !parent.is( 'element' ) ) {
+		return false;
+	}
+
+	for ( const child of parent.getChildren() ) {
+		if ( !child.is( 'uiElement' ) ) {
+			return false;
+		}
+	}
+
+	return true;
+}
+
 /**
  * Function factory, creates a converter that clears artifacts after the previous
  * {@link module:engine/model/selection~Selection model selection} conversion. It removes all empty

tests/conversion/model-selection-to-view-converters.js

@@ -11,6 +11,7 @@ import ModelPosition from '../../src/model/position';
 import ViewDocument from '../../src/view/document';
 import ViewContainerElement from '../../src/view/containerelement';
 import ViewAttributeElement from '../../src/view/attributeelement';
+import ViewUIElement from '../../src/view/uielement';
 import { mergeAttributes } from '../../src/view/writer';
 
 import Mapper from '../../src/conversion/mapper';
@@ -314,6 +315,69 @@ describe( 'model-selection-to-view-converters', () => {
 					.to.equal( '<div>foo{}bar</div>' );
 			} );
 
+			// #1072 - if the container has only ui elements, collapsed selection attribute should be rendered after those ui elements.
+			it( 'selection with attribute before ui element - no non-ui children', () => {
+				setModelData( modelDoc, '' );
+
+				// Add two ui elements to view.
+				viewRoot.appendChildren( [
+					new ViewUIElement( 'span' ),
+					new ViewUIElement( 'span' )
+				] );
+
+				modelSelection.setRanges( [ new ModelRange( new ModelPosition( modelRoot, [ 0 ] ) ) ] );
+				modelSelection.setAttribute( 'bold', true );
+
+				// Convert model to view.
+				dispatcher.convertSelection( modelSelection, [] );
+
+				// Stringify view and check if it is same as expected.
+				expect( stringifyView( viewRoot, viewSelection, { showType: false } ) )
+					.to.equal( '<div><span></span><span></span><strong>[]</strong></div>' );
+			} );
+
+			// #1072.
+			it( 'selection with attribute before ui element - has non-ui children #1', () => {
+				setModelData( modelDoc, 'x' );
+
+				modelSelection.setRanges( [ new ModelRange( new ModelPosition( modelRoot, [ 1 ] ) ) ] );
+				modelSelection.setAttribute( 'bold', true );
+
+				// Convert model to view.
+				dispatcher.convertInsertion( ModelRange.createIn( modelRoot ) );
+
+				// Add ui element to view.
+				const uiElement = new ViewUIElement( 'span' );
+				viewRoot.insertChildren( 1, uiElement );
+
+				dispatcher.convertSelection( modelSelection, [] );
+
+				// Stringify view and check if it is same as expected.
+				expect( stringifyView( viewRoot, viewSelection, { showType: false } ) )
+					.to.equal( '<div>x<strong>[]</strong><span></span></div>' );
+			} );
+
+			// #1072.
+			it( 'selection with attribute before ui element - has non-ui children #2', () => {
+				setModelData( modelDoc, '<$text bold="true">x</$text>y' );
+
+				modelSelection.setRanges( [ new ModelRange( new ModelPosition( modelRoot, [ 1 ] ) ) ] );
+				modelSelection.setAttribute( 'bold', true );
+
+				// Convert model to view.
+				dispatcher.convertInsertion( ModelRange.createIn( modelRoot ) );
+
+				// Add ui element to view.
+				const uiElement = new ViewUIElement( 'span' );
+				viewRoot.insertChildren( 1, uiElement );
+
+				dispatcher.convertSelection( modelSelection, [] );
+
+				// Stringify view and check if it is same as expected.
+				expect( stringifyView( viewRoot, viewSelection, { showType: false } ) )
+					.to.equal( '<div><strong>x{}</strong><span></span>y</div>' );
+			} );
+
 			it( 'consumes consumable values properly', () => {
 				// Add callbacks that will fire before default ones.
 				// This should prevent default callbacks doing anything.