From c15a2b705ad2ef0312914e6858576fc5df8d8d75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 6 Aug 2019 19:57:04 +0200 Subject: [PATCH 01/25] Better detection of block filler in data. --- src/view/domconverter.js | 6 +- .../whitespace-handling-integration.js | 62 ++++++++++++++++--- 2 files changed, 57 insertions(+), 11 deletions(-) diff --git a/src/view/domconverter.js b/src/view/domconverter.js index 66505be86..c76ab5709 100644 --- a/src/view/domconverter.js +++ b/src/view/domconverter.js @@ -370,7 +370,7 @@ export default class DomConverter { * or `null` if DOM node is a {@link module:engine/view/filler filler} or the given node is an empty text node. */ domToView( domNode, options = {} ) { - if ( isBlockFiller( domNode, this.blockFiller ) ) { + if ( isBlockFiller( domNode, this.blockFiller ) && isFooBar( domNode, this.blockElements ) ) { return null; } @@ -1196,3 +1196,7 @@ function forEachDomNodeAncestor( node, callback ) { node = node.parentNode; } } + +function isFooBar( domNode, blockElements ) { + return !_hasDomParentOfType( domNode, blockElements ) || ( !domNode.parentNode || domNode.parentNode.childNodes.length === 1 ); +} diff --git a/tests/view/domconverter/whitespace-handling-integration.js b/tests/view/domconverter/whitespace-handling-integration.js index 13eea812e..bd80df033 100644 --- a/tests/view/domconverter/whitespace-handling-integration.js +++ b/tests/view/domconverter/whitespace-handling-integration.js @@ -23,6 +23,15 @@ describe( 'DomConverter – whitespace handling – integration', () => { .create( { plugins: [ Paragraph ] } ) .then( newEditor => { editor = newEditor; + + editor.model.schema.extend( '$text', { allowAttributes: [ 'bold' ] } ); + editor.conversion.attributeToElement( { model: 'bold', view: 'b' } ); + + editor.model.schema.register( 'blockQuote', { + allowWhere: '$block', + allowContentOf: '$root' + } ); + editor.conversion.elementToElement( { model: 'blockQuote', view: 'blockquote' } ); } ); } ); @@ -48,14 +57,13 @@ describe( 'DomConverter – whitespace handling – integration', () => { expect( editor.getData() ).to.equal( '

foo

' ); } ); - // Controversial result. See https://github.com/ckeditor/ckeditor5-engine/issues/987. it( 'nbsp at the end of the content is not ignored', () => { - editor.setData( '

foo

' ); + editor.setData( '

foo 

' ); expect( getData( editor.model, { withoutSelection: true } ) ) - .to.equal( 'foo' ); + .to.equal( 'foo ' ); - expect( editor.getData() ).to.equal( '

foo

' ); + expect( editor.getData() ).to.equal( '

foo 

' ); } ); it( 'new line at the beginning of the content is ignored', () => { @@ -76,14 +84,13 @@ describe( 'DomConverter – whitespace handling – integration', () => { expect( editor.getData() ).to.equal( '

foo

' ); } ); - // Controversial result. See https://github.com/ckeditor/ckeditor5-engine/issues/987. it( 'nbsp at the beginning of the content is not ignored', () => { - editor.setData( '

foo

' ); + editor.setData( '

 foo

' ); expect( getData( editor.model, { withoutSelection: true } ) ) - .to.equal( 'foo' ); + .to.equal( ' foo' ); - expect( editor.getData() ).to.equal( '

foo

' ); + expect( editor.getData() ).to.equal( '

 foo

' ); } ); it( 'new line between blocks is ignored', () => { @@ -104,8 +111,7 @@ describe( 'DomConverter – whitespace handling – integration', () => { expect( editor.getData() ).to.equal( '

foo

bar

' ); } ); - // Controversial result. See https://github.com/ckeditor/ckeditor5-engine/issues/987. - it( 'nbsp between blocks is not ignored', () => { + it( 'nbsp between blocks is ignored #1', () => { editor.setData( '

foo

 

bar

' ); expect( getData( editor.model, { withoutSelection: true } ) ) @@ -114,6 +120,24 @@ describe( 'DomConverter – whitespace handling – integration', () => { expect( editor.getData() ).to.equal( '

foo

bar

' ); } ); + it( 'nbsp between blocks is ignored #2', () => { + editor.setData( 'foo
 

bar

' ); + + expect( getData( editor.model, { withoutSelection: true } ) ) + .to.equal( 'foobar' ); + + expect( editor.getData() ).to.equal( '

foo

bar

' ); + } ); + + it( 'nbsp between blocks is ignored #2', () => { + editor.setData( '
foo
 
bar
' ); + + expect( getData( editor.model, { withoutSelection: true } ) ) + .to.equal( '
foo
bar
' ); + + expect( editor.getData() ).to.equal( '

foo

bar

' ); + } ); + it( 'new lines inside blocks are ignored', () => { editor.setData( '

\nfoo\n

' ); @@ -141,6 +165,15 @@ describe( 'DomConverter – whitespace handling – integration', () => { expect( editor.getData() ).to.equal( '

 foo 

' ); } ); + it( 'single nbsp inside blocks are ignored', () => { + editor.setData( '

 

' ); + + expect( getData( editor.model, { withoutSelection: true } ) ) + .to.equal( '' ); + + expect( editor.getData() ).to.equal( '' ); // trimmed + } ); + it( 'all whitespaces together are ignored', () => { editor.setData( '\n

foo\n\r\n \t

\n

bar

' ); @@ -149,6 +182,15 @@ describe( 'DomConverter – whitespace handling – integration', () => { expect( editor.getData() ).to.equal( '

foo

bar

' ); } ); + + it( 'nbsp between inline elements inside blocks are not ignored', () => { + editor.setData( '

foo bar

' ); + + expect( getData( editor.model, { withoutSelection: true } ) ) + .to.equal( '<$text bold="true">foo\u00A0<$text bold="true">bar' ); + + expect( editor.getData() ).to.equal( '

foo bar

' ); + } ); } ); // https://github.com/ckeditor/ckeditor5/issues/1024 From 63e81a5676525a73ac95917209346a8045db0447 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 7 Aug 2019 12:11:45 +0200 Subject: [PATCH 02/25] Introduce isNegligibleBlockFiller helper function. --- src/view/domconverter.js | 42 +++++++++++++++++-- .../whitespace-handling-integration.js | 11 ++++- 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/src/view/domconverter.js b/src/view/domconverter.js index c76ab5709..8dff4706b 100644 --- a/src/view/domconverter.js +++ b/src/view/domconverter.js @@ -16,7 +16,7 @@ import ViewRange from './range'; import ViewSelection from './selection'; import ViewDocumentFragment from './documentfragment'; import ViewTreeWalker from './treewalker'; -import { BR_FILLER, INLINE_FILLER_LENGTH, isBlockFiller, isInlineFiller, startsWithFiller, getDataWithoutFiller } from './filler'; +import { BR_FILLER, getDataWithoutFiller, INLINE_FILLER_LENGTH, isBlockFiller, isInlineFiller, startsWithFiller } from './filler'; import global from '@ckeditor/ckeditor5-utils/src/dom/global'; import indexOf from '@ckeditor/ckeditor5-utils/src/dom/indexof'; @@ -80,6 +80,14 @@ export default class DomConverter { */ this.blockElements = [ 'p', 'div', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6' ]; + /** + * Tag names of DOM `Element`s which are considered inline elements. + * + * @readonly + * @member {Array.} module:engine/view/domconverter~DomConverter#inlineElements + */ + this.inlineElements = [ 'span', 'strong', 'i', 'b', 'a' ]; + /** * DOM to View mapping. * @@ -370,7 +378,7 @@ export default class DomConverter { * or `null` if DOM node is a {@link module:engine/view/filler filler} or the given node is an empty text node. */ domToView( domNode, options = {} ) { - if ( isBlockFiller( domNode, this.blockFiller ) && isFooBar( domNode, this.blockElements ) ) { + if ( isNegligibleBlockFiller( domNode, this.blockFiller, this.inlineElements ) ) { return null; } @@ -1197,6 +1205,32 @@ function forEachDomNodeAncestor( node, callback ) { } } -function isFooBar( domNode, blockElements ) { - return !_hasDomParentOfType( domNode, blockElements ) || ( !domNode.parentNode || domNode.parentNode.childNodes.length === 1 ); +// Checks if given node is negligible and should be removed. +// The negligible block fillers are: +// -   between block nodes +// - single   in block (ie inside

); +// +// The relevant block fillers are: +// -   between inline elements +// -   inside inline elements +function isNegligibleBlockFiller( domNode, blockFiller, inlineElements ) { + if ( !isBlockFiller( domNode, blockFiller ) ) { + return false; + } + + const isSingleDomNode = !domNode.parentNode || domNode.parentNode.childNodes.length === 1; + + if ( isSingleDomNode ) { + // Single DOM nodes are negligible - unless they are in inline element. + return !isInlineElement( domNode.parentNode, inlineElements ); + } + + const prevIsInline = isInlineElement( domNode.previousSibling, inlineElements ); + const nextIsInline = isInlineElement( domNode.nextSibling, inlineElements ); + + return !( prevIsInline || nextIsInline ); +} + +function isInlineElement( domNode, types ) { + return domNode && types.includes( domNode.tagName.toLowerCase() ); } diff --git a/tests/view/domconverter/whitespace-handling-integration.js b/tests/view/domconverter/whitespace-handling-integration.js index bd80df033..b258e1cbf 100644 --- a/tests/view/domconverter/whitespace-handling-integration.js +++ b/tests/view/domconverter/whitespace-handling-integration.js @@ -129,7 +129,7 @@ describe( 'DomConverter – whitespace handling – integration', () => { expect( editor.getData() ).to.equal( '

foo

bar

' ); } ); - it( 'nbsp between blocks is ignored #2', () => { + it( 'nbsp between blocks is ignored #3', () => { editor.setData( '
foo
 
bar
' ); expect( getData( editor.model, { withoutSelection: true } ) ) @@ -191,6 +191,15 @@ describe( 'DomConverter – whitespace handling – integration', () => { expect( editor.getData() ).to.equal( '

foo bar

' ); } ); + + it( 'nbsp inside inline element is not ignored', () => { + editor.setData( '

Foo 

' ); + + expect( getData( editor.model, { withoutSelection: true } ) ) + .to.equal( 'Foo<$text bold="true"> ' ); + + expect( editor.getData() ).to.equal( '

Foo 

' ); + } ); } ); // https://github.com/ckeditor/ckeditor5/issues/1024 From fd2ae413dbfda2eb99c09913b89cc344bac6b983 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 7 Aug 2019 12:23:33 +0200 Subject: [PATCH 03/25] Update docs of isNegligibleBlockFiller() helper function. --- src/view/domconverter.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/view/domconverter.js b/src/view/domconverter.js index 8dff4706b..9cdcbbe14 100644 --- a/src/view/domconverter.js +++ b/src/view/domconverter.js @@ -378,7 +378,7 @@ export default class DomConverter { * or `null` if DOM node is a {@link module:engine/view/filler filler} or the given node is an empty text node. */ domToView( domNode, options = {} ) { - if ( isNegligibleBlockFiller( domNode, this.blockFiller, this.inlineElements ) ) { + if ( isNegligibleBlockFiller( domNode, this.blockFiller, this.inlineElements, this.blockElements ) ) { return null; } @@ -1208,7 +1208,8 @@ function forEachDomNodeAncestor( node, callback ) { // Checks if given node is negligible and should be removed. // The negligible block fillers are: // -   between block nodes -// - single   in block (ie inside

); +// - single   in blocks +// -   between blocks // // The relevant block fillers are: // -   between inline elements From fd76bf2b0285f92905355d679dbe446e7c892597 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 8 Aug 2019 13:53:46 +0200 Subject: [PATCH 04/25] Change the way how isBlockFiller check nbps fillers. --- src/view/domconverter.js | 43 +--------- src/view/filler.js | 81 ++++++++++++++++++- .../whitespace-handling-integration.js | 19 +++-- 3 files changed, 96 insertions(+), 47 deletions(-) diff --git a/src/view/domconverter.js b/src/view/domconverter.js index 9cdcbbe14..66505be86 100644 --- a/src/view/domconverter.js +++ b/src/view/domconverter.js @@ -16,7 +16,7 @@ import ViewRange from './range'; import ViewSelection from './selection'; import ViewDocumentFragment from './documentfragment'; import ViewTreeWalker from './treewalker'; -import { BR_FILLER, getDataWithoutFiller, INLINE_FILLER_LENGTH, isBlockFiller, isInlineFiller, startsWithFiller } from './filler'; +import { BR_FILLER, INLINE_FILLER_LENGTH, isBlockFiller, isInlineFiller, startsWithFiller, getDataWithoutFiller } from './filler'; import global from '@ckeditor/ckeditor5-utils/src/dom/global'; import indexOf from '@ckeditor/ckeditor5-utils/src/dom/indexof'; @@ -80,14 +80,6 @@ export default class DomConverter { */ this.blockElements = [ 'p', 'div', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6' ]; - /** - * Tag names of DOM `Element`s which are considered inline elements. - * - * @readonly - * @member {Array.} module:engine/view/domconverter~DomConverter#inlineElements - */ - this.inlineElements = [ 'span', 'strong', 'i', 'b', 'a' ]; - /** * DOM to View mapping. * @@ -378,7 +370,7 @@ export default class DomConverter { * or `null` if DOM node is a {@link module:engine/view/filler filler} or the given node is an empty text node. */ domToView( domNode, options = {} ) { - if ( isNegligibleBlockFiller( domNode, this.blockFiller, this.inlineElements, this.blockElements ) ) { + if ( isBlockFiller( domNode, this.blockFiller ) ) { return null; } @@ -1204,34 +1196,3 @@ function forEachDomNodeAncestor( node, callback ) { node = node.parentNode; } } - -// Checks if given node is negligible and should be removed. -// The negligible block fillers are: -// -   between block nodes -// - single   in blocks -// -   between blocks -// -// The relevant block fillers are: -// -   between inline elements -// -   inside inline elements -function isNegligibleBlockFiller( domNode, blockFiller, inlineElements ) { - if ( !isBlockFiller( domNode, blockFiller ) ) { - return false; - } - - const isSingleDomNode = !domNode.parentNode || domNode.parentNode.childNodes.length === 1; - - if ( isSingleDomNode ) { - // Single DOM nodes are negligible - unless they are in inline element. - return !isInlineElement( domNode.parentNode, inlineElements ); - } - - const prevIsInline = isInlineElement( domNode.previousSibling, inlineElements ); - const nextIsInline = isInlineElement( domNode.nextSibling, inlineElements ); - - return !( prevIsInline || nextIsInline ); -} - -function isInlineElement( domNode, types ) { - return domNode && types.includes( domNode.tagName.toLowerCase() ); -} diff --git a/src/view/filler.js b/src/view/filler.js index aad0890fc..88ab6d9c7 100644 --- a/src/view/filler.js +++ b/src/view/filler.js @@ -140,7 +140,9 @@ export function isBlockFiller( domNode, blockFiller ) { templateBlockFillers.set( blockFiller, templateBlockFiller ); } - return domNode.isEqualNode( templateBlockFiller ); + const isFiller = domNode.isEqualNode( templateBlockFiller ); + + return isText( templateBlockFiller ) ? isFiller && !hasInlineSibling( domNode ) : isFiller; } /** @@ -168,3 +170,80 @@ function jumpOverInlineFiller( evt, data ) { } } } + +// Checks if domNode has inline sibling. +// +// @param {Node} domNode DOM node. +// @returns {Boolean} +function hasInlineSibling( domNode ) { + const hasParent = !!domNode.parentNode; + + if ( !hasParent || domNode.parentNode.childNodes.length <= 1 ) { + return false; + } + + const prevIsInline = isInlineElement( domNode.previousSibling ); + const nextIsInline = isInlineElement( domNode.nextSibling ); + + return prevIsInline || nextIsInline; +} + +// Tag names of DOM `Element`s which are considered block elements. +// todo which other inline elements to include? +// - abbr +// - acronym +// - audio (if it has visible controls) +// - bdi +// - bdo +// - big +// - br +// - button +// - canvas +// - cite +// - code +// - data +// - datalist +// - del +// - dfn +// - em +// - embed +// - iframe +// - img +// - input +// - ins +// - kbd +// - label +// - map +// - mark +// - meter +// - noscript +// - object +// - output +// - picture +// - progress +// - q +// - ruby +// - s +// - samp +// - script +// - select +// - slot +// - small +// - strong +// - sub +// - sup +// - svg +// - template +// - textarea +// - time +// - u +// - tt +// - var +// - video +// - wbr +const inlineElements = [ 'a', 'b', 'span', 'i', 'span' ]; + +// Checks if passed domNode is considered inline. +function isInlineElement( domNode ) { + return domNode && inlineElements.includes( domNode.nodeName.toLowerCase() ); +} diff --git a/tests/view/domconverter/whitespace-handling-integration.js b/tests/view/domconverter/whitespace-handling-integration.js index b258e1cbf..2c360af94 100644 --- a/tests/view/domconverter/whitespace-handling-integration.js +++ b/tests/view/domconverter/whitespace-handling-integration.js @@ -183,7 +183,7 @@ describe( 'DomConverter – whitespace handling – integration', () => { expect( editor.getData() ).to.equal( '

foo

bar

' ); } ); - it( 'nbsp between inline elements inside blocks are not ignored', () => { + it( 'nbsp between inline elements is not ignored', () => { editor.setData( '

foo bar

' ); expect( getData( editor.model, { withoutSelection: true } ) ) @@ -192,13 +192,22 @@ describe( 'DomConverter – whitespace handling – integration', () => { expect( editor.getData() ).to.equal( '

foo bar

' ); } ); - it( 'nbsp inside inline element is not ignored', () => { - editor.setData( '

Foo 

' ); + it( 'nbsp before inline element is not ignored', () => { + editor.setData( '

 bar

' ); expect( getData( editor.model, { withoutSelection: true } ) ) - .to.equal( 'Foo<$text bold="true"> ' ); + .to.equal( ' <$text bold="true">bar' ); - expect( editor.getData() ).to.equal( '

Foo 

' ); + expect( editor.getData() ).to.equal( '

 bar

' ); + } ); + + it( 'nbsp after inline element is not ignored', () => { + editor.setData( '

bar 

' ); + + expect( getData( editor.model, { withoutSelection: true } ) ) + .to.equal( '<$text bold="true">bar ' ); + + expect( editor.getData() ).to.equal( '

bar 

' ); } ); } ); From be11659e53c7cc3164e2c18db835ca52646b7325 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 8 Aug 2019 14:09:48 +0200 Subject: [PATCH 05/25] The isBlockFiller() should allow single   inside inline elements. --- src/view/filler.js | 13 ++++-- tests/dataprocessor/htmldataprocessor.js | 53 ++++++++++++------------ 2 files changed, 36 insertions(+), 30 deletions(-) diff --git a/src/view/filler.js b/src/view/filler.js index 88ab6d9c7..90b6b8f2a 100644 --- a/src/view/filler.js +++ b/src/view/filler.js @@ -142,7 +142,7 @@ export function isBlockFiller( domNode, blockFiller ) { const isFiller = domNode.isEqualNode( templateBlockFiller ); - return isText( templateBlockFiller ) ? isFiller && !hasInlineSibling( domNode ) : isFiller; + return isText( templateBlockFiller ) ? isFiller && !hasInlineSiblingOrParent( domNode ) : isFiller; } /** @@ -175,13 +175,20 @@ function jumpOverInlineFiller( evt, data ) { // // @param {Node} domNode DOM node. // @returns {Boolean} -function hasInlineSibling( domNode ) { +function hasInlineSiblingOrParent( domNode ) { const hasParent = !!domNode.parentNode; - if ( !hasParent || domNode.parentNode.childNodes.length <= 1 ) { + + if ( !hasParent ) { return false; } + const isSingleNode = hasParent && domNode.parentNode.childNodes.length <= 1; + + if ( isSingleNode ) { + return isInlineElement( domNode.parentNode ); + } + const prevIsInline = isInlineElement( domNode.previousSibling ); const nextIsInline = isInlineElement( domNode.nextSibling ); diff --git a/tests/dataprocessor/htmldataprocessor.js b/tests/dataprocessor/htmldataprocessor.js index 8828916f1..f881f0101 100644 --- a/tests/dataprocessor/htmldataprocessor.js +++ b/tests/dataprocessor/htmldataprocessor.js @@ -59,33 +59,32 @@ describe( 'HtmlDataProcessor', () => { } ); } - // Uncomment this test after fixing #404. - // describe( 'https://github.com/ckeditor/ckeditor5-clipboard/issues/2#issuecomment-310417731 + #404', () => { - // it( 'does not lose whitespaces in Chrome\'s paste-like content', () => { - // const fragment = dataProcessor.toView( - // '' + - // 'This is the\u00a0' + - // 'third developer preview' + - // '\u00a0of\u00a0' + - // 'CKEditor\u00a05' + - // '.' - // ); - - // expect( stringify( fragment ) ).to.equal( - // 'This is the\u00a0' + - // 'third developer preview' + - // '\u00a0of\u00a0' + - // 'CKEditor\u00a05' + - // '.' - // ); - - // // Just to be sure... stringify() uses conversion and the browser extensively, - // // so it's not entirely safe. - // expect( fragment.getChild( 0 ).getChild( 1 ).getChild( 0 ).data ).to.equal( '\u00a0' ); - // expect( fragment.getChild( 2 ).getChild( 0 ).getChild( 0 ).data ).to.equal( '\u00a0' ); - // expect( fragment.getChild( 2 ).getChild( 2 ).getChild( 0 ).data ).to.equal( '\u00a0' ); - // } ); - // } ); + describe( 'https://github.com/ckeditor/ckeditor5-clipboard/issues/2#issuecomment-310417731 + #404', () => { + it( 'does not lose whitespaces in Chrome\'s paste-like content', () => { + const fragment = dataProcessor.toView( + '' + + 'This is the\u00a0' + + 'third developer preview' + + '\u00a0of\u00a0' + + 'CKEditor\u00a05' + + '.' + ); + + expect( stringify( fragment ) ).to.equal( + 'This is the\u00a0' + + 'third developer preview' + + '\u00a0of\u00a0' + + 'CKEditor\u00a05' + + '.' + ); + + // Just to be sure... stringify() uses conversion and the browser extensively, + // so it's not entirely safe. + expect( fragment.getChild( 0 ).getChild( 1 ).getChild( 0 ).data ).to.equal( '\u00a0' ); + expect( fragment.getChild( 2 ).getChild( 0 ).getChild( 0 ).data ).to.equal( '\u00a0' ); + expect( fragment.getChild( 2 ).getChild( 2 ).getChild( 0 ).data ).to.equal( '\u00a0' ); + } ); + } ); } ); describe( 'toData()', () => { From f9f89aed36e089e0efcb884cb323b74fbff0b587 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 8 Aug 2019 14:51:24 +0200 Subject: [PATCH 06/25] Use more elements in `isInline()` check. --- src/view/filler.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/view/filler.js b/src/view/filler.js index 90b6b8f2a..9a8c2f425 100644 --- a/src/view/filler.js +++ b/src/view/filler.js @@ -212,7 +212,6 @@ function hasInlineSiblingOrParent( domNode ) { // - datalist // - del // - dfn -// - em // - embed // - iframe // - img @@ -230,7 +229,6 @@ function hasInlineSiblingOrParent( domNode ) { // - progress // - q // - ruby -// - s // - samp // - script // - select @@ -243,12 +241,11 @@ function hasInlineSiblingOrParent( domNode ) { // - template // - textarea // - time -// - u // - tt // - var // - video // - wbr -const inlineElements = [ 'a', 'b', 'span', 'i', 'span' ]; +const inlineElements = [ 'a', 'b', 'em', 'i', 's', 'span', 'u' ]; // Checks if passed domNode is considered inline. function isInlineElement( domNode ) { From 0ac3b6b5dd76aca62e37a5d8b6874c39631cf6b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 8 Aug 2019 14:51:48 +0200 Subject: [PATCH 07/25] Fix code style issues. --- src/view/filler.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/view/filler.js b/src/view/filler.js index 9a8c2f425..ca7ffdc9f 100644 --- a/src/view/filler.js +++ b/src/view/filler.js @@ -178,7 +178,6 @@ function jumpOverInlineFiller( evt, data ) { function hasInlineSiblingOrParent( domNode ) { const hasParent = !!domNode.parentNode; - if ( !hasParent ) { return false; } From 92da88e6c391055dddcb2cbf60b45fcce37b0450 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Mon, 12 Aug 2019 16:59:19 +0200 Subject: [PATCH 08/25] wip --- src/view/domconverter.js | 12 +++++- src/view/filler.js | 84 +--------------------------------------- 2 files changed, 12 insertions(+), 84 deletions(-) diff --git a/src/view/domconverter.js b/src/view/domconverter.js index 66505be86..c5d3bafa9 100644 --- a/src/view/domconverter.js +++ b/src/view/domconverter.js @@ -371,7 +371,17 @@ export default class DomConverter { */ domToView( domNode, options = {} ) { if ( isBlockFiller( domNode, this.blockFiller ) ) { - return null; + const isSingle = domNode.parentNode && domNode.parentNode.childNodes.length <= 1; + + if ( isSingle && _hasDomParentOfType( domNode, this.blockElements ) ) { + return null; + } + + // if ( isText( domNode ) ) { + // + // } else { + // return null; + // } } // When node is inside UIElement return that UIElement as it's view representation. diff --git a/src/view/filler.js b/src/view/filler.js index ca7ffdc9f..aad0890fc 100644 --- a/src/view/filler.js +++ b/src/view/filler.js @@ -140,9 +140,7 @@ export function isBlockFiller( domNode, blockFiller ) { templateBlockFillers.set( blockFiller, templateBlockFiller ); } - const isFiller = domNode.isEqualNode( templateBlockFiller ); - - return isText( templateBlockFiller ) ? isFiller && !hasInlineSiblingOrParent( domNode ) : isFiller; + return domNode.isEqualNode( templateBlockFiller ); } /** @@ -170,83 +168,3 @@ function jumpOverInlineFiller( evt, data ) { } } } - -// Checks if domNode has inline sibling. -// -// @param {Node} domNode DOM node. -// @returns {Boolean} -function hasInlineSiblingOrParent( domNode ) { - const hasParent = !!domNode.parentNode; - - if ( !hasParent ) { - return false; - } - - const isSingleNode = hasParent && domNode.parentNode.childNodes.length <= 1; - - if ( isSingleNode ) { - return isInlineElement( domNode.parentNode ); - } - - const prevIsInline = isInlineElement( domNode.previousSibling ); - const nextIsInline = isInlineElement( domNode.nextSibling ); - - return prevIsInline || nextIsInline; -} - -// Tag names of DOM `Element`s which are considered block elements. -// todo which other inline elements to include? -// - abbr -// - acronym -// - audio (if it has visible controls) -// - bdi -// - bdo -// - big -// - br -// - button -// - canvas -// - cite -// - code -// - data -// - datalist -// - del -// - dfn -// - embed -// - iframe -// - img -// - input -// - ins -// - kbd -// - label -// - map -// - mark -// - meter -// - noscript -// - object -// - output -// - picture -// - progress -// - q -// - ruby -// - samp -// - script -// - select -// - slot -// - small -// - strong -// - sub -// - sup -// - svg -// - template -// - textarea -// - time -// - tt -// - var -// - video -// - wbr -const inlineElements = [ 'a', 'b', 'em', 'i', 's', 'span', 'u' ]; - -// Checks if passed domNode is considered inline. -function isInlineElement( domNode ) { - return domNode && inlineElements.includes( domNode.nodeName.toLowerCase() ); -} From 2daf569a64cd541270664ddaa523bc21005e6c58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 13 Aug 2019 12:04:17 +0200 Subject: [PATCH 09/25] Revert to previous tests of nbsp between blocks. --- .../whitespace-handling-integration.js | 29 ++----------------- 1 file changed, 3 insertions(+), 26 deletions(-) diff --git a/tests/view/domconverter/whitespace-handling-integration.js b/tests/view/domconverter/whitespace-handling-integration.js index 2c360af94..38930371a 100644 --- a/tests/view/domconverter/whitespace-handling-integration.js +++ b/tests/view/domconverter/whitespace-handling-integration.js @@ -13,7 +13,7 @@ import { getData } from '../../../src/dev-utils/model'; // dev utils' setData() loses white spaces so don't use it for tests here!!! // https://github.com/ckeditor/ckeditor5-engine/issues/1428 -describe( 'DomConverter – whitespace handling – integration', () => { +describe.only( 'DomConverter – whitespace handling – integration', () => { let editor; // See https://github.com/ckeditor/ckeditor5-engine/issues/822. @@ -26,12 +26,6 @@ describe( 'DomConverter – whitespace handling – integration', () => { editor.model.schema.extend( '$text', { allowAttributes: [ 'bold' ] } ); editor.conversion.attributeToElement( { model: 'bold', view: 'b' } ); - - editor.model.schema.register( 'blockQuote', { - allowWhere: '$block', - allowContentOf: '$root' - } ); - editor.conversion.elementToElement( { model: 'blockQuote', view: 'blockquote' } ); } ); } ); @@ -111,7 +105,8 @@ describe( 'DomConverter – whitespace handling – integration', () => { expect( editor.getData() ).to.equal( '

foo

bar

' ); } ); - it( 'nbsp between blocks is ignored #1', () => { + // Controversial result. See https://github.com/ckeditor/ckeditor5-engine/issues/987. + it( 'nbsp between blocks is not ignored', () => { editor.setData( '

foo

 

bar

' ); expect( getData( editor.model, { withoutSelection: true } ) ) @@ -120,24 +115,6 @@ describe( 'DomConverter – whitespace handling – integration', () => { expect( editor.getData() ).to.equal( '

foo

bar

' ); } ); - it( 'nbsp between blocks is ignored #2', () => { - editor.setData( 'foo
 

bar

' ); - - expect( getData( editor.model, { withoutSelection: true } ) ) - .to.equal( 'foobar' ); - - expect( editor.getData() ).to.equal( '

foo

bar

' ); - } ); - - it( 'nbsp between blocks is ignored #3', () => { - editor.setData( '
foo
 
bar
' ); - - expect( getData( editor.model, { withoutSelection: true } ) ) - .to.equal( '
foo
bar
' ); - - expect( editor.getData() ).to.equal( '

foo

bar

' ); - } ); - it( 'new lines inside blocks are ignored', () => { editor.setData( '

\nfoo\n

' ); From 86a89706502d5e537c1000aa32e4791b996c345a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 13 Aug 2019 12:09:21 +0200 Subject: [PATCH 10/25] Handle differently br and nbsp fillers. --- src/view/domconverter.js | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/view/domconverter.js b/src/view/domconverter.js index c5d3bafa9..c942c7001 100644 --- a/src/view/domconverter.js +++ b/src/view/domconverter.js @@ -373,15 +373,13 @@ export default class DomConverter { if ( isBlockFiller( domNode, this.blockFiller ) ) { const isSingle = domNode.parentNode && domNode.parentNode.childNodes.length <= 1; - if ( isSingle && _hasDomParentOfType( domNode, this.blockElements ) ) { + if ( isText( domNode ) ) { + if ( isSingle && _hasDomParentOfType( domNode, this.blockElements ) ) { + return null; + } + } else { return null; } - - // if ( isText( domNode ) ) { - // - // } else { - // return null; - // } } // When node is inside UIElement return that UIElement as it's view representation. From 600dacbf7f86894fb62db5b11c6f9163f0928045 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 13 Aug 2019 12:10:11 +0200 Subject: [PATCH 11/25] Remove .only from tests. --- tests/view/domconverter/whitespace-handling-integration.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/view/domconverter/whitespace-handling-integration.js b/tests/view/domconverter/whitespace-handling-integration.js index 38930371a..c7d82897a 100644 --- a/tests/view/domconverter/whitespace-handling-integration.js +++ b/tests/view/domconverter/whitespace-handling-integration.js @@ -13,7 +13,7 @@ import { getData } from '../../../src/dev-utils/model'; // dev utils' setData() loses white spaces so don't use it for tests here!!! // https://github.com/ckeditor/ckeditor5-engine/issues/1428 -describe.only( 'DomConverter – whitespace handling – integration', () => { +describe( 'DomConverter – whitespace handling – integration', () => { let editor; // See https://github.com/ckeditor/ckeditor5-engine/issues/822. From 6053e18370ae2f4a7e634fe3061d1aa22378bb61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 13 Aug 2019 13:27:36 +0200 Subject: [PATCH 12/25] Update "nbsp" between blocks tests cases. --- .../whitespace-handling-integration.js | 27 ++++++++++++++++--- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/tests/view/domconverter/whitespace-handling-integration.js b/tests/view/domconverter/whitespace-handling-integration.js index c7d82897a..9d3f49de4 100644 --- a/tests/view/domconverter/whitespace-handling-integration.js +++ b/tests/view/domconverter/whitespace-handling-integration.js @@ -8,6 +8,7 @@ import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; import ShiftEnter from '@ckeditor/ckeditor5-enter/src/shiftenter'; import { getData } from '../../../src/dev-utils/model'; +import TableEditing from '@ckeditor/ckeditor5-table/src/tableediting'; // NOTE: // dev utils' setData() loses white spaces so don't use it for tests here!!! @@ -20,7 +21,7 @@ describe( 'DomConverter – whitespace handling – integration', () => { describe( 'normalizing whitespaces around block boundaries (#822)', () => { beforeEach( () => { return VirtualTestEditor - .create( { plugins: [ Paragraph ] } ) + .create( { plugins: [ Paragraph, TableEditing ] } ) .then( newEditor => { editor = newEditor; @@ -106,13 +107,31 @@ describe( 'DomConverter – whitespace handling – integration', () => { } ); // Controversial result. See https://github.com/ckeditor/ckeditor5-engine/issues/987. - it( 'nbsp between blocks is not ignored', () => { + it( 'nbsp between blocks is not ignored (between paragraphs)', () => { editor.setData( '

foo

 

bar

' ); expect( getData( editor.model, { withoutSelection: true } ) ) - .to.equal( 'foobar' ); + .to.equal( 'foo bar' ); - expect( editor.getData() ).to.equal( '

foo

bar

' ); + expect( editor.getData() ).to.equal( '

foo

 

bar

' ); + } ); + + it( 'nbsp between blocks is not ignored (different blocks)', () => { + editor.setData( '
foo
 

bar

' ); + + expect( getData( editor.model, { withoutSelection: true } ) ) + .to.equal( + 'foo
' + + ' ' + + 'bar' + ); + + expect( editor.getData() ) + .to.equal( + '
foo
' + + '

 

' + + '

bar

' + ); } ); it( 'new lines inside blocks are ignored', () => { From 9ba5731b479103cd0f1c1ede957db784ef53e9c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 14 Aug 2019 14:07:46 +0200 Subject: [PATCH 13/25] Simplify nbsp between blocks tests. --- .../domconverter/whitespace-handling-integration.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/view/domconverter/whitespace-handling-integration.js b/tests/view/domconverter/whitespace-handling-integration.js index 9d3f49de4..156d2549e 100644 --- a/tests/view/domconverter/whitespace-handling-integration.js +++ b/tests/view/domconverter/whitespace-handling-integration.js @@ -8,7 +8,6 @@ import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; import ShiftEnter from '@ckeditor/ckeditor5-enter/src/shiftenter'; import { getData } from '../../../src/dev-utils/model'; -import TableEditing from '@ckeditor/ckeditor5-table/src/tableediting'; // NOTE: // dev utils' setData() loses white spaces so don't use it for tests here!!! @@ -21,7 +20,7 @@ describe( 'DomConverter – whitespace handling – integration', () => { describe( 'normalizing whitespaces around block boundaries (#822)', () => { beforeEach( () => { return VirtualTestEditor - .create( { plugins: [ Paragraph, TableEditing ] } ) + .create( { plugins: [ Paragraph ] } ) .then( newEditor => { editor = newEditor; @@ -117,18 +116,20 @@ describe( 'DomConverter – whitespace handling – integration', () => { } ); it( 'nbsp between blocks is not ignored (different blocks)', () => { - editor.setData( '
foo
 

bar

' ); + editor.model.schema.register( 'block', { inheritAllFrom: '$block' } ); + editor.conversion.elementToElement( { model: 'block', view: 'block' } ); + editor.setData( 'foo 

bar

' ); expect( getData( editor.model, { withoutSelection: true } ) ) .to.equal( - 'foo
' + + 'foo' + ' ' + 'bar' ); expect( editor.getData() ) .to.equal( - '
foo
' + + 'foo' + '

 

' + '

bar

' ); From c782dc99a4cebccd4c3dff141ed2627cdfbf16a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 3 Sep 2019 15:59:06 +0200 Subject: [PATCH 14/25] Introduce blockFillerMode in DomConverter. --- src/dataprocessor/htmldataprocessor.js | 3 +-- src/dataprocessor/xmldataprocessor.js | 3 +-- src/view/domconverter.js | 30 +++++++++++++++++++------ src/view/filler.js | 4 +++- src/view/renderer.js | 10 ++++----- tests/view/domconverter/dom-to-view.js | 10 +++++---- tests/view/domconverter/domconverter.js | 12 +++++----- tests/view/domconverter/view-to-dom.js | 4 ++-- tests/view/filler.js | 12 +++++----- tests/view/renderer.js | 6 ++--- tests/view/view/view.js | 4 ++-- 11 files changed, 58 insertions(+), 40 deletions(-) diff --git a/src/dataprocessor/htmldataprocessor.js b/src/dataprocessor/htmldataprocessor.js index 66d9893fa..76565d1cd 100644 --- a/src/dataprocessor/htmldataprocessor.js +++ b/src/dataprocessor/htmldataprocessor.js @@ -11,7 +11,6 @@ import BasicHtmlWriter from './basichtmlwriter'; import DomConverter from '../view/domconverter'; -import { NBSP_FILLER } from '../view/filler'; /** * The HTML data processor class. @@ -38,7 +37,7 @@ export default class HtmlDataProcessor { * @private * @member {module:engine/view/domconverter~DomConverter} */ - this._domConverter = new DomConverter( { blockFiller: NBSP_FILLER } ); + this._domConverter = new DomConverter( { blockFillerMode: 'nbsp' } ); /** * A basic HTML writer instance used to convert DOM elements to an HTML string. diff --git a/src/dataprocessor/xmldataprocessor.js b/src/dataprocessor/xmldataprocessor.js index 29d55f284..584637558 100644 --- a/src/dataprocessor/xmldataprocessor.js +++ b/src/dataprocessor/xmldataprocessor.js @@ -11,7 +11,6 @@ import BasicHtmlWriter from './basichtmlwriter'; import DomConverter from '../view/domconverter'; -import { NBSP_FILLER } from '../view/filler'; /** * The XML data processor class. @@ -54,7 +53,7 @@ export default class XmlDataProcessor { * @private * @member {module:engine/view/domconverter~DomConverter} */ - this._domConverter = new DomConverter( { blockFiller: NBSP_FILLER } ); + this._domConverter = new DomConverter( { blockFillerMode: 'nbsp' } ); /** * A basic HTML writer instance used to convert DOM elements to an XML string. diff --git a/src/view/domconverter.js b/src/view/domconverter.js index c942c7001..5412675d6 100644 --- a/src/view/domconverter.js +++ b/src/view/domconverter.js @@ -16,7 +16,15 @@ import ViewRange from './range'; import ViewSelection from './selection'; import ViewDocumentFragment from './documentfragment'; import ViewTreeWalker from './treewalker'; -import { BR_FILLER, INLINE_FILLER_LENGTH, isBlockFiller, isInlineFiller, startsWithFiller, getDataWithoutFiller } from './filler'; +import { + BR_FILLER, + NBSP_FILLER, + INLINE_FILLER_LENGTH, + isBlockFiller, + isInlineFiller, + startsWithFiller, + getDataWithoutFiller +} from './filler'; import global from '@ckeditor/ckeditor5-utils/src/dom/global'; import indexOf from '@ckeditor/ckeditor5-utils/src/dom/indexof'; @@ -42,7 +50,7 @@ export default class DomConverter { * Creates DOM converter. * * @param {Object} options Object with configuration options. - * @param {Function} [options.blockFiller=module:engine/view/filler~BR_FILLER] Block filler creator. + * @param {Function} [options.blockFillerMode='br'] Block filler mode - either 'br' or 'nbsp'. */ constructor( options = {} ) { // Using WeakMap prevent memory leaks: when the converter will be destroyed all referenced between View and DOM @@ -55,14 +63,22 @@ export default class DomConverter { // // I've been here. Seen stuff. Afraid of code now. + /** + * + * @readonly + * @member {String} module:engine/view/domconverter~DomConverter#blockFillerMode + */ + this.blockFillerMode = options.blockFillerMode || 'br'; + /** * Block {@link module:engine/view/filler filler} creator, which is used to create all block fillers during the * view to DOM conversion and to recognize block fillers during the DOM to view conversion. * * @readonly + * @private * @member {Function} module:engine/view/domconverter~DomConverter#blockFiller */ - this.blockFiller = options.blockFiller || BR_FILLER; + this._blockFiller = this.blockFillerMode == 'br' ? BR_FILLER : NBSP_FILLER; /** * Tag names of DOM `Element`s which are considered pre-formatted elements. @@ -254,7 +270,7 @@ export default class DomConverter { for ( const childView of viewElement.getChildren() ) { if ( fillerPositionOffset === offset ) { - yield this.blockFiller( domDocument ); + yield this._blockFiller( domDocument ); } yield this.viewToDom( childView, domDocument, options ); @@ -263,7 +279,7 @@ export default class DomConverter { } if ( fillerPositionOffset === offset ) { - yield this.blockFiller( domDocument ); + yield this._blockFiller( domDocument ); } } @@ -370,7 +386,7 @@ export default class DomConverter { * or `null` if DOM node is a {@link module:engine/view/filler filler} or the given node is an empty text node. */ domToView( domNode, options = {} ) { - if ( isBlockFiller( domNode, this.blockFiller ) ) { + if ( isBlockFiller( domNode, this.blockFillerMode ) ) { const isSingle = domNode.parentNode && domNode.parentNode.childNodes.length <= 1; if ( isText( domNode ) ) { @@ -536,7 +552,7 @@ export default class DomConverter { * @returns {module:engine/view/position~Position} viewPosition View position. */ domPositionToView( domParent, domOffset ) { - if ( isBlockFiller( domParent, this.blockFiller ) ) { + if ( isBlockFiller( domParent, this.blockFillerMode ) ) { return this.domPositionToView( domParent.parentNode, indexOf( domParent ) ); } diff --git a/src/view/filler.js b/src/view/filler.js index aad0890fc..d9ebee97c 100644 --- a/src/view/filler.js +++ b/src/view/filler.js @@ -132,7 +132,9 @@ const templateBlockFillers = new WeakMap(); * @param {Function} blockFiller Block filler creator. * @returns {Boolean} True if text node contains only {@link module:engine/view/filler~INLINE_FILLER inline filler}. */ -export function isBlockFiller( domNode, blockFiller ) { +export function isBlockFiller( domNode, blockFillerMode ) { + const blockFiller = blockFillerMode == 'br' ? BR_FILLER : NBSP_FILLER; + let templateBlockFiller = templateBlockFillers.get( blockFiller ); if ( !templateBlockFiller ) { diff --git a/src/view/renderer.js b/src/view/renderer.js index c8bc5cc2e..c3ed5b392 100644 --- a/src/view/renderer.js +++ b/src/view/renderer.js @@ -583,7 +583,7 @@ export default class Renderer { _diffNodeLists( actualDomChildren, expectedDomChildren ) { actualDomChildren = filterOutFakeSelectionContainer( actualDomChildren, this._fakeSelectionContainer ); - return diff( actualDomChildren, expectedDomChildren, sameNodes.bind( null, this.domConverter.blockFiller ) ); + return diff( actualDomChildren, expectedDomChildren, sameNodes.bind( null, this.domConverter.blockFillerMode ) ); } /** @@ -910,11 +910,11 @@ function areSimilar( node1, node2 ) { // * Two block filler elements. // // @private -// @param {Function} blockFiller Block filler creator function, see {@link module:engine/view/domconverter~DomConverter#blockFiller}. +// @param {String} blockFillerMode Block filler mode, see {@link module:engine/view/domconverter~DomConverter#blockFillerMode}. // @param {Node} node1 // @param {Node} node2 // @returns {Boolean} -function sameNodes( blockFiller, actualDomChild, expectedDomChild ) { +function sameNodes( blockFillerMode, actualDomChild, expectedDomChild ) { // Elements. if ( actualDomChild === expectedDomChild ) { return true; @@ -924,8 +924,8 @@ function sameNodes( blockFiller, actualDomChild, expectedDomChild ) { return actualDomChild.data === expectedDomChild.data; } // Block fillers. - else if ( isBlockFiller( actualDomChild, blockFiller ) && - isBlockFiller( expectedDomChild, blockFiller ) ) { + else if ( isBlockFiller( actualDomChild, blockFillerMode ) && + isBlockFiller( expectedDomChild, blockFillerMode ) ) { return true; } diff --git a/tests/view/domconverter/dom-to-view.js b/tests/view/domconverter/dom-to-view.js index f5f9b43fa..cd8fc854a 100644 --- a/tests/view/domconverter/dom-to-view.js +++ b/tests/view/domconverter/dom-to-view.js @@ -9,7 +9,7 @@ import ViewElement from '../../../src/view/element'; import ViewDocumentSelection from '../../../src/view/documentselection'; import DomConverter from '../../../src/view/domconverter'; import ViewDocumentFragment from '../../../src/view/documentfragment'; -import { INLINE_FILLER, INLINE_FILLER_LENGTH, NBSP_FILLER } from '../../../src/view/filler'; +import { INLINE_FILLER, INLINE_FILLER_LENGTH, NBSP_FILLER, BR_FILLER } from '../../../src/view/filler'; import { parse, stringify } from '../../../src/dev-utils/view'; @@ -157,7 +157,8 @@ describe( 'DomConverter', () => { } ); it( 'should return null for block filler', () => { - const domFiller = converter.blockFiller( document ); + // eslint-disable-next-line new-cap + const domFiller = BR_FILLER( document ); expect( converter.domToView( domFiller ) ).to.be.null; } ); @@ -682,7 +683,8 @@ describe( 'DomConverter', () => { } ); it( 'should skip filler', () => { - const domFiller = converter.blockFiller( document ); + // eslint-disable-next-line new-cap + const domFiller = BR_FILLER( document ); const domP = createElement( document, 'p', null, domFiller ); const viewChildren = Array.from( converter.domChildrenToView( domP ) ); @@ -761,7 +763,7 @@ describe( 'DomConverter', () => { } ); it( 'should converter position inside block filler', () => { - const converter = new DomConverter( { blockFiller: NBSP_FILLER } ); + const converter = new DomConverter( { blockFillerMode: 'nbsp' } ); const domFiller = NBSP_FILLER( document ); // eslint-disable-line new-cap const domP = createElement( document, 'p', null, domFiller ); diff --git a/tests/view/domconverter/domconverter.js b/tests/view/domconverter/domconverter.js index 661c181a0..f5a8e8cc6 100644 --- a/tests/view/domconverter/domconverter.js +++ b/tests/view/domconverter/domconverter.js @@ -10,7 +10,7 @@ import ViewEditable from '../../../src/view/editableelement'; import ViewDocument from '../../../src/view/document'; import ViewUIElement from '../../../src/view/uielement'; import ViewContainerElement from '../../../src/view/containerelement'; -import { BR_FILLER, NBSP_FILLER, INLINE_FILLER, INLINE_FILLER_LENGTH } from '../../../src/view/filler'; +import { INLINE_FILLER, INLINE_FILLER_LENGTH } from '../../../src/view/filler'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import global from '@ckeditor/ckeditor5-utils/src/dom/global'; @@ -24,13 +24,13 @@ describe( 'DomConverter', () => { } ); describe( 'constructor()', () => { - it( 'should create converter with BR block filler by default', () => { - expect( converter.blockFiller ).to.equal( BR_FILLER ); + it( 'should create converter with BR block filler mode by default', () => { + expect( converter.blockFillerMode ).to.equal( 'br' ); } ); - it( 'should create converter with defined block filler', () => { - converter = new DomConverter( { blockFiller: NBSP_FILLER } ); - expect( converter.blockFiller ).to.equal( NBSP_FILLER ); + it( 'should create converter with defined block mode filler', () => { + converter = new DomConverter( { blockFillerMode: 'nbsp' } ); + expect( converter.blockFillerMode ).to.equal( 'nbsp' ); } ); } ); diff --git a/tests/view/domconverter/view-to-dom.js b/tests/view/domconverter/view-to-dom.js index 475db8c03..77befcdcf 100644 --- a/tests/view/domconverter/view-to-dom.js +++ b/tests/view/domconverter/view-to-dom.js @@ -634,7 +634,7 @@ describe( 'DomConverter', () => { const domChildren = Array.from( converter.viewChildrenToDom( viewP, document ) ); expect( domChildren.length ).to.equal( 1 ); - expect( isBlockFiller( domChildren[ 0 ], converter.blockFiller ) ).to.be.true; + expect( isBlockFiller( domChildren[ 0 ], converter.blockFillerMode ) ).to.be.true; } ); it( 'should add filler according to fillerPositionOffset', () => { @@ -644,7 +644,7 @@ describe( 'DomConverter', () => { const domChildren = Array.from( converter.viewChildrenToDom( viewP, document ) ); expect( domChildren.length ).to.equal( 2 ); - expect( isBlockFiller( domChildren[ 0 ], converter.blockFiller ) ).to.be.true; + expect( isBlockFiller( domChildren[ 0 ], converter.blockFillerMode ) ).to.be.true; expect( domChildren[ 1 ].data ).to.equal( 'foo' ); } ); diff --git a/tests/view/filler.js b/tests/view/filler.js index 4390fda27..11ccbe420 100644 --- a/tests/view/filler.js +++ b/tests/view/filler.js @@ -127,22 +127,22 @@ describe( 'filler', () => { it( 'should return true if the node is an instance of the BR block filler', () => { const brFillerInstance = BR_FILLER( document ); // eslint-disable-line new-cap - expect( isBlockFiller( brFillerInstance, BR_FILLER ) ).to.be.true; + expect( isBlockFiller( brFillerInstance, 'br' ) ).to.be.true; // Check it twice to ensure that caching breaks nothing. - expect( isBlockFiller( brFillerInstance, BR_FILLER ) ).to.be.true; + expect( isBlockFiller( brFillerInstance, 'br' ) ).to.be.true; } ); it( 'should return true if the node is an instance of the NBSP block filler', () => { const nbspFillerInstance = NBSP_FILLER( document ); // eslint-disable-line new-cap - expect( isBlockFiller( nbspFillerInstance, NBSP_FILLER ) ).to.be.true; + expect( isBlockFiller( nbspFillerInstance, 'nbsp' ) ).to.be.true; // Check it twice to ensure that caching breaks nothing. - expect( isBlockFiller( nbspFillerInstance, NBSP_FILLER ) ).to.be.true; + expect( isBlockFiller( nbspFillerInstance, 'nbsp' ) ).to.be.true; } ); it( 'should return false for inline filler', () => { - expect( isBlockFiller( document.createTextNode( INLINE_FILLER ), BR_FILLER ) ).to.be.false; - expect( isBlockFiller( document.createTextNode( INLINE_FILLER ), NBSP_FILLER ) ).to.be.false; + expect( isBlockFiller( document.createTextNode( INLINE_FILLER ), 'br' ) ).to.be.false; + expect( isBlockFiller( document.createTextNode( INLINE_FILLER ), 'nbsp' ) ).to.be.false; } ); } ); } ); diff --git a/tests/view/renderer.js b/tests/view/renderer.js index 617a7fda6..b978b10dd 100644 --- a/tests/view/renderer.js +++ b/tests/view/renderer.js @@ -804,7 +804,7 @@ describe( 'Renderer', () => { // Step 3: Check whether in the first paragraph there's a
filler and that // in the second one there are two tags. expect( domP.childNodes.length ).to.equal( 1 ); - expect( isBlockFiller( domP.childNodes[ 0 ], BR_FILLER ) ).to.be.true; + expect( isBlockFiller( domP.childNodes[ 0 ], 'br' ) ).to.be.true; expect( domP2.childNodes.length ).to.equal( 2 ); expect( domP2.childNodes[ 0 ].tagName.toLowerCase() ).to.equal( 'b' ); @@ -886,7 +886,7 @@ describe( 'Renderer', () => { const domP = domRoot.childNodes[ 0 ]; expect( domP.childNodes.length ).to.equal( 1 ); - expect( isBlockFiller( domP.childNodes[ 0 ], BR_FILLER ) ).to.be.true; + expect( isBlockFiller( domP.childNodes[ 0 ], 'br' ) ).to.be.true; expect( domSelection.rangeCount ).to.equal( 1 ); expect( domSelection.getRangeAt( 0 ).startContainer ).to.equal( domP ); @@ -925,7 +925,7 @@ describe( 'Renderer', () => { const domP = domRoot.childNodes[ 0 ]; expect( domP.childNodes.length ).to.equal( 1 ); - expect( isBlockFiller( domP.childNodes[ 0 ], BR_FILLER ) ).to.be.true; + expect( isBlockFiller( domP.childNodes[ 0 ], 'br' ) ).to.be.true; expect( domSelection.rangeCount ).to.equal( 1 ); expect( domSelection.getRangeAt( 0 ).startContainer ).to.equal( domP ); diff --git a/tests/view/view/view.js b/tests/view/view/view.js index d8ccd3aa3..d21fa184c 100644 --- a/tests/view/view/view.js +++ b/tests/view/view/view.js @@ -18,7 +18,7 @@ import ViewRange from '../../../src/view/range'; import ViewElement from '../../../src/view/element'; import ViewPosition from '../../../src/view/position'; import ViewSelection from '../../../src/view/selection'; -import { isBlockFiller, BR_FILLER } from '../../../src/view/filler'; +import { isBlockFiller } from '../../../src/view/filler'; import count from '@ckeditor/ckeditor5-utils/src/count'; import global from '@ckeditor/ckeditor5-utils/src/dom/global'; @@ -565,7 +565,7 @@ describe( 'view', () => { view.forceRender(); expect( domDiv.childNodes.length ).to.equal( 1 ); - expect( isBlockFiller( domDiv.childNodes[ 0 ], BR_FILLER ) ).to.be.true; + expect( isBlockFiller( domDiv.childNodes[ 0 ], 'br' ) ).to.be.true; view.destroy(); domDiv.remove(); From d55bbde80b8899efda5d48475f91e0837afd09c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 3 Sep 2019 16:59:24 +0200 Subject: [PATCH 15/25] Rewrite the isBlockFiller method with logic from DomConvrter#domToView(). --- src/view/domconverter.js | 10 +--------- src/view/filler.js | 39 +++++++++++++++++++++++++-------------- tests/view/filler.js | 24 ++++++++++++++++++++---- 3 files changed, 46 insertions(+), 27 deletions(-) diff --git a/src/view/domconverter.js b/src/view/domconverter.js index 5412675d6..ca4d38a95 100644 --- a/src/view/domconverter.js +++ b/src/view/domconverter.js @@ -387,15 +387,7 @@ export default class DomConverter { */ domToView( domNode, options = {} ) { if ( isBlockFiller( domNode, this.blockFillerMode ) ) { - const isSingle = domNode.parentNode && domNode.parentNode.childNodes.length <= 1; - - if ( isText( domNode ) ) { - if ( isSingle && _hasDomParentOfType( domNode, this.blockElements ) ) { - return null; - } - } else { - return null; - } + return null; } // When node is inside UIElement return that UIElement as it's view representation. diff --git a/src/view/filler.js b/src/view/filler.js index d9ebee97c..bf6fe0996 100644 --- a/src/view/filler.js +++ b/src/view/filler.js @@ -6,6 +6,7 @@ /* globals window */ import { keyCodes } from '@ckeditor/ckeditor5-utils/src/keyboard'; +import getAncestors from '@ckeditor/ckeditor5-utils/src/dom/getancestors'; import isText from '@ckeditor/ckeditor5-utils/src/dom/istext'; /** @@ -50,6 +51,9 @@ export const BR_FILLER = domDocument => { return fillerBr; }; +// eslint-disable-next-line new-cap +const BR_FILLER_REF = BR_FILLER( window.document ); + /** * Non-breaking space filler creator. This is a function which creates ` ` text node. * It defines how the filler is created. @@ -119,30 +123,27 @@ export function getDataWithoutFiller( domText ) { } } -// Cache block fillers templates to improve performance. -const templateBlockFillers = new WeakMap(); - /** * Checks if the node is an instance of the block filler of the given type. * * const brFillerInstance = BR_FILLER( document ); - * isBlockFiller( brFillerInstance, BR_FILLER ); // true + * isBlockFiller( brFillerInstance, 'br' ); // true + * isBlockFiller( brFillerInstance, 'nbsp' ); // false * * @param {Node} domNode DOM node to check. - * @param {Function} blockFiller Block filler creator. - * @returns {Boolean} True if text node contains only {@link module:engine/view/filler~INLINE_FILLER inline filler}. + * @param {String} blockFillerMode Block filler mode - either 'br' or 'nbsp'. + * @returns {Boolean} True if a node is considered a block filler for given mode. */ export function isBlockFiller( domNode, blockFillerMode ) { - const blockFiller = blockFillerMode == 'br' ? BR_FILLER : NBSP_FILLER; - - let templateBlockFiller = templateBlockFillers.get( blockFiller ); - - if ( !templateBlockFiller ) { - templateBlockFiller = blockFiller( window.document ); - templateBlockFillers.set( blockFiller, templateBlockFiller ); + if ( blockFillerMode == 'br' ) { + return domNode.isEqualNode( BR_FILLER_REF ); } - return domNode.isEqualNode( templateBlockFiller ); + const isNBSP = isText( domNode ) && domNode.data == '\u00A0'; + const isInsideBlockNode = _hasDomParentOfType( domNode, [ 'p', 'div', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6' ] ); + const isSingle = !!domNode.parentNode && domNode.parentNode.childNodes.length === 1; + + return isNBSP && isSingle && isInsideBlockNode; } /** @@ -170,3 +171,13 @@ function jumpOverInlineFiller( evt, data ) { } } } + +function _hasDomParentOfType( node, types, boundaryParent ) { + let parents = getAncestors( node ); + + if ( boundaryParent ) { + parents = parents.slice( parents.indexOf( boundaryParent ) + 1 ); + } + + return parents.some( parent => parent.tagName && types.includes( parent.tagName.toLowerCase() ) ); +} diff --git a/tests/view/filler.js b/tests/view/filler.js index 11ccbe420..25ba34763 100644 --- a/tests/view/filler.js +++ b/tests/view/filler.js @@ -23,7 +23,7 @@ describe( 'filler', () => { } ); } ); - describe( 'startsWithFiller', () => { + describe( 'startsWithFiller()', () => { it( 'should be true for node which contains only filler', () => { const node = document.createTextNode( INLINE_FILLER ); @@ -67,7 +67,7 @@ describe( 'filler', () => { } ); } ); - describe( 'getDataWithoutFiller', () => { + describe( 'getDataWithoutFiller()', () => { it( 'should return data without filler', () => { const node = document.createTextNode( INLINE_FILLER + 'foo' ); @@ -87,7 +87,7 @@ describe( 'filler', () => { } ); } ); - describe( 'isInlineFiller', () => { + describe( 'isInlineFiller()', () => { it( 'should be true for inline filler', () => { const node = document.createTextNode( INLINE_FILLER ); @@ -123,7 +123,7 @@ describe( 'filler', () => { } ); } ); - describe( 'isBlockFiller', () => { + describe( 'isBlockFiller()', () => { it( 'should return true if the node is an instance of the BR block filler', () => { const brFillerInstance = BR_FILLER( document ); // eslint-disable-line new-cap @@ -134,12 +134,28 @@ describe( 'filler', () => { it( 'should return true if the node is an instance of the NBSP block filler', () => { const nbspFillerInstance = NBSP_FILLER( document ); // eslint-disable-line new-cap + // NBSP must be check inside a context. + const context = document.createElement( 'div' ); + context.appendChild( nbspFillerInstance ); expect( isBlockFiller( nbspFillerInstance, 'nbsp' ) ).to.be.true; // Check it twice to ensure that caching breaks nothing. expect( isBlockFiller( nbspFillerInstance, 'nbsp' ) ).to.be.true; } ); + it( 'should return false if the nbsp filler is inside context', () => { + const nbspFillerInstance = NBSP_FILLER( document ); // eslint-disable-line new-cap + // NBSP must be check inside a context. + const context = document.createElement( 'div' ); + context.appendChild( nbspFillerInstance ); + // eslint-disable-next-line new-cap + context.appendChild( NBSP_FILLER( document ) ); + + expect( isBlockFiller( nbspFillerInstance, 'nbsp' ) ).to.be.false; + // Check it twice to ensure that caching breaks nothing. + expect( isBlockFiller( nbspFillerInstance, 'nbsp' ) ).to.be.false; + } ); + it( 'should return false for inline filler', () => { expect( isBlockFiller( document.createTextNode( INLINE_FILLER ), 'br' ) ).to.be.false; expect( isBlockFiller( document.createTextNode( INLINE_FILLER ), 'nbsp' ) ).to.be.false; From e5c736101472c703cf0356658788de3a7f5c2105 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 3 Sep 2019 17:06:22 +0200 Subject: [PATCH 16/25] Simplify isBlockFiller internals. --- src/view/filler.js | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/view/filler.js b/src/view/filler.js index bf6fe0996..2c466a0ff 100644 --- a/src/view/filler.js +++ b/src/view/filler.js @@ -140,10 +140,8 @@ export function isBlockFiller( domNode, blockFillerMode ) { } const isNBSP = isText( domNode ) && domNode.data == '\u00A0'; - const isInsideBlockNode = _hasDomParentOfType( domNode, [ 'p', 'div', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6' ] ); - const isSingle = !!domNode.parentNode && domNode.parentNode.childNodes.length === 1; - return isNBSP && isSingle && isInsideBlockNode; + return isNBSP && hasBlockParent( domNode ) && domNode.parentNode.childNodes.length === 1; } /** @@ -172,12 +170,10 @@ function jumpOverInlineFiller( evt, data ) { } } -function _hasDomParentOfType( node, types, boundaryParent ) { - let parents = getAncestors( node ); +const blockElements = [ 'p', 'div', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6' ]; - if ( boundaryParent ) { - parents = parents.slice( parents.indexOf( boundaryParent ) + 1 ); - } +function hasBlockParent( node ) { + const parent = node.parentNode; - return parents.some( parent => parent.tagName && types.includes( parent.tagName.toLowerCase() ) ); + return parent && parent.tagName && blockElements.includes( parent.tagName.toLowerCase() ); } From 101260586466ef895232799028a91e300a21d178 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 4 Sep 2019 11:20:49 +0200 Subject: [PATCH 17/25] Expand documentation for new block filler methods. --- src/view/domconverter.js | 23 ++++++++++---------- src/view/filler.js | 46 ++++++++++++++++++++++++++++++---------- 2 files changed, 47 insertions(+), 22 deletions(-) diff --git a/src/view/domconverter.js b/src/view/domconverter.js index ca4d38a95..c10ba5b30 100644 --- a/src/view/domconverter.js +++ b/src/view/domconverter.js @@ -50,7 +50,7 @@ export default class DomConverter { * Creates DOM converter. * * @param {Object} options Object with configuration options. - * @param {Function} [options.blockFillerMode='br'] Block filler mode - either 'br' or 'nbsp'. + * @param {module:engine/view/filler~blockFillerMode} [options.blockFillerMode='br'] Type of a block filler w */ constructor( options = {} ) { // Using WeakMap prevent memory leaks: when the converter will be destroyed all referenced between View and DOM @@ -64,22 +64,13 @@ export default class DomConverter { // I've been here. Seen stuff. Afraid of code now. /** + * The mode of a block filler used by DOM converter. * * @readonly * @member {String} module:engine/view/domconverter~DomConverter#blockFillerMode */ this.blockFillerMode = options.blockFillerMode || 'br'; - /** - * Block {@link module:engine/view/filler filler} creator, which is used to create all block fillers during the - * view to DOM conversion and to recognize block fillers during the DOM to view conversion. - * - * @readonly - * @private - * @member {Function} module:engine/view/domconverter~DomConverter#blockFiller - */ - this._blockFiller = this.blockFillerMode == 'br' ? BR_FILLER : NBSP_FILLER; - /** * Tag names of DOM `Element`s which are considered pre-formatted elements. * @@ -96,6 +87,16 @@ export default class DomConverter { */ this.blockElements = [ 'p', 'div', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6' ]; + /** + * Block {@link module:engine/view/filler filler} creator, which is used to create all block fillers during the + * view to DOM conversion and to recognize block fillers during the DOM to view conversion. + * + * @readonly + * @private + * @member {Function} module:engine/view/domconverter~DomConverter#_blockFiller + */ + this._blockFiller = this.blockFillerMode == 'br' ? BR_FILLER : NBSP_FILLER; + /** * DOM to View mapping. * diff --git a/src/view/filler.js b/src/view/filler.js index 2c466a0ff..f760a9001 100644 --- a/src/view/filler.js +++ b/src/view/filler.js @@ -130,18 +130,14 @@ export function getDataWithoutFiller( domText ) { * isBlockFiller( brFillerInstance, 'br' ); // true * isBlockFiller( brFillerInstance, 'nbsp' ); // false * + * **Note:**: For 'nbsp' mode the method also checks context of a node so it cannot be a detached node. + * * @param {Node} domNode DOM node to check. - * @param {String} blockFillerMode Block filler mode - either 'br' or 'nbsp'. + * @param {module:engine/view/filler~blockFillerMode} mode Mode of a block filler. * @returns {Boolean} True if a node is considered a block filler for given mode. */ -export function isBlockFiller( domNode, blockFillerMode ) { - if ( blockFillerMode == 'br' ) { - return domNode.isEqualNode( BR_FILLER_REF ); - } - - const isNBSP = isText( domNode ) && domNode.data == '\u00A0'; - - return isNBSP && hasBlockParent( domNode ) && domNode.parentNode.childNodes.length === 1; +export function isBlockFiller( domNode, mode ) { + return mode == 'br' ? domNode.isEqualNode( BR_FILLER_REF ) : isNbspFillerFiller( domNode ); } /** @@ -170,10 +166,38 @@ function jumpOverInlineFiller( evt, data ) { } } +// Checks if given node is a nbsp block filler. +// +// A   is a block filler only if it is a single child of a block element. +// +// @param {Node} domNode DOM node. +// @returns {Boolean} +function isNbspFillerFiller( domNode ) { + const isNBSP = isText( domNode ) && domNode.data == '\u00A0'; + + return isNBSP && hasBlockParent( domNode ) && domNode.parentNode.childNodes.length === 1; +} + +// Name of DOM nodes that are considered a block. const blockElements = [ 'p', 'div', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6' ]; -function hasBlockParent( node ) { - const parent = node.parentNode; +// Checks if domNode has block parent. +// +// @param {Node} domNode DOM node. +// @returns {Boolean} +function hasBlockParent( domNode ) { + const parent = domNode.parentNode; return parent && parent.tagName && blockElements.includes( parent.tagName.toLowerCase() ); } + +/** + * Enum representing type of the block filler. + * + * Possible values: + * + * * `br` - for `
` block filler used in editing view, + * * `nbsp` - for ` ` block fillers used in the data. + * + * @typedef {String} module:engine/view/filler~blockFillerMode + */ From dbecbf1ef2bec70c8b6d968e88e5ed6a9509a5f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 4 Sep 2019 11:22:39 +0200 Subject: [PATCH 18/25] Remove unused import --- src/view/filler.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/view/filler.js b/src/view/filler.js index f760a9001..98fdc1f35 100644 --- a/src/view/filler.js +++ b/src/view/filler.js @@ -6,7 +6,6 @@ /* globals window */ import { keyCodes } from '@ckeditor/ckeditor5-utils/src/keyboard'; -import getAncestors from '@ckeditor/ckeditor5-utils/src/dom/getancestors'; import isText from '@ckeditor/ckeditor5-utils/src/dom/istext'; /** From c62a9fdb1526945007655862ca55b16bc6fed422 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 4 Sep 2019 11:26:54 +0200 Subject: [PATCH 19/25] Rename isNbspFillerFiller() to isNbspBlockFiller(). --- src/view/filler.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/view/filler.js b/src/view/filler.js index 98fdc1f35..9740b978e 100644 --- a/src/view/filler.js +++ b/src/view/filler.js @@ -136,7 +136,7 @@ export function getDataWithoutFiller( domText ) { * @returns {Boolean} True if a node is considered a block filler for given mode. */ export function isBlockFiller( domNode, mode ) { - return mode == 'br' ? domNode.isEqualNode( BR_FILLER_REF ) : isNbspFillerFiller( domNode ); + return mode == 'br' ? domNode.isEqualNode( BR_FILLER_REF ) : isNbspBlockFiller( domNode ); } /** @@ -171,7 +171,7 @@ function jumpOverInlineFiller( evt, data ) { // // @param {Node} domNode DOM node. // @returns {Boolean} -function isNbspFillerFiller( domNode ) { +function isNbspBlockFiller( domNode ) { const isNBSP = isText( domNode ) && domNode.data == '\u00A0'; return isNBSP && hasBlockParent( domNode ) && domNode.parentNode.childNodes.length === 1; From 75bad14230cbc1e87d9209770a2a4320aae781cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 4 Sep 2019 11:52:08 +0200 Subject: [PATCH 20/25] Filler docs fixes. --- src/view/domconverter.js | 4 ++-- src/view/filler.js | 20 ++++++++++++-------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/view/domconverter.js b/src/view/domconverter.js index c10ba5b30..faecef266 100644 --- a/src/view/domconverter.js +++ b/src/view/domconverter.js @@ -50,7 +50,7 @@ export default class DomConverter { * Creates DOM converter. * * @param {Object} options Object with configuration options. - * @param {module:engine/view/filler~blockFillerMode} [options.blockFillerMode='br'] Type of a block filler w + * @param {module:engine/view/filler~BlockFillerMode} [options.blockFillerMode='br'] Type of a block filler w */ constructor( options = {} ) { // Using WeakMap prevent memory leaks: when the converter will be destroyed all referenced between View and DOM @@ -67,7 +67,7 @@ export default class DomConverter { * The mode of a block filler used by DOM converter. * * @readonly - * @member {String} module:engine/view/domconverter~DomConverter#blockFillerMode + * @member {String} module:engine/view/domconverter~DomConverter#BlockFillerMode */ this.blockFillerMode = options.blockFillerMode || 'br'; diff --git a/src/view/filler.js b/src/view/filler.js index 9740b978e..1147bc035 100644 --- a/src/view/filler.js +++ b/src/view/filler.js @@ -68,13 +68,17 @@ export const NBSP_FILLER = domDocument => domDocument.createTextNode( '\u00A0' ) export const INLINE_FILLER_LENGTH = 7; /** - * Inline filler which is sequence of the zero width spaces. + * Inline filler which is a sequence of the zero width spaces. */ -export let INLINE_FILLER = ''; +export const INLINE_FILLER = ( () => { + let inlineFiller = ''; -for ( let i = 0; i < INLINE_FILLER_LENGTH; i++ ) { - INLINE_FILLER += '\u200b'; -} + for ( let i = 0; i < INLINE_FILLER_LENGTH; i++ ) { + inlineFiller += '\u200b'; + } + + return inlineFiller; +} )(); // Usu IIF so the INLINE_FILLER appears as a constant in the docs. /** * Checks if the node is a text node which starts with the {@link module:engine/view/filler~INLINE_FILLER inline filler}. @@ -129,10 +133,10 @@ export function getDataWithoutFiller( domText ) { * isBlockFiller( brFillerInstance, 'br' ); // true * isBlockFiller( brFillerInstance, 'nbsp' ); // false * - * **Note:**: For 'nbsp' mode the method also checks context of a node so it cannot be a detached node. + * **Note:**: For the `'nbsp'` mode the method also checks context of a node so it cannot be a detached node. * * @param {Node} domNode DOM node to check. - * @param {module:engine/view/filler~blockFillerMode} mode Mode of a block filler. + * @param {module:engine/view/filler~BlockFillerMode} mode Mode of a block filler. * @returns {Boolean} True if a node is considered a block filler for given mode. */ export function isBlockFiller( domNode, mode ) { @@ -198,5 +202,5 @@ function hasBlockParent( domNode ) { * * `br` - for `
` block filler used in editing view, * * `nbsp` - for ` ` block fillers used in the data. * - * @typedef {String} module:engine/view/filler~blockFillerMode + * @typedef {String} module:engine/view/filler~BlockFillerMode */ From a9f4b7a5ec2fba08cb5bdef948ee38b4353bb3f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 5 Sep 2019 18:04:26 +0200 Subject: [PATCH 21/25] Docs fix. --- src/view/domconverter.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/view/domconverter.js b/src/view/domconverter.js index faecef266..3cfed3f34 100644 --- a/src/view/domconverter.js +++ b/src/view/domconverter.js @@ -67,7 +67,7 @@ export default class DomConverter { * The mode of a block filler used by DOM converter. * * @readonly - * @member {String} module:engine/view/domconverter~DomConverter#BlockFillerMode + * @member {String} module:engine/view/domconverter~DomConverter#blockFillerMode */ this.blockFillerMode = options.blockFillerMode || 'br'; From d9dbda288b7bf300f9beede3d1aa786d329dc737 Mon Sep 17 00:00:00 2001 From: Maciej Date: Tue, 17 Sep 2019 15:19:58 +0200 Subject: [PATCH 22/25] Update src/view/domconverter.js MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Piotrek Koszuliński --- src/view/domconverter.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/view/domconverter.js b/src/view/domconverter.js index 3cfed3f34..68165af30 100644 --- a/src/view/domconverter.js +++ b/src/view/domconverter.js @@ -67,7 +67,7 @@ export default class DomConverter { * The mode of a block filler used by DOM converter. * * @readonly - * @member {String} module:engine/view/domconverter~DomConverter#blockFillerMode + * @member {'br'|'nbsp'} module:engine/view/domconverter~DomConverter#blockFillerMode */ this.blockFillerMode = options.blockFillerMode || 'br'; From d37094790707c3cb0c28562a8d4de6f002ec07fa Mon Sep 17 00:00:00 2001 From: Maciej Date: Tue, 17 Sep 2019 15:34:47 +0200 Subject: [PATCH 23/25] Update src/view/domconverter.js MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Piotrek Koszuliński --- src/view/domconverter.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/view/domconverter.js b/src/view/domconverter.js index 68165af30..4c2f0b457 100644 --- a/src/view/domconverter.js +++ b/src/view/domconverter.js @@ -50,7 +50,7 @@ export default class DomConverter { * Creates DOM converter. * * @param {Object} options Object with configuration options. - * @param {module:engine/view/filler~BlockFillerMode} [options.blockFillerMode='br'] Type of a block filler w + * @param {module:engine/view/filler~BlockFillerMode} [options.blockFillerMode='br'] The type of the block filler to use. */ constructor( options = {} ) { // Using WeakMap prevent memory leaks: when the converter will be destroyed all referenced between View and DOM From 23b1b792267dd093ccdd7053d56b16bb76fcd2d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 17 Sep 2019 16:57:36 +0200 Subject: [PATCH 24/25] Move block filler handling to DomConverter. --- src/view/domconverter.js | 69 ++++++++++++++++++---- src/view/filler.js | 76 +++---------------------- src/view/renderer.js | 10 ++-- tests/view/domconverter/dom-to-view.js | 2 +- tests/view/domconverter/domconverter.js | 70 ++++++++++++++++++++++- tests/view/domconverter/view-to-dom.js | 6 +- tests/view/filler.js | 42 -------------- tests/view/renderer.js | 8 +-- tests/view/view/view.js | 3 +- 9 files changed, 149 insertions(+), 137 deletions(-) diff --git a/src/view/domconverter.js b/src/view/domconverter.js index 4c2f0b457..753b12c58 100644 --- a/src/view/domconverter.js +++ b/src/view/domconverter.js @@ -7,7 +7,7 @@ * @module engine/view/domconverter */ -/* globals document, Node, NodeFilter, Text */ +/* globals window, document, Node, NodeFilter, Text */ import ViewText from './text'; import ViewElement from './element'; @@ -16,15 +16,7 @@ import ViewRange from './range'; import ViewSelection from './selection'; import ViewDocumentFragment from './documentfragment'; import ViewTreeWalker from './treewalker'; -import { - BR_FILLER, - NBSP_FILLER, - INLINE_FILLER_LENGTH, - isBlockFiller, - isInlineFiller, - startsWithFiller, - getDataWithoutFiller -} from './filler'; +import { BR_FILLER, getDataWithoutFiller, INLINE_FILLER_LENGTH, isInlineFiller, NBSP_FILLER, startsWithFiller } from './filler'; import global from '@ckeditor/ckeditor5-utils/src/dom/global'; import indexOf from '@ckeditor/ckeditor5-utils/src/dom/indexof'; @@ -33,6 +25,9 @@ import getCommonAncestor from '@ckeditor/ckeditor5-utils/src/dom/getcommonancest import isText from '@ckeditor/ckeditor5-utils/src/dom/istext'; import { isElement } from 'lodash-es'; +// eslint-disable-next-line new-cap +const BR_FILLER_REF = BR_FILLER( window.document ); + /** * DomConverter is a set of tools to do transformations between DOM nodes and view nodes. It also handles * {@link module:engine/view/domconverter~DomConverter#bindElements binding} these nodes. @@ -387,7 +382,7 @@ export default class DomConverter { * or `null` if DOM node is a {@link module:engine/view/filler filler} or the given node is an empty text node. */ domToView( domNode, options = {} ) { - if ( isBlockFiller( domNode, this.blockFillerMode ) ) { + if ( this.isBlockFiller( domNode, this.blockFillerMode ) ) { return null; } @@ -545,7 +540,7 @@ export default class DomConverter { * @returns {module:engine/view/position~Position} viewPosition View position. */ domPositionToView( domParent, domOffset ) { - if ( isBlockFiller( domParent, this.blockFillerMode ) ) { + if ( this.isBlockFiller( domParent, this.blockFillerMode ) ) { return this.domPositionToView( domParent.parentNode, indexOf( domParent ) ); } @@ -802,6 +797,23 @@ export default class DomConverter { return node && node.nodeType == Node.COMMENT_NODE; } + /** + * Checks if the node is an instance of the block filler for this DOM converter. + * + * const converter = new DomConverter( { blockFillerMode: 'br' } ); + * + * converter.isBlockFiller( BR_FILLER( document ) ); // true + * converter.isBlockFiller( NBSP_FILLER( document ) ); // false + * + * **Note:**: For the `'nbsp'` mode the method also checks context of a node so it cannot be a detached node. + * + * @param {Node} domNode DOM node to check. + * @returns {Boolean} True if a node is considered a block filler for given mode. + */ + isBlockFiller( domNode ) { + return this.blockFillerMode == 'br' ? domNode.isEqualNode( BR_FILLER_REF ) : isNbspBlockFiller( domNode, this.blockElements ); + } + /** * Returns `true` if given selection is a backward selection, that is, if it's `focus` is before `anchor`. * @@ -1213,3 +1225,36 @@ function forEachDomNodeAncestor( node, callback ) { node = node.parentNode; } } + +// Checks if given node is a nbsp block filler. +// +// A   is a block filler only if it is a single child of a block element. +// +// @param {Node} domNode DOM node. +// @returns {Boolean} +function isNbspBlockFiller( domNode, blockElements ) { + const isNBSP = isText( domNode ) && domNode.data == '\u00A0'; + + return isNBSP && hasBlockParent( domNode, blockElements ) && domNode.parentNode.childNodes.length === 1; +} + +// Checks if domNode has block parent. +// +// @param {Node} domNode DOM node. +// @returns {Boolean} +function hasBlockParent( domNode, blockElements ) { + const parent = domNode.parentNode; + + return parent && parent.tagName && blockElements.includes( parent.tagName.toLowerCase() ); +} + +/** + * Enum representing type of the block filler. + * + * Possible values: + * + * * `br` - for `
` block filler used in editing view, + * * `nbsp` - for ` ` block fillers used in the data. + * + * @typedef {String} module:engine/view/filler~BlockFillerMode + */ diff --git a/src/view/filler.js b/src/view/filler.js index 1147bc035..f8bca06a6 100644 --- a/src/view/filler.js +++ b/src/view/filler.js @@ -3,8 +3,6 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ -/* globals window */ - import { keyCodes } from '@ckeditor/ckeditor5-utils/src/keyboard'; import isText from '@ckeditor/ckeditor5-utils/src/dom/istext'; @@ -36,6 +34,15 @@ import isText from '@ckeditor/ckeditor5-utils/src/dom/istext'; * @module engine/view/filler */ +/** + * Non-breaking space filler creator. This is a function which creates ` ` text node. + * It defines how the filler is created. + * + * @see module:engine/view/filler~BR_FILLER + * @function + */ +export const NBSP_FILLER = domDocument => domDocument.createTextNode( '\u00A0' ); + /** * `
` filler creator. This is a function which creates `
` element. * It defines how the filler is created. @@ -50,18 +57,6 @@ export const BR_FILLER = domDocument => { return fillerBr; }; -// eslint-disable-next-line new-cap -const BR_FILLER_REF = BR_FILLER( window.document ); - -/** - * Non-breaking space filler creator. This is a function which creates ` ` text node. - * It defines how the filler is created. - * - * @see module:engine/view/filler~BR_FILLER - * @function - */ -export const NBSP_FILLER = domDocument => domDocument.createTextNode( '\u00A0' ); - /** * Length of the {@link module:engine/view/filler~INLINE_FILLER INLINE_FILLER}. */ @@ -126,23 +121,6 @@ export function getDataWithoutFiller( domText ) { } } -/** - * Checks if the node is an instance of the block filler of the given type. - * - * const brFillerInstance = BR_FILLER( document ); - * isBlockFiller( brFillerInstance, 'br' ); // true - * isBlockFiller( brFillerInstance, 'nbsp' ); // false - * - * **Note:**: For the `'nbsp'` mode the method also checks context of a node so it cannot be a detached node. - * - * @param {Node} domNode DOM node to check. - * @param {module:engine/view/filler~BlockFillerMode} mode Mode of a block filler. - * @returns {Boolean} True if a node is considered a block filler for given mode. - */ -export function isBlockFiller( domNode, mode ) { - return mode == 'br' ? domNode.isEqualNode( BR_FILLER_REF ) : isNbspBlockFiller( domNode ); -} - /** * Assign key observer which move cursor from the end of the inline filler to the beginning of it when * the left arrow is pressed, so the filler does not break navigation. @@ -168,39 +146,3 @@ function jumpOverInlineFiller( evt, data ) { } } } - -// Checks if given node is a nbsp block filler. -// -// A   is a block filler only if it is a single child of a block element. -// -// @param {Node} domNode DOM node. -// @returns {Boolean} -function isNbspBlockFiller( domNode ) { - const isNBSP = isText( domNode ) && domNode.data == '\u00A0'; - - return isNBSP && hasBlockParent( domNode ) && domNode.parentNode.childNodes.length === 1; -} - -// Name of DOM nodes that are considered a block. -const blockElements = [ 'p', 'div', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6' ]; - -// Checks if domNode has block parent. -// -// @param {Node} domNode DOM node. -// @returns {Boolean} -function hasBlockParent( domNode ) { - const parent = domNode.parentNode; - - return parent && parent.tagName && blockElements.includes( parent.tagName.toLowerCase() ); -} - -/** - * Enum representing type of the block filler. - * - * Possible values: - * - * * `br` - for `
` block filler used in editing view, - * * `nbsp` - for ` ` block fillers used in the data. - * - * @typedef {String} module:engine/view/filler~BlockFillerMode - */ diff --git a/src/view/renderer.js b/src/view/renderer.js index a8b502e69..53a6abede 100644 --- a/src/view/renderer.js +++ b/src/view/renderer.js @@ -11,7 +11,7 @@ import ViewText from './text'; import ViewPosition from './position'; -import { INLINE_FILLER, INLINE_FILLER_LENGTH, startsWithFiller, isInlineFiller, isBlockFiller } from './filler'; +import { INLINE_FILLER, INLINE_FILLER_LENGTH, startsWithFiller, isInlineFiller } from './filler'; import mix from '@ckeditor/ckeditor5-utils/src/mix'; import diff from '@ckeditor/ckeditor5-utils/src/diff'; @@ -590,7 +590,7 @@ export default class Renderer { _diffNodeLists( actualDomChildren, expectedDomChildren ) { actualDomChildren = filterOutFakeSelectionContainer( actualDomChildren, this._fakeSelectionContainer ); - return diff( actualDomChildren, expectedDomChildren, sameNodes.bind( null, this.domConverter.blockFillerMode ) ); + return diff( actualDomChildren, expectedDomChildren, sameNodes.bind( null, this.domConverter ) ); } /** @@ -926,7 +926,7 @@ function areSimilar( node1, node2 ) { // @param {Node} node1 // @param {Node} node2 // @returns {Boolean} -function sameNodes( blockFillerMode, actualDomChild, expectedDomChild ) { +function sameNodes( domConverter, actualDomChild, expectedDomChild ) { // Elements. if ( actualDomChild === expectedDomChild ) { return true; @@ -936,8 +936,8 @@ function sameNodes( blockFillerMode, actualDomChild, expectedDomChild ) { return actualDomChild.data === expectedDomChild.data; } // Block fillers. - else if ( isBlockFiller( actualDomChild, blockFillerMode ) && - isBlockFiller( expectedDomChild, blockFillerMode ) ) { + else if ( domConverter.isBlockFiller( actualDomChild ) && + domConverter.isBlockFiller( expectedDomChild ) ) { return true; } diff --git a/tests/view/domconverter/dom-to-view.js b/tests/view/domconverter/dom-to-view.js index cd8fc854a..5880cea45 100644 --- a/tests/view/domconverter/dom-to-view.js +++ b/tests/view/domconverter/dom-to-view.js @@ -9,7 +9,7 @@ import ViewElement from '../../../src/view/element'; import ViewDocumentSelection from '../../../src/view/documentselection'; import DomConverter from '../../../src/view/domconverter'; import ViewDocumentFragment from '../../../src/view/documentfragment'; -import { INLINE_FILLER, INLINE_FILLER_LENGTH, NBSP_FILLER, BR_FILLER } from '../../../src/view/filler'; +import { BR_FILLER, INLINE_FILLER, INLINE_FILLER_LENGTH, NBSP_FILLER } from '../../../src/view/filler'; import { parse, stringify } from '../../../src/dev-utils/view'; diff --git a/tests/view/domconverter/domconverter.js b/tests/view/domconverter/domconverter.js index f5a8e8cc6..0a276ae5b 100644 --- a/tests/view/domconverter/domconverter.js +++ b/tests/view/domconverter/domconverter.js @@ -10,7 +10,7 @@ import ViewEditable from '../../../src/view/editableelement'; import ViewDocument from '../../../src/view/document'; import ViewUIElement from '../../../src/view/uielement'; import ViewContainerElement from '../../../src/view/containerelement'; -import { INLINE_FILLER, INLINE_FILLER_LENGTH } from '../../../src/view/filler'; +import { BR_FILLER, INLINE_FILLER, INLINE_FILLER_LENGTH, NBSP_FILLER } from '../../../src/view/filler'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import global from '@ckeditor/ckeditor5-utils/src/dom/global'; @@ -290,4 +290,72 @@ describe( 'DomConverter', () => { } ); } ); } ); + + describe( 'isBlockFiller()', () => { + describe( 'mode "nbsp"', () => { + beforeEach( () => { + converter = new DomConverter( { blockFillerMode: 'nbsp' } ); + } ); + + it( 'should return true if the node is an instance of the NBSP block filler', () => { + converter = new DomConverter( { blockFillerMode: 'nbsp' } ); + const nbspFillerInstance = NBSP_FILLER( document ); // eslint-disable-line new-cap + // NBSP must be check inside a context. + const context = document.createElement( 'div' ); + context.appendChild( nbspFillerInstance ); + + expect( converter.isBlockFiller( nbspFillerInstance ) ).to.be.true; + } ); + + it( 'should return false if the node is an instance of the BR block filler', () => { + const brFillerInstance = BR_FILLER( document ); // eslint-disable-line new-cap + + expect( converter.isBlockFiller( brFillerInstance ) ).to.be.false; + } ); + + it( 'should return false if the nbsp filler is inside context', () => { + converter = new DomConverter( { blockFillerMode: 'nbsp' } ); + const nbspFillerInstance = NBSP_FILLER( document ); // eslint-disable-line new-cap + // NBSP must be check inside a context. + const context = document.createElement( 'div' ); + context.appendChild( nbspFillerInstance ); + // eslint-disable-next-line new-cap + context.appendChild( NBSP_FILLER( document ) ); + + expect( converter.isBlockFiller( nbspFillerInstance ) ).to.be.false; + } ); + + it( 'should return false for inline filler', () => { + expect( converter.isBlockFiller( document.createTextNode( INLINE_FILLER ) ) ).to.be.false; + } ); + } ); + + describe( 'mode "br"', () => { + beforeEach( () => { + converter = new DomConverter( { blockFillerMode: 'br' } ); + } ); + + it( 'should return true if the node is an instance of the BR block filler', () => { + const brFillerInstance = BR_FILLER( document ); // eslint-disable-line new-cap + + expect( converter.isBlockFiller( brFillerInstance ) ).to.be.true; + // Check it twice to ensure that caching breaks nothing. + expect( converter.isBlockFiller( brFillerInstance ) ).to.be.true; + } ); + + it( 'should return false if the node is an instance of the NBSP block filler', () => { + converter = new DomConverter( { blockFillerMode: 'br' } ); + const nbspFillerInstance = NBSP_FILLER( document ); // eslint-disable-line new-cap + // NBSP must be check inside a context. + const context = document.createElement( 'div' ); + context.appendChild( nbspFillerInstance ); + + expect( converter.isBlockFiller( nbspFillerInstance ) ).to.be.false; + } ); + + it( 'should return false for inline filler', () => { + expect( converter.isBlockFiller( document.createTextNode( INLINE_FILLER ) ) ).to.be.false; + } ); + } ); + } ); } ); diff --git a/tests/view/domconverter/view-to-dom.js b/tests/view/domconverter/view-to-dom.js index 77befcdcf..5948600fa 100644 --- a/tests/view/domconverter/view-to-dom.js +++ b/tests/view/domconverter/view-to-dom.js @@ -13,7 +13,7 @@ import ViewAttributeElement from '../../../src/view/attributeelement'; import ViewEmptyElement from '../../../src/view/emptyelement'; import DomConverter from '../../../src/view/domconverter'; import ViewDocumentFragment from '../../../src/view/documentfragment'; -import { INLINE_FILLER, INLINE_FILLER_LENGTH, isBlockFiller } from '../../../src/view/filler'; +import { INLINE_FILLER, INLINE_FILLER_LENGTH } from '../../../src/view/filler'; import { parse } from '../../../src/dev-utils/view'; @@ -634,7 +634,7 @@ describe( 'DomConverter', () => { const domChildren = Array.from( converter.viewChildrenToDom( viewP, document ) ); expect( domChildren.length ).to.equal( 1 ); - expect( isBlockFiller( domChildren[ 0 ], converter.blockFillerMode ) ).to.be.true; + expect( converter.isBlockFiller( domChildren[ 0 ] ) ).to.be.true; } ); it( 'should add filler according to fillerPositionOffset', () => { @@ -644,7 +644,7 @@ describe( 'DomConverter', () => { const domChildren = Array.from( converter.viewChildrenToDom( viewP, document ) ); expect( domChildren.length ).to.equal( 2 ); - expect( isBlockFiller( domChildren[ 0 ], converter.blockFillerMode ) ).to.be.true; + expect( converter.isBlockFiller( domChildren[ 0 ] ) ).to.be.true; expect( domChildren[ 1 ].data ).to.equal( 'foo' ); } ); diff --git a/tests/view/filler.js b/tests/view/filler.js index 25ba34763..8f0ec00d4 100644 --- a/tests/view/filler.js +++ b/tests/view/filler.js @@ -6,14 +6,11 @@ /* globals document */ import { - BR_FILLER, - NBSP_FILLER, INLINE_FILLER_LENGTH, INLINE_FILLER, startsWithFiller, isInlineFiller, getDataWithoutFiller, - isBlockFiller } from '../../src/view/filler'; describe( 'filler', () => { @@ -122,43 +119,4 @@ describe( 'filler', () => { document.body.removeChild( iframe ); } ); } ); - - describe( 'isBlockFiller()', () => { - it( 'should return true if the node is an instance of the BR block filler', () => { - const brFillerInstance = BR_FILLER( document ); // eslint-disable-line new-cap - - expect( isBlockFiller( brFillerInstance, 'br' ) ).to.be.true; - // Check it twice to ensure that caching breaks nothing. - expect( isBlockFiller( brFillerInstance, 'br' ) ).to.be.true; - } ); - - it( 'should return true if the node is an instance of the NBSP block filler', () => { - const nbspFillerInstance = NBSP_FILLER( document ); // eslint-disable-line new-cap - // NBSP must be check inside a context. - const context = document.createElement( 'div' ); - context.appendChild( nbspFillerInstance ); - - expect( isBlockFiller( nbspFillerInstance, 'nbsp' ) ).to.be.true; - // Check it twice to ensure that caching breaks nothing. - expect( isBlockFiller( nbspFillerInstance, 'nbsp' ) ).to.be.true; - } ); - - it( 'should return false if the nbsp filler is inside context', () => { - const nbspFillerInstance = NBSP_FILLER( document ); // eslint-disable-line new-cap - // NBSP must be check inside a context. - const context = document.createElement( 'div' ); - context.appendChild( nbspFillerInstance ); - // eslint-disable-next-line new-cap - context.appendChild( NBSP_FILLER( document ) ); - - expect( isBlockFiller( nbspFillerInstance, 'nbsp' ) ).to.be.false; - // Check it twice to ensure that caching breaks nothing. - expect( isBlockFiller( nbspFillerInstance, 'nbsp' ) ).to.be.false; - } ); - - it( 'should return false for inline filler', () => { - expect( isBlockFiller( document.createTextNode( INLINE_FILLER ), 'br' ) ).to.be.false; - expect( isBlockFiller( document.createTextNode( INLINE_FILLER ), 'nbsp' ) ).to.be.false; - } ); - } ); } ); diff --git a/tests/view/renderer.js b/tests/view/renderer.js index ef1ab3138..16065e2db 100644 --- a/tests/view/renderer.js +++ b/tests/view/renderer.js @@ -21,7 +21,7 @@ import DocumentFragment from '../../src/view/documentfragment'; import DowncastWriter from '../../src/view/downcastwriter'; import { parse, setData as setViewData, getData as getViewData } from '../../src/dev-utils/view'; -import { INLINE_FILLER, INLINE_FILLER_LENGTH, isBlockFiller, BR_FILLER } from '../../src/view/filler'; +import { BR_FILLER, INLINE_FILLER, INLINE_FILLER_LENGTH } from '../../src/view/filler'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import createViewRoot from './_utils/createroot'; import createElement from '@ckeditor/ckeditor5-utils/src/dom/createelement'; @@ -804,7 +804,7 @@ describe( 'Renderer', () => { // Step 3: Check whether in the first paragraph there's a
filler and that // in the second one there are two tags. expect( domP.childNodes.length ).to.equal( 1 ); - expect( isBlockFiller( domP.childNodes[ 0 ], 'br' ) ).to.be.true; + expect( domConverter.isBlockFiller( domP.childNodes[ 0 ] ) ).to.be.true; expect( domP2.childNodes.length ).to.equal( 2 ); expect( domP2.childNodes[ 0 ].tagName.toLowerCase() ).to.equal( 'b' ); @@ -886,7 +886,7 @@ describe( 'Renderer', () => { const domP = domRoot.childNodes[ 0 ]; expect( domP.childNodes.length ).to.equal( 1 ); - expect( isBlockFiller( domP.childNodes[ 0 ], 'br' ) ).to.be.true; + expect( domConverter.isBlockFiller( domP.childNodes[ 0 ] ) ).to.be.true; expect( domSelection.rangeCount ).to.equal( 1 ); expect( domSelection.getRangeAt( 0 ).startContainer ).to.equal( domP ); @@ -925,7 +925,7 @@ describe( 'Renderer', () => { const domP = domRoot.childNodes[ 0 ]; expect( domP.childNodes.length ).to.equal( 1 ); - expect( isBlockFiller( domP.childNodes[ 0 ], 'br' ) ).to.be.true; + expect( domConverter.isBlockFiller( domP.childNodes[ 0 ] ) ).to.be.true; expect( domSelection.rangeCount ).to.equal( 1 ); expect( domSelection.getRangeAt( 0 ).startContainer ).to.equal( domP ); diff --git a/tests/view/view/view.js b/tests/view/view/view.js index d21fa184c..b26a98391 100644 --- a/tests/view/view/view.js +++ b/tests/view/view/view.js @@ -18,7 +18,6 @@ import ViewRange from '../../../src/view/range'; import ViewElement from '../../../src/view/element'; import ViewPosition from '../../../src/view/position'; import ViewSelection from '../../../src/view/selection'; -import { isBlockFiller } from '../../../src/view/filler'; import count from '@ckeditor/ckeditor5-utils/src/count'; import global from '@ckeditor/ckeditor5-utils/src/dom/global'; @@ -565,7 +564,7 @@ describe( 'view', () => { view.forceRender(); expect( domDiv.childNodes.length ).to.equal( 1 ); - expect( isBlockFiller( domDiv.childNodes[ 0 ], 'br' ) ).to.be.true; + expect( view.domConverter.isBlockFiller( domDiv.childNodes[ 0 ] ) ).to.be.true; view.destroy(); domDiv.remove(); From fa8ccb6d484d1c7ed05e4cff4b15a2192c8f373f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Wed, 18 Sep 2019 08:17:57 +0200 Subject: [PATCH 25/25] Cleanup. --- src/view/domconverter.js | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/src/view/domconverter.js b/src/view/domconverter.js index 34fac2cdf..bd908d84d 100644 --- a/src/view/domconverter.js +++ b/src/view/domconverter.js @@ -7,7 +7,7 @@ * @module engine/view/domconverter */ -/* globals window, document, Node, NodeFilter, Text */ +/* globals document, Node, NodeFilter, Text */ import ViewText from './text'; import ViewElement from './element'; @@ -26,7 +26,7 @@ import isText from '@ckeditor/ckeditor5-utils/src/dom/istext'; import { isElement } from 'lodash-es'; // eslint-disable-next-line new-cap -const BR_FILLER_REF = BR_FILLER( window.document ); +const BR_FILLER_REF = BR_FILLER( document ); /** * DomConverter is a set of tools to do transformations between DOM nodes and view nodes. It also handles @@ -48,16 +48,6 @@ export default class DomConverter { * @param {module:engine/view/filler~BlockFillerMode} [options.blockFillerMode='br'] The type of the block filler to use. */ constructor( options = {} ) { - // Using WeakMap prevent memory leaks: when the converter will be destroyed all referenced between View and DOM - // will be removed. Also because it is a *Weak*Map when both view and DOM elements will be removed referenced - // will be also removed, isn't it brilliant? - // - // Yes, PJ. It is. - // - // You guys so smart. - // - // I've been here. Seen stuff. Afraid of code now. - /** * The mode of a block filler used by DOM converter. * @@ -67,7 +57,7 @@ export default class DomConverter { this.blockFillerMode = options.blockFillerMode || 'br'; /** - * Tag names of DOM `Element`s which are considered pre-formatted elements. + * Elements which are considered pre-formatted elements. * * @readonly * @member {Array.} module:engine/view/domconverter~DomConverter#preElements @@ -75,12 +65,17 @@ export default class DomConverter { this.preElements = [ 'pre' ]; /** - * Tag names of DOM `Element`s which are considered block elements. + * Elements which are considered block elements (and hence should be filled with a + * {@link ~isBlockFiller block filler}). + * + * Whether an element is considered a block element also affects handling of trailing whitespaces. + * + * You can extend this array if you introduce support for block elements which are not yet recognized here. * * @readonly * @member {Array.} module:engine/view/domconverter~DomConverter#blockElements */ - this.blockElements = [ 'p', 'div', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6' ]; + this.blockElements = [ 'p', 'div', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'li', 'dd', 'dt', 'figcaption' ]; /** * Block {@link module:engine/view/filler filler} creator, which is used to create all block fillers during the