Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Commit

Permalink
Merge pull request #935 from ckeditor/t/647
Browse files Browse the repository at this point in the history 
Fix: `DomConverter#domToView()` will not throw when converting a comment. Closes #647.
  • Loading branch information
@scofalik
scofalik authored Apr 25, 2017
2 parents 16ae05a + 1d2fb1e commit ffc41d4
Show file tree
Hide file tree
Showing 6 changed files with 57 additions and 34 deletions.
12 changes: 12 additions & 0 deletions src/view/domconverter.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
Expand Down Expand Up @@ -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`.
*
Expand Down
18 changes: 9 additions & 9 deletions tests/view/domconverter/binding.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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' );
Expand All @@ -35,7 +35,7 @@ describe( 'DomConverter', () => {
} );
} );

describe( 'bindDocumentFragments', () => {
describe( 'bindDocumentFragments()', () => {
it( 'should bind document fragments', () => {
const domFragment = document.createDocumentFragment();
const viewFragment = new ViewDocumentFragment();
Expand All @@ -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' );
Expand Down Expand Up @@ -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' );
Expand All @@ -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 );
Expand All @@ -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' );
Expand Down Expand Up @@ -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' );
Expand Down Expand Up @@ -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' );
Expand All @@ -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();
Expand Down
14 changes: 10 additions & 4 deletions tests/view/domconverter/dom-to-view.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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' );
Expand Down Expand Up @@ -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', {}, [
Expand Down Expand Up @@ -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' );
Expand Down Expand Up @@ -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' );
Expand Down Expand Up @@ -570,7 +576,7 @@ describe( 'DomConverter', () => {
} );
} );

describe( 'domSelectionToView', () => {
describe( 'domSelectionToView()', () => {
it( 'should convert selection', () => {
const domFoo = document.createTextNode( 'foo' );
const domBar = document.createTextNode( 'bar' );
Expand Down
27 changes: 22 additions & 5 deletions tests/view/domconverter/domconverter.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ describe( 'DomConverter', () => {
} );
} );

describe( 'focus', () => {
describe( 'focus()', () => {
let viewEditable, domEditable, viewDocument;

beforeEach( () => {
Expand Down Expand Up @@ -69,48 +69,65 @@ 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;
} );

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

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

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;
} );
} );
} );
} );
8 changes: 4 additions & 4 deletions tests/view/domconverter/view-to-dom.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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' );
Expand Down Expand Up @@ -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>' );

Expand Down Expand Up @@ -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 );
Expand Down Expand Up @@ -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 );
Expand Down
12 changes: 0 additions & 12 deletions tests/view/observer/domeventdata.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down

0 comments on commit ffc41d4

Please sign in to comment.