Browse Source

Do not clear attributes if they are detached. They were most probably already cleared.

Piotr Jasiun 9 years ago
parent
commit
7a19f8daad

+ 6 - 4
packages/ckeditor5-engine/src/conversion/model-selection-to-view-converters.js

@@ -198,10 +198,12 @@ export function convertSelectionAttribute( elementCreator ) {
 export function clearAttributes() {
 	return ( evt, data, consumable, conversionApi ) => {
 		for ( let range of conversionApi.viewSelection.getRanges() ) {
-			conversionApi.writer.mergeAttributes( range.end );
-
-			if ( !range.isCollapsed ) {
-				conversionApi.writer.mergeAttributes( range.start );
+			// Not collapsed selection should not have artifacts.
+			if ( range.isCollapsed ) {
+				// Position might be in the node removed by the Writer.
+				if ( range.end.parent.getDocument() ) {
+					conversionApi.writer.mergeAttributes( range.start );
+				}
 			}
 		}
 		conversionApi.viewSelection.removeAllRanges();

+ 8 - 7
packages/ckeditor5-engine/tests/conversion/model-converter-builder.js

@@ -15,12 +15,11 @@ import ModelText from '/ckeditor5/engine/model/text.js';
 import ModelRange from '/ckeditor5/engine/model/range.js';
 import ModelPosition from '/ckeditor5/engine/model/position.js';
 
+import ViewDocument from '/ckeditor5/engine/view/document.js';
 import ViewElement from '/ckeditor5/engine/view/element.js';
 import ViewContainerElement from '/ckeditor5/engine/view/containerelement.js';
 import ViewAttributeElement from '/ckeditor5/engine/view/attributeelement.js';
 import ViewText from '/ckeditor5/engine/view/text.js';
-import ViewWriter from '/ckeditor5/engine/view/writer.js';
-import ViewSelection from '/ckeditor5/engine/view/selection.js';
 
 import Mapper from '/ckeditor5/engine/conversion/mapper.js';
 import ModelConversionDispatcher from '/ckeditor5/engine/conversion/modelconversiondispatcher.js';
@@ -70,20 +69,22 @@ function viewToString( item ) {
 }
 
 describe( 'Model converter builder', () => {
-	let dispatcher, modelDoc, modelRoot, viewRoot, mapper, writer, viewSelection;
+	let dispatcher, mapper;
+	let modelDoc, modelRoot;
+	let viewDoc, viewRoot, writer, viewSelection;
 
 	beforeEach( () => {
 		modelDoc = new ModelDocument();
 		modelRoot = modelDoc.createRoot( 'root', 'root' );
 
-		viewRoot = new ViewContainerElement( 'div' );
+		viewDoc = new ViewDocument();
+		viewRoot = viewDoc.createRoot( 'div' );
+		writer = viewDoc.writer;
+		viewSelection = viewDoc.selection;
 
 		mapper = new Mapper();
 		mapper.bindElements( modelRoot, viewRoot );
 
-		writer = new ViewWriter();
-		viewSelection = new ViewSelection();
-
 		dispatcher = new ModelConversionDispatcher( { writer, mapper, viewSelection } );
 
 		dispatcher.on( 'insert:$text', insertText() );

+ 35 - 9
packages/ckeditor5-engine/tests/conversion/model-selection-to-view-converters.js

@@ -12,10 +12,9 @@ import ModelElement from '/ckeditor5/engine/model/element.js';
 import ModelRange from '/ckeditor5/engine/model/range.js';
 import ModelPosition from '/ckeditor5/engine/model/position.js';
 
+import ViewDocument from '/ckeditor5/engine/view/document.js';
 import ViewContainerElement from '/ckeditor5/engine/view/containerelement.js';
 import ViewAttributeElement from '/ckeditor5/engine/view/attributeelement.js';
-import ViewWriter from  '/ckeditor5/engine/view/writer.js';
-import ViewSelection from  '/ckeditor5/engine/view/selection.js';
 
 import Mapper from '/ckeditor5/engine/conversion/mapper.js';
 import ModelConversionDispatcher from '/ckeditor5/engine/conversion/modelconversiondispatcher.js';
@@ -35,21 +34,23 @@ import {
 import { stringify as stringifyView } from '/tests/engine/_utils/view.js';
 import { setData as setModelData } from '/tests/engine/_utils/model.js';
 
-let dispatcher, modelDoc, modelRoot, modelSelection, mapper, viewRoot, writer, viewSelection;
+let dispatcher, mapper;
+let modelDoc, modelRoot, modelSelection;
+let viewDoc, viewRoot, writer, viewSelection;
 
 beforeEach( () => {
 	modelDoc = new ModelDocument();
 	modelRoot = modelDoc.createRoot( 'main' );
 	modelSelection = modelDoc.selection;
 
-	viewRoot = new ViewContainerElement( 'viewRoot' );
+	viewDoc = new ViewDocument();
+	viewRoot = viewDoc.createRoot( 'div' );
+	writer = viewDoc.writer;
+	viewSelection = viewDoc.selection;
 
 	mapper = new Mapper();
 	mapper.bindElements( modelRoot, viewRoot );
 
-	writer = new ViewWriter();
-	viewSelection = new ViewSelection();
-
 	dispatcher = new ModelConversionDispatcher( { mapper, writer, viewSelection } );
 
 	dispatcher.on( 'insert:$text', insertText() );
@@ -272,7 +273,32 @@ describe( 'clean-up', () => {
 			expect( viewSelection.rangeCount ).to.equal( 1 );
 
 			const viewString = stringifyView( viewRoot, viewSelection, { showType: false } );
-			expect( viewString ).to.equal( '<viewRoot>f{}oobar</viewRoot>' );
+			expect( viewString ).to.equal( '<div>f{}oobar</div>' );
+		} );
+
+		it( 'should do nothing if the attribute element had been already removed', () => {
+			dispatcher.on( 'selectionAttribute:bold', convertSelectionAttribute( new ViewAttributeElement( 'b' ) ) );
+			dispatcher.on( 'addAttribute:style', wrap( new ViewAttributeElement( 'b' ) ) );
+
+			test(
+				[ 3, 3 ],
+				'foobar',
+				'foo<b>[]</b>bar',
+				{ bold: 'true' }
+			);
+
+			// Remove <b></b> manually.
+			writer.mergeAttributes( viewSelection.getFirstPosition() );
+
+			const modelRange = ModelRange.createFromParentsAndOffsets( modelRoot, 1, modelRoot, 1 );
+			modelDoc.selection.setRanges( [ modelRange ] );
+
+			dispatcher.convertSelection( modelDoc.selection );
+
+			expect( viewSelection.rangeCount ).to.equal( 1 );
+
+			const viewString = stringifyView( viewRoot, viewSelection, { showType: false } );
+			expect( viewString ).to.equal( '<div>f{}oobar</div>' );
 		} );
 	} );
 } );
@@ -468,5 +494,5 @@ function test( selectionPaths, modelInput, expectedView, selectionAttributes = {
 	dispatcher.convertSelection( modelSelection );
 
 	// Stringify view and check if it is same as expected.
-	expect( stringifyView( viewRoot, viewSelection, { showType: false } ) ).to.equal( '<viewRoot>' + expectedView + '</viewRoot>' );
+	expect( stringifyView( viewRoot, viewSelection, { showType: false } ) ).to.equal( '<div>' + expectedView + '</div>' );
 }