Browse Source

Merge pull request #464 from ckeditor/t/463

t/463: Bugs related to typing
Szymon Cofalik 9 years ago
parent
commit
048427504f

+ 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();

+ 2 - 3
packages/ckeditor5-engine/src/view/attributeelement.js

@@ -6,7 +6,6 @@
 'use strict';
 
 import Element from './element.js';
-import ContainerElement from './containerelement.js';
 
 // Default attribute priority.
 const DEFAULT_PRIORITY = 10;
@@ -83,7 +82,7 @@ export default class AttributeElement extends Element {
 		let element = this.parent;
 
 		// <p><b></b></p> needs filler -> <p><b><br></b></p>
-		while ( !( element instanceof ContainerElement ) ) {
+		while ( element instanceof AttributeElement ) {
 			if ( element.getChildCount() > 1 ) {
 				return null;
 			}
@@ -91,7 +90,7 @@ export default class AttributeElement extends Element {
 			element = element.parent;
 		}
 
-		if ( element.getChildCount() > 1 ) {
+		if ( !element || element.getChildCount() > 1 ) {
 			return null;
 		}
 

+ 5 - 4
packages/ckeditor5-engine/src/view/filler.js

@@ -77,17 +77,18 @@ for ( let i = 0; i < INLINE_FILLER_LENGTH; i++ ) {
 }
 
 /**
- * Checks if the text node starts with the {@link engine.view.filler.INLINE_FILLER inline filler}.
+ * Checks if the node is a text node which starts with the {@link engine.view.filler.INLINE_FILLER inline filler}.
  *
  *		startsWithFiller( document.createTextNode( INLINE_FILLER ) ); // true
  *		startsWithFiller( document.createTextNode( INLINE_FILLER + 'foo' ) ); // true
  *		startsWithFiller( document.createTextNode( 'foo' ) ); // false
+ *		startsWithFiller( document.createElement( 'p' ) ); // false
  *
- * @param {Text} domText DOM text node.
+ * @param {Node} domNode DOM node.
  * @returns {Boolean} True if the text node starts with the {@link engine.view.filler.INLINE_FILLER inline filler}.
  */
-export function startsWithFiller( domText ) {
-	return ( domText.data.substr( 0, INLINE_FILLER_LENGTH ) === INLINE_FILLER );
+export function startsWithFiller( domNode ) {
+	return ( domNode instanceof Text ) && ( domNode.data.substr( 0, INLINE_FILLER_LENGTH ) === INLINE_FILLER );
 }
 
 /**

+ 2 - 1
packages/ckeditor5-engine/src/view/node.js

@@ -87,7 +87,8 @@ export default class Node {
 	 * @returns {engine.view.Document|null} View Document of the node or null.
 	 */
 	getDocument() {
-		if ( this.parent ) {
+		// Parent might be Node, null or DocumentFragment.
+		if ( this.parent instanceof Node ) {
 			return this.parent.getDocument();
 		} else {
 			return null;

+ 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>' );
 }

+ 7 - 0
packages/ckeditor5-engine/tests/view/attributeelement.js

@@ -99,5 +99,12 @@ describe( 'AttributeElement', () => {
 
 			expect( attribute.getFillerOffset() ).to.be.null;
 		} );
+
+		it( 'should return null if there is no parent container element', () => {
+			const { selection } = parse( '<attribute:b><attribute:i>[]</attribute:i>foo</attribute:b>' );
+			const attribute = selection.getFirstPosition().parent;
+
+			expect( attribute.getFillerOffset() ).to.be.null;
+		} );
 	} );
 } );

+ 8 - 2
packages/ckeditor5-engine/tests/view/filler.js

@@ -26,18 +26,24 @@ describe( 'filler', () => {
 	} );
 
 	describe( 'startsWithFiller', () => {
-		it( 'should be true for element which contains only filler', () => {
+		it( 'should be true for node which contains only filler', () => {
 			const node = document.createTextNode( INLINE_FILLER );
 
 			expect( startsWithFiller( node ) ).to.be.true;
 		} );
 
-		it( 'should be true for element which starts with filler', () => {
+		it( 'should be true for node which starts with filler', () => {
 			const node = document.createTextNode( INLINE_FILLER + 'foo' );
 
 			expect( startsWithFiller( node ) ).to.be.true;
 		} );
 
+		it( 'should be false for element', () => {
+			const node = document.createElement( 'p' );
+
+			expect( startsWithFiller( node ) ).to.be.false;
+		} );
+
 		it( 'should be false which contains filler in the middle', () => {
 			const node = document.createTextNode( 'x' + INLINE_FILLER + 'x' );
 

+ 8 - 0
packages/ckeditor5-engine/tests/view/node.js

@@ -9,6 +9,7 @@
 
 import Element from '/ckeditor5/engine/view/element.js';
 import Text from '/ckeditor5/engine/view/text.js';
+import DocumentFragment from '/ckeditor5/engine/view/documentfragment.js';
 import RootEditableElement from '/ckeditor5/engine/view/rooteditableelement.js';
 import CKEditorError from '/ckeditor5/utils/ckeditorerror.js';
 
@@ -146,6 +147,13 @@ describe( 'Node', () => {
 			expect( parent.getDocument() ).to.equal( docMock );
 			expect( child.getDocument() ).to.equal( docMock );
 		} );
+
+		it( 'should return null if element is inside DocumentFragment', () => {
+			const child = new Element( 'p' );
+			new DocumentFragment( [ child ] );
+
+			expect( child.getDocument() ).to.be.null;
+		} );
 	} );
 
 	describe( 'getRoot', () => {