Ver código fonte

Merge pull request #935 from ckeditor/t/647

Fix: `DomConverter#domToView()` will not throw when converting a comment. Closes #647.
Szymon Cofalik 8 anos atrás
pai
commit
00a455b20b

+ 12 - 0
packages/ckeditor5-engine/src/view/domconverter.js

@@ -360,6 +360,8 @@ export default class DomConverter {
 
 				return textData === '' ? null : new ViewText( textData );
 			}
+		} else if ( this.isComment( domNode ) ) {
+			return null;
 		} else {
 			if ( this.getCorrespondingView( domNode ) ) {
 				return this.getCorrespondingView( domNode );
@@ -766,6 +768,16 @@ export default class DomConverter {
 		return node && node.nodeType == Node.DOCUMENT_FRAGMENT_NODE;
 	}
 
+	/**
+	 * Returns `true` when `node.nodeType` equals `Node.COMMENT_NODE`.
+	 *
+	 * @param {Node} node Node to check.
+	 * @returns {Boolean}
+	 */
+	isComment( node ) {
+		return node && node.nodeType == Node.COMMENT_NODE;
+	}
+
 	/**
 	 * Returns `true` if given selection is a backward selection, that is, if it's `focus` is before `anchor`.
 	 *

+ 9 - 9
packages/ckeditor5-engine/tests/view/domconverter/binding.js

@@ -23,7 +23,7 @@ describe( 'DomConverter', () => {
 		converter = new DomConverter();
 	} );
 
-	describe( 'bindElements', () => {
+	describe( 'bindElements()', () => {
 		it( 'should bind elements', () => {
 			const domElement = document.createElement( 'p' );
 			const viewElement = new ViewElement( 'p' );
@@ -35,7 +35,7 @@ describe( 'DomConverter', () => {
 		} );
 	} );
 
-	describe( 'bindDocumentFragments', () => {
+	describe( 'bindDocumentFragments()', () => {
 		it( 'should bind document fragments', () => {
 			const domFragment = document.createDocumentFragment();
 			const viewFragment = new ViewDocumentFragment();
@@ -47,7 +47,7 @@ describe( 'DomConverter', () => {
 		} );
 	} );
 
-	describe( 'getCorrespondingView', () => {
+	describe( 'getCorrespondingView()', () => {
 		it( 'should return corresponding view element if element is passed', () => {
 			const domElement = document.createElement( 'p' );
 			const viewElement = new ViewElement( 'p' );
@@ -86,7 +86,7 @@ describe( 'DomConverter', () => {
 		} );
 	} );
 
-	describe( 'getCorrespondingViewElement', () => {
+	describe( 'getCorrespondingViewElement()', () => {
 		it( 'should return corresponding view element', () => {
 			const domElement = document.createElement( 'p' );
 			const viewElement = new ViewElement( 'p' );
@@ -97,7 +97,7 @@ describe( 'DomConverter', () => {
 		} );
 	} );
 
-	describe( 'getCorrespondingViewDocumentFragment', () => {
+	describe( 'getCorrespondingViewDocumentFragment()', () => {
 		it( 'should return corresponding view document fragment', () => {
 			const domFragment = document.createDocumentFragment();
 			const viewFragment = converter.domToView( domFragment );
@@ -108,7 +108,7 @@ describe( 'DomConverter', () => {
 		} );
 	} );
 
-	describe( 'getCorrespondingViewText', () => {
+	describe( 'getCorrespondingViewText()', () => {
 		it( 'should return corresponding view text based on sibling', () => {
 			const domImg = document.createElement( 'img' );
 			const domText = document.createTextNode( 'foo' );
@@ -207,7 +207,7 @@ describe( 'DomConverter', () => {
 		} );
 	} );
 
-	describe( 'getCorrespondingDom', () => {
+	describe( 'getCorrespondingDom()', () => {
 		it( 'should return corresponding DOM element if element was passed', () => {
 			const domElement = document.createElement( 'p' );
 			const viewElement = new ViewElement( 'p' );
@@ -245,7 +245,7 @@ describe( 'DomConverter', () => {
 		} );
 	} );
 
-	describe( 'getCorrespondingDomElement', () => {
+	describe( 'getCorrespondingDomElement()', () => {
 		it( 'should return corresponding DOM element', () => {
 			const domElement = document.createElement( 'p' );
 			const viewElement = new ViewElement( 'p' );
@@ -256,7 +256,7 @@ describe( 'DomConverter', () => {
 		} );
 	} );
 
-	describe( 'getCorrespondingDomDocumentFragment', () => {
+	describe( 'getCorrespondingDomDocumentFragment()', () => {
 		it( 'should return corresponding DOM document fragment', () => {
 			const domFragment = document.createDocumentFragment();
 			const viewFragment = new ViewDocumentFragment();

+ 10 - 4
packages/ckeditor5-engine/tests/view/domconverter/dom-to-view.js

@@ -24,7 +24,7 @@ describe( 'DomConverter', () => {
 		converter = new DomConverter();
 	} );
 
-	describe( 'domToView', () => {
+	describe( 'domToView()', () => {
 		it( 'should create tree of view elements from DOM elements', () => {
 			const domImg = createElement( document, 'img' );
 			const domText = document.createTextNode( 'foo' );
@@ -169,6 +169,12 @@ describe( 'DomConverter', () => {
 			expect( converter.domToView( textNode ) ).to.be.null;
 		} );
 
+		it( 'should return null for a comment', () => {
+			const comment = document.createComment( 'abc' );
+
+			expect( converter.domToView( comment ) ).to.be.null;
+		} );
+
 		describe( 'it should clear whitespaces', () => {
 			it( 'at the beginning of block element', () => {
 				const domDiv = createElement( document, 'div', {}, [
@@ -371,7 +377,7 @@ describe( 'DomConverter', () => {
 		} );
 	} );
 
-	describe( 'domPositionToView', () => {
+	describe( 'domPositionToView()', () => {
 		it( 'should converter position in text', () => {
 			const domText = document.createTextNode( 'foo' );
 			const domB = createElement( document, 'b', null, 'bar' );
@@ -533,7 +539,7 @@ describe( 'DomConverter', () => {
 		} );
 	} );
 
-	describe( 'domRangeToView', () => {
+	describe( 'domRangeToView()', () => {
 		it( 'should converter DOM range', () => {
 			const domFoo = document.createTextNode( 'foo' );
 			const domBar = document.createTextNode( 'bar' );
@@ -570,7 +576,7 @@ describe( 'DomConverter', () => {
 		} );
 	} );
 
-	describe( 'domSelectionToView', () => {
+	describe( 'domSelectionToView()', () => {
 		it( 'should convert selection', () => {
 			const domFoo = document.createTextNode( 'foo' );
 			const domBar = document.createTextNode( 'bar' );

+ 22 - 5
packages/ckeditor5-engine/tests/view/domconverter/domconverter.js

@@ -31,7 +31,7 @@ describe( 'DomConverter', () => {
 		} );
 	} );
 
-	describe( 'focus', () => {
+	describe( 'focus()', () => {
 		let viewEditable, domEditable, viewDocument;
 
 		beforeEach( () => {
@@ -69,15 +69,16 @@ describe( 'DomConverter', () => {
 	} );
 
 	describe( 'DOM nodes type checking', () => {
-		let text, element, documentFragment;
+		let text, element, documentFragment, comment;
 
 		before( () => {
 			text = document.createTextNode( 'test' );
 			element = document.createElement( 'div' );
 			documentFragment = document.createDocumentFragment();
+			comment = document.createComment( 'a' );
 		} );
 
-		describe( 'isText', () => {
+		describe( 'isText()', () => {
 			it( 'should return true for Text nodes', () => {
 				expect( converter.isText( text ) ).to.be.true;
 			} );
@@ -85,11 +86,12 @@ describe( 'DomConverter', () => {
 			it( 'should return false for other arguments', () => {
 				expect( converter.isText( element ) ).to.be.false;
 				expect( converter.isText( documentFragment ) ).to.be.false;
+				expect( converter.isText( comment ) ).to.be.false;
 				expect( converter.isText( {} ) ).to.be.false;
 			} );
 		} );
 
-		describe( 'isElement', () => {
+		describe( 'isElement()', () => {
 			it( 'should return true for HTMLElement nodes', () => {
 				expect( converter.isElement( element ) ).to.be.true;
 			} );
@@ -97,11 +99,12 @@ describe( 'DomConverter', () => {
 			it( 'should return false for other arguments', () => {
 				expect( converter.isElement( text ) ).to.be.false;
 				expect( converter.isElement( documentFragment ) ).to.be.false;
+				expect( converter.isElement( comment ) ).to.be.false;
 				expect( converter.isElement( {} ) ).to.be.false;
 			} );
 		} );
 
-		describe( 'isDocumentFragment', () => {
+		describe( 'isDocumentFragment()', () => {
 			it( 'should return true for HTMLElement nodes', () => {
 				expect( converter.isDocumentFragment( documentFragment ) ).to.be.true;
 			} );
@@ -109,8 +112,22 @@ describe( 'DomConverter', () => {
 			it( 'should return false for other arguments', () => {
 				expect( converter.isDocumentFragment( text ) ).to.be.false;
 				expect( converter.isDocumentFragment( element ) ).to.be.false;
+				expect( converter.isDocumentFragment( comment ) ).to.be.false;
 				expect( converter.isDocumentFragment( {} ) ).to.be.false;
 			} );
 		} );
+
+		describe( 'isComment()', () => {
+			it( 'should return true for HTML comments', () => {
+				expect( converter.isComment( comment ) ).to.be.true;
+			} );
+
+			it( 'should return false for other arguments', () => {
+				expect( converter.isComment( text ) ).to.be.false;
+				expect( converter.isComment( element ) ).to.be.false;
+				expect( converter.isComment( documentFragment ) ).to.be.false;
+				expect( converter.isComment( {} ) ).to.be.false;
+			} );
+		} );
 	} );
 } );

+ 4 - 4
packages/ckeditor5-engine/tests/view/domconverter/view-to-dom.js

@@ -25,7 +25,7 @@ describe( 'DomConverter', () => {
 		converter = new DomConverter();
 	} );
 
-	describe( 'viewToDom', () => {
+	describe( 'viewToDom()', () => {
 		it( 'should create tree of DOM elements from view elements', () => {
 			const viewImg = new ViewElement( 'img' );
 			const viewText = new ViewText( 'foo' );
@@ -354,7 +354,7 @@ describe( 'DomConverter', () => {
 		} );
 	} );
 
-	describe( 'viewChildrenToDom', () => {
+	describe( 'viewChildrenToDom()', () => {
 		it( 'should convert children', () => {
 			const viewP = parse( '<container:p>foo<attribute:b>bar</attribute:b></container:p>' );
 
@@ -398,7 +398,7 @@ describe( 'DomConverter', () => {
 		} );
 	} );
 
-	describe( 'viewPositionToDom', () => {
+	describe( 'viewPositionToDom()', () => {
 		it( 'should convert the position in the text', () => {
 			const domFoo = document.createTextNode( 'foo' );
 			const domP = createElement( document, 'p', null, domFoo );
@@ -538,7 +538,7 @@ describe( 'DomConverter', () => {
 		} );
 	} );
 
-	describe( 'viewRangeToDom', () => {
+	describe( 'viewRangeToDom()', () => {
 		it( 'should convert view range to DOM range', () => {
 			const domFoo = document.createTextNode( 'foo' );
 			const domP = createElement( document, 'p', null, domFoo );

+ 0 - 12
packages/ckeditor5-engine/tests/view/observer/domeventdata.js

@@ -11,18 +11,6 @@ import ViewDocument from '../../../src/view/document';
 describe( 'DomEventData', () => {
 	let viewDocument, viewBody, domRoot;
 
-	// Todo: the whole `before` hook can be removed.
-	// Depends on: https://github.com/ckeditor/ckeditor5-engine/issues/647
-	before( () => {
-		// Use Array.from because of MS Edge (#923).
-		for ( const node of Array.from( document.body.childNodes ) ) {
-			// Remove all <!-- Comments -->
-			if ( node.nodeType === 8 ) {
-				document.body.removeChild( node );
-			}
-		}
-	} );
-
 	beforeEach( () => {
 		viewDocument = new ViewDocument();