From ce0aad76d5148a9a96aa07054afd7e6716e06f36 Mon Sep 17 00:00:00 2001 From: Jacek Bogdanski Date: Mon, 21 Jun 2021 11:11:20 +0200 Subject: [PATCH 1/5] Expanded DomConverter.blockElements. --- .../ckeditor5-engine/src/view/domconverter.js | 17 +++- .../tests/view/domconverter/domconverter.js | 81 +++++++++++-------- 2 files changed, 60 insertions(+), 38 deletions(-) diff --git a/packages/ckeditor5-engine/src/view/domconverter.js b/packages/ckeditor5-engine/src/view/domconverter.js index 429134a4201..7debb7351f1 100644 --- a/packages/ckeditor5-engine/src/view/domconverter.js +++ b/packages/ckeditor5-engine/src/view/domconverter.js @@ -85,9 +85,14 @@ export default class DomConverter { * 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 + * @member {Set.} module:engine/view/domconverter~DomConverter#blockElements */ - this.blockElements = [ 'p', 'div', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'li', 'dd', 'dt', 'figcaption', 'td', 'th' ]; + this.blockElements = new Set( [ + 'address', 'article', 'aside', 'blockquote', 'caption', 'center', 'dd', 'details', 'dir', 'div', + 'dl', 'dt', 'fieldset', 'figcaption', 'figure', 'footer', 'form', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'header', + 'hgroup', 'legend', 'li', 'main', 'menu', 'nav', 'ol', 'p', 'pre', 'section', 'summary', 'table', 'tbody', + 'td', 'tfoot', 'th', 'thead', 'tr', 'ul' + ] ); /** * The DOM-to-view mapping. @@ -1299,12 +1304,14 @@ export default class DomConverter { // Used to check if given native `Element` or `Text` node has parent with tag name from `types` array. // // @param {Node} node -// @param {Array.} types +// @param {Iterable.} types // @param {Boolean} [boundaryParent] Can be given if parents should be checked up to a given element (excluding that element). // @returns {Boolean} `true` if such parent exists or `false` if it does not. function _hasDomParentOfType( node, types, boundaryParent ) { let parents = getAncestors( node ); + types = Array.from( types ); + if ( boundaryParent ) { parents = parents.slice( parents.indexOf( boundaryParent ) + 1 ); } @@ -1329,6 +1336,7 @@ function forEachDomNodeAncestor( node, callback ) { // A   is a block filler only if it is a single child of a block element. // // @param {Node} domNode DOM node. +// @param {Iterable.} blockElements // @returns {Boolean} function isNbspBlockFiller( domNode, blockElements ) { const isNBSP = domNode.isEqualNode( NBSP_FILLER_REF ); @@ -1339,10 +1347,13 @@ function isNbspBlockFiller( domNode, blockElements ) { // Checks if domNode has block parent. // // @param {Node} domNode DOM node. +// @param {Iterable.} blockElements // @returns {Boolean} function hasBlockParent( domNode, blockElements ) { const parent = domNode.parentNode; + blockElements = Array.from( blockElements ); + return parent && parent.tagName && blockElements.includes( parent.tagName.toLowerCase() ); } diff --git a/packages/ckeditor5-engine/tests/view/domconverter/domconverter.js b/packages/ckeditor5-engine/tests/view/domconverter/domconverter.js index c30f28cc18f..3e51a8080af 100644 --- a/packages/ckeditor5-engine/tests/view/domconverter/domconverter.js +++ b/packages/ckeditor5-engine/tests/view/domconverter/domconverter.js @@ -295,40 +295,66 @@ describe( 'DomConverter', () => { } ); describe( 'isBlockFiller()', () => { + const blockElements = new Set( [ + 'address', 'article', 'aside', 'blockquote', 'caption', 'center', 'dd', 'details', 'dir', 'div', + 'dl', 'dt', 'fieldset', 'figcaption', 'figure', 'footer', 'form', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'header', + 'hgroup', 'legend', 'li', 'main', 'menu', 'nav', 'ol', 'p', 'pre', 'section', 'summary', 'table', 'tbody', + 'td', 'tfoot', 'th', 'thead', 'tr', 'ul' + ] ); + for ( const mode of [ 'nbsp', 'markedNbsp' ] ) { describe( 'mode "' + mode + '"', () => { beforeEach( () => { converter = new DomConverter( viewDocument, { blockFillerMode: mode } ); } ); - it( 'should return true if the node is an nbsp filler and is a single child of a block level element', () => { - const nbspFillerInstance = NBSP_FILLER( document ); // eslint-disable-line new-cap + for ( const elementName of blockElements ) { + describe( `<${ elementName }> context`, () => { + it( 'should return true if the node is an nbsp filler and is a single child of a block level element', () => { + const nbspFillerInstance = NBSP_FILLER( document ); // eslint-disable-line new-cap - const context = document.createElement( 'div' ); - context.appendChild( nbspFillerInstance ); + const context = document.createElement( elementName ); + context.appendChild( nbspFillerInstance ); - expect( converter.isBlockFiller( nbspFillerInstance ) ).to.be.true; - } ); + expect( converter.isBlockFiller( nbspFillerInstance ) ).to.be.true; + } ); - it( 'should return false if the node is an nbsp filler and is not a single child of a block level element', () => { - const nbspFillerInstance = NBSP_FILLER( document ); // eslint-disable-line new-cap + it( 'should return false if the node is an nbsp filler and is not a single child of a block level element', () => { + const nbspFillerInstance = NBSP_FILLER( document ); // eslint-disable-line new-cap - const context = document.createElement( 'div' ); - context.appendChild( nbspFillerInstance ); - context.appendChild( document.createTextNode( 'a' ) ); + const context = document.createElement( elementName ); + context.appendChild( nbspFillerInstance ); + context.appendChild( document.createTextNode( 'a' ) ); - expect( converter.isBlockFiller( nbspFillerInstance ) ).to.be.false; - } ); + expect( converter.isBlockFiller( nbspFillerInstance ) ).to.be.false; + } ); - it( 'should return false if there are two nbsp fillers in a block element', () => { - const nbspFillerInstance = NBSP_FILLER( document ); // eslint-disable-line new-cap + it( 'should return false if there are two nbsp fillers in a block element', () => { + const nbspFillerInstance = NBSP_FILLER( document ); // eslint-disable-line new-cap - const context = document.createElement( 'div' ); - context.appendChild( nbspFillerInstance ); - context.appendChild( NBSP_FILLER( document ) ); // eslint-disable-line new-cap + const context = document.createElement( elementName ); + context.appendChild( nbspFillerInstance ); + context.appendChild( NBSP_FILLER( document ) ); // eslint-disable-line new-cap - expect( converter.isBlockFiller( nbspFillerInstance ) ).to.be.false; - } ); + expect( converter.isBlockFiller( nbspFillerInstance ) ).to.be.false; + } ); + + it( 'should return false for a normal
element', () => { + const context = document.createElement( elementName ); + context.innerHTML = 'x
x'; + + expect( converter.isBlockFiller( context.childNodes[ 1 ] ) ).to.be.false; + } ); + + // SPECIAL CASE (see ckeditor5#5564). + it( 'should return true for a
element which is the only child of its block parent', () => { + const context = document.createElement( elementName ); + context.innerHTML = '
'; + + expect( converter.isBlockFiller( context.firstChild ) ).to.be.true; + } ); + } ); + } it( 'should return false filler is placed in a non-block element', () => { const nbspFillerInstance = NBSP_FILLER( document ); // eslint-disable-line new-cap @@ -349,21 +375,6 @@ describe( 'DomConverter', () => { expect( converter.isBlockFiller( document.createTextNode( INLINE_FILLER ) ) ).to.be.false; } ); - it( 'should return false for a normal
element', () => { - const context = document.createElement( 'div' ); - context.innerHTML = 'x
x'; - - expect( converter.isBlockFiller( context.childNodes[ 1 ] ) ).to.be.false; - } ); - - // SPECIAL CASE (see ckeditor5#5564). - it( 'should return true for a
element which is the only child of its block parent', () => { - const context = document.createElement( 'div' ); - context.innerHTML = '
'; - - expect( converter.isBlockFiller( context.firstChild ) ).to.be.true; - } ); - it( 'should return false for a
element which is the only child of its non-block parent', () => { const context = document.createElement( 'span' ); context.innerHTML = '
'; From e2f3038f7d13b8069777428b665138f32a9fc939 Mon Sep 17 00:00:00 2001 From: Jacek Bogdanski Date: Mon, 21 Jun 2021 12:29:48 +0200 Subject: [PATCH 2/5] Corrected codeblock test. --- packages/ckeditor5-code-block/tests/codeblockediting.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/ckeditor5-code-block/tests/codeblockediting.js b/packages/ckeditor5-code-block/tests/codeblockediting.js index 4b8f94e0d92..0ecebfffa56 100644 --- a/packages/ckeditor5-code-block/tests/codeblockediting.js +++ b/packages/ckeditor5-code-block/tests/codeblockediting.js @@ -1135,10 +1135,8 @@ describe( 'CodeBlockEditing', () => { editor.setData( `
foo
bar
` ); - // Note: The empty in between should not be here. It's a conversion/auto–paragraphing bug. expect( getModelData( model ) ).to.equal( '[]foo' + - ' ' + 'bar' ); } ); From cd4756310a6f631a5be3bd84d9077a5855344ab7 Mon Sep 17 00:00:00 2001 From: Jacek Bogdanski Date: Mon, 21 Jun 2021 12:39:37 +0200 Subject: [PATCH 3/5] Corrected markdown-gfm tests. --- .../tests/gfmdataprocessor/blockquotes.js | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/packages/ckeditor5-markdown-gfm/tests/gfmdataprocessor/blockquotes.js b/packages/ckeditor5-markdown-gfm/tests/gfmdataprocessor/blockquotes.js index 6586e9a0d5a..9b7577fccea 100644 --- a/packages/ckeditor5-markdown-gfm/tests/gfmdataprocessor/blockquotes.js +++ b/packages/ckeditor5-markdown-gfm/tests/gfmdataprocessor/blockquotes.js @@ -115,8 +115,7 @@ describe( 'GFMDataProcessor', () => { '' + 'code 2' + '' + - // Space after `` might be due to bug in engine. See: https://github.com/ckeditor/ckeditor5/issues/7863. - ' ' + + '' + '', '> Example 1:\n' + @@ -129,9 +128,7 @@ describe( 'GFMDataProcessor', () => { '>\n' + '> ```\n' + '> code 2\n' + - '> ```' + - // The below is an artefact of space after ``. See comment above & https://github.com/ckeditor/ckeditor5/issues/7863. - '\n>\n>' + '> ```' ); } ); @@ -169,8 +166,7 @@ describe( 'GFMDataProcessor', () => { '' + 'code 2' + '' + - // Space after `` might be due to bug in engine. See: https://github.com/ckeditor/ckeditor5/issues/7863. - ' ' + + '' + '', // When converting back to data, DataProcessor will normalize tabs to ```. @@ -184,9 +180,7 @@ describe( 'GFMDataProcessor', () => { '>\n' + '> ```\n' + '> code 2\n' + - '> ```' + - // The below is an artefact of space after ``. See comment above & https://github.com/ckeditor/ckeditor5/issues/7863. - '\n>\n>' + '> ```' ); } ); } ); From 39fa38f6d6d15e0ac8acc6474797c1f61ef1e362 Mon Sep 17 00:00:00 2001 From: Jacek Bogdanski Date: Mon, 21 Jun 2021 12:41:41 +0200 Subject: [PATCH 4/5] Reverted type change. --- packages/ckeditor5-engine/src/view/domconverter.js | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/packages/ckeditor5-engine/src/view/domconverter.js b/packages/ckeditor5-engine/src/view/domconverter.js index 7debb7351f1..a91480f0ab6 100644 --- a/packages/ckeditor5-engine/src/view/domconverter.js +++ b/packages/ckeditor5-engine/src/view/domconverter.js @@ -85,14 +85,14 @@ export default class DomConverter { * You can extend this array if you introduce support for block elements which are not yet recognized here. * * @readonly - * @member {Set.} module:engine/view/domconverter~DomConverter#blockElements + * @member {Array.} module:engine/view/domconverter~DomConverter#blockElements */ - this.blockElements = new Set( [ + this.blockElements = [ 'address', 'article', 'aside', 'blockquote', 'caption', 'center', 'dd', 'details', 'dir', 'div', 'dl', 'dt', 'fieldset', 'figcaption', 'figure', 'footer', 'form', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'header', 'hgroup', 'legend', 'li', 'main', 'menu', 'nav', 'ol', 'p', 'pre', 'section', 'summary', 'table', 'tbody', 'td', 'tfoot', 'th', 'thead', 'tr', 'ul' - ] ); + ]; /** * The DOM-to-view mapping. @@ -1310,8 +1310,6 @@ export default class DomConverter { function _hasDomParentOfType( node, types, boundaryParent ) { let parents = getAncestors( node ); - types = Array.from( types ); - if ( boundaryParent ) { parents = parents.slice( parents.indexOf( boundaryParent ) + 1 ); } @@ -1336,7 +1334,7 @@ function forEachDomNodeAncestor( node, callback ) { // A   is a block filler only if it is a single child of a block element. // // @param {Node} domNode DOM node. -// @param {Iterable.} blockElements +// @param {Array.} blockElements // @returns {Boolean} function isNbspBlockFiller( domNode, blockElements ) { const isNBSP = domNode.isEqualNode( NBSP_FILLER_REF ); @@ -1347,13 +1345,11 @@ function isNbspBlockFiller( domNode, blockElements ) { // Checks if domNode has block parent. // // @param {Node} domNode DOM node. -// @param {Iterable.} blockElements +// @param {Array.} blockElements // @returns {Boolean} function hasBlockParent( domNode, blockElements ) { const parent = domNode.parentNode; - blockElements = Array.from( blockElements ); - return parent && parent.tagName && blockElements.includes( parent.tagName.toLowerCase() ); } From 86aa9f5e39841a88ef0903077101a48d3cab29e7 Mon Sep 17 00:00:00 2001 From: Jacek Bogdanski Date: Mon, 21 Jun 2021 12:43:00 +0200 Subject: [PATCH 5/5] Docs type revert. --- packages/ckeditor5-engine/src/view/domconverter.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ckeditor5-engine/src/view/domconverter.js b/packages/ckeditor5-engine/src/view/domconverter.js index a91480f0ab6..0dc618cd063 100644 --- a/packages/ckeditor5-engine/src/view/domconverter.js +++ b/packages/ckeditor5-engine/src/view/domconverter.js @@ -1304,7 +1304,7 @@ export default class DomConverter { // Used to check if given native `Element` or `Text` node has parent with tag name from `types` array. // // @param {Node} node -// @param {Iterable.} types +// @param {Array.} types // @param {Boolean} [boundaryParent] Can be given if parents should be checked up to a given element (excluding that element). // @returns {Boolean} `true` if such parent exists or `false` if it does not. function _hasDomParentOfType( node, types, boundaryParent ) {