Skip to content

Commit

Permalink
Merge pull request #9931 from ckeditor/ck/9801-block-elements
Browse files Browse the repository at this point in the history 
Fix (engine): DomConverter.blockElements is missing some of the HTML block element names. Closes #9801. Closes #7863.
  • Loading branch information
@maxbarnas
maxbarnas committed Jun 21, 2021
2 parents 2a670d4 + 86aa9f5 commit 67b4c7d
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 48 deletions.
2 changes: 0 additions & 2 deletions packages/ckeditor5-code-block/tests/codeblockediting.js
View file
Expand Up @@ -1135,10 +1135,8 @@ describe( 'CodeBlockEditing', () => {
editor.setData( `<pre><code>foo</code></pre>
<pre><code>bar</code></pre>` );

// Note: The empty <paragraph> in between should not be here. It's a conversion/auto–paragraphing bug.
expect( getModelData( model ) ).to.equal(
'<codeBlock language="plaintext">[]foo</codeBlock>' +
'<paragraph> </paragraph>' +
'<codeBlock language="plaintext">bar</codeBlock>' );
} );

Expand Down
9 changes: 8 additions & 1 deletion packages/ckeditor5-engine/src/view/domconverter.js
View file
Expand Up @@ -87,7 +87,12 @@ export default class DomConverter {
* @readonly
* @member {Array.<String>} module:engine/view/domconverter~DomConverter#blockElements
*/
this.blockElements = [ 'p', 'div', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'li', 'dd', 'dt', 'figcaption', 'td', 'th' ];
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.
Expand Down Expand Up @@ -1329,6 +1334,7 @@ function forEachDomNodeAncestor( node, callback ) {
// A &nbsp; is a block filler only if it is a single child of a block element.
//
// @param {Node} domNode DOM node.
// @param {Array.<String>} blockElements
// @returns {Boolean}
function isNbspBlockFiller( domNode, blockElements ) {
const isNBSP = domNode.isEqualNode( NBSP_FILLER_REF );
Expand All @@ -1339,6 +1345,7 @@ function isNbspBlockFiller( domNode, blockElements ) {
// Checks if domNode has block parent.
//
// @param {Node} domNode DOM node.
// @param {Array.<String>} blockElements
// @returns {Boolean}
function hasBlockParent( domNode, blockElements ) {
const parent = domNode.parentNode;
Expand Down
81 changes: 46 additions & 35 deletions packages/ckeditor5-engine/tests/view/domconverter/domconverter.js
View file
Expand Up @@ -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 <br> element', () => {
const context = document.createElement( elementName );
context.innerHTML = 'x<br>x';

expect( converter.isBlockFiller( context.childNodes[ 1 ] ) ).to.be.false;
} );

// SPECIAL CASE (see ckeditor5#5564).
it( 'should return true for a <br> element which is the only child of its block parent', () => {
const context = document.createElement( elementName );
context.innerHTML = '<br>';

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
Expand All @@ -349,21 +375,6 @@ describe( 'DomConverter', () => {
expect( converter.isBlockFiller( document.createTextNode( INLINE_FILLER ) ) ).to.be.false;
} );

it( 'should return false for a normal <br> element', () => {
const context = document.createElement( 'div' );
context.innerHTML = 'x<br>x';

expect( converter.isBlockFiller( context.childNodes[ 1 ] ) ).to.be.false;
} );

// SPECIAL CASE (see ckeditor5#5564).
it( 'should return true for a <br> element which is the only child of its block parent', () => {
const context = document.createElement( 'div' );
context.innerHTML = '<br>';

expect( converter.isBlockFiller( context.firstChild ) ).to.be.true;
} );

it( 'should return false for a <br> element which is the only child of its non-block parent', () => {
const context = document.createElement( 'span' );
context.innerHTML = '<br>';
Expand Down
14 changes: 4 additions & 10 deletions packages/ckeditor5-markdown-gfm/tests/gfmdataprocessor/blockquotes.js
View file
Expand Up @@ -115,8 +115,7 @@ describe( 'GFMDataProcessor', () => {
'<code>' +
'code 2' +
'</code>' +
// Space after `</pre>` might be due to bug in engine. See: https://github.com/ckeditor/ckeditor5/issues/7863.
'</pre> ' +
'</pre>' +
'</blockquote>',

'> Example 1:\n' +
Expand All @@ -129,9 +128,7 @@ describe( 'GFMDataProcessor', () => {
'>\n' +
'> ```\n' +
'> code 2\n' +
'> ```' +
// The below is an artefact of space after `</pre>`. See comment above & https://github.com/ckeditor/ckeditor5/issues/7863.
'\n>\n>'
'> ```'
);
} );

Expand Down Expand Up @@ -169,8 +166,7 @@ describe( 'GFMDataProcessor', () => {
'<code>' +
'code 2' +
'</code>' +
// Space after `</pre>` might be due to bug in engine. See: https://github.com/ckeditor/ckeditor5/issues/7863.
'</pre> ' +
'</pre>' +
'</blockquote>',

// When converting back to data, DataProcessor will normalize tabs to ```.
Expand All @@ -184,9 +180,7 @@ describe( 'GFMDataProcessor', () => {
'>\n' +
'> ```\n' +
'> code 2\n' +
'> ```' +
// The below is an artefact of space after `</pre>`. See comment above & https://github.com/ckeditor/ckeditor5/issues/7863.
'\n>\n>'
'> ```'
);
} );
} );
Expand Down

0 comments on commit 67b4c7d

Please sign in to comment.