Skip to content

Commit

Permalink
Merge pull request #16636 from ckeditor/ck/16450
Browse files Browse the repository at this point in the history 
Fix (list): Text nodes directly inside `<ul>` or `<ol>` should not be removed while loading editor data. Closes #16450.
  • Loading branch information
@niegowski
niegowski authored Jul 3, 2024
2 parents 54dc8ef + da0f00e commit 09b31af
Show file tree
Hide file tree
Showing 4 changed files with 129 additions and 71 deletions.
23 changes: 0 additions & 23 deletions packages/ckeditor5-list/src/list/converters.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,29 +120,6 @@ export function listItemUpcastConverter(): GetCallback<UpcastElementEvent> {
};
}

/**
* Returns the upcast converter for the `<ul>` and `<ol>` view elements that cleans the input view of garbage.
* This is mostly to clean whitespaces from between the `<li>` view elements inside the view list element. However,
* incorrect data can also be cleared if the view was incorrect.
*
* @internal
*/
export function listUpcastCleanList(): GetCallback<UpcastElementEvent> {
return ( evt, data, conversionApi ) => {
if ( !conversionApi.consumable.test( data.viewItem, { name: true } ) ) {
return;
}

const viewWriter = new UpcastWriter( data.viewItem.document );

for ( const child of Array.from( data.viewItem.getChildren() ) ) {
if ( !isListItemView( child ) && !isListView( child ) ) {
viewWriter.remove( child );
}
}
};
}

/**
* Returns a model document change:data event listener that triggers conversion of related items if needed.
*
Expand Down
3 changes: 0 additions & 3 deletions packages/ckeditor5-list/src/list/listediting.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ import {
listItemDowncastConverter,
listItemDowncastRemoveConverter,
listItemUpcastConverter,
listUpcastCleanList,
reconvertItemsOnDataChange
} from './converters.js';
import {
Expand Down Expand Up @@ -456,8 +455,6 @@ export default class ListEditing extends Plugin {
} )
.add( dispatcher => {
dispatcher.on<UpcastElementEvent>( 'element:li', listItemUpcastConverter() );
dispatcher.on<UpcastElementEvent>( 'element:ul', listUpcastCleanList(), { priority: 'high' } );
dispatcher.on<UpcastElementEvent>( 'element:ol', listUpcastCleanList(), { priority: 'high' } );
} );

if ( !multiBlock ) {
Expand Down
107 changes: 84 additions & 23 deletions packages/ckeditor5-list/tests/list/converters-data-single-block.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,8 @@ describe( 'ListEditing (multiBlock=false) - converters - data pipeline', () => {
);
} );

it( 'clears incorrect elements', () => {
// See https://github.com/ckeditor/ckeditor5/issues/16450.
it( 'does not clear incorrect nodes within <ul>', () => {
test.data(
'<ul>' +
'x' +
Expand All @@ -249,18 +250,70 @@ describe( 'ListEditing (multiBlock=false) - converters - data pipeline', () => {
'</ul>' +
'<p>c</p>',

'<paragraph>x</paragraph>' +
'<listItem listIndent="0" listItemId="a00" listType="bulleted">a</listItem>' +
'<listItem listIndent="0" listItemId="a01" listType="bulleted">b</listItem>' +
'<paragraph>xxx</paragraph>' +
'<paragraph>x</paragraph>' +
'<paragraph>c</paragraph>',

'<p>x</p>' +
'<ul>' +
'<li>a</li>' +
'<li>b</li>' +
'</ul>' +
'<p>xxx</p>' +
'<p>x</p>' +
'<p>c</p>'
);
} );

// See https://github.com/ckeditor/ckeditor5/issues/16450.
it( 'does not clear incorrect nodes and elements within <ul>', () => {
test.data(
'<p>0</p>' +
'<ul>' +
'<strong>x</strong>y' +
'<li>a</li>' +
'<p>z</p>' +
'<li>b</li>' +
'<p>z1</p>' +
'<p>z2</p>' +
'<li>c</li>' +
'x' +
'</ul>' +
'<p>1</p>',

'<paragraph>0</paragraph>' +
'<paragraph><$text bold="true">x</$text>y</paragraph>' +
'<listItem listIndent="0" listItemId="a00" listType="bulleted">a</listItem>' +
'<paragraph>z</paragraph>' +
'<listItem listIndent="0" listItemId="a01" listType="bulleted">b</listItem>' +
'<paragraph>z1</paragraph>' +
'<paragraph>z2</paragraph>' +
'<listItem listIndent="0" listItemId="a02" listType="bulleted">c</listItem>' +
'<paragraph>x</paragraph>' +
'<paragraph>1</paragraph>',

'<p>0</p>' +
'<p><strong>x</strong>y</p>' +
'<ul>' +
'<li>a</li>' +
'</ul>' +
'<p>z</p>' +
'<ul>' +
'<li>b</li>' +
'</ul>' +
'<p>z1</p>' +
'<p>z2</p>' +
'<ul>' +
'<li>c</li>' +
'</ul>' +
'<p>x</p>' +
'<p>1</p>'
);
} );

it( 'clears whitespaces', () => {
test.data(
'<p>foo</p>' +
Expand Down Expand Up @@ -1540,6 +1593,7 @@ describe( 'ListEditing (multiBlock=false) - converters - data pipeline', () => {
);
} );

// See https://github.com/ckeditor/ckeditor5/issues/16450.
it( 'mixed lists deep structure, white spaces, incorrect content, empty items', () => {
test.data(
'<p>foo</p>' +
Expand Down Expand Up @@ -1586,29 +1640,33 @@ describe( 'ListEditing (multiBlock=false) - converters - data pipeline', () => {
'<p>bar</p>',

'<paragraph>foo</paragraph>' +
'<listItem listIndent="0" listItemId="a07" listType="bulleted">1</listItem>' +
'<paragraph>xxx</paragraph>' +
'<listItem listIndent="0" listItemId="a07" listType="bulleted">1xxx</listItem>' +
'<listItem listIndent="1" listItemId="a04" listType="bulleted"></listItem>' +
'<listItem listIndent="2" listItemId="a00" listType="bulleted"></listItem>' +
'<listItem listIndent="2" listItemId="a01" listType="bulleted">1.1.2</listItem>' +
'<listItem listIndent="2" listItemId="a02" listType="numbered">1.1.3</listItem>' +
'<listItem listIndent="2" listItemId="a03" listType="numbered">1.1.4</listItem>' +
'<listItem listIndent="1" listItemId="a06" listType="bulleted"></listItem>' +
'<listItem listIndent="2" listItemId="a05" listType="bulleted">1.2.1</listItem>' +
'<listItem listIndent="0" listItemId="a08" listType="bulleted">2</listItem>' +
'<listItem listIndent="0" listItemId="a10" listType="bulleted"></listItem>' +
'<listItem listIndent="1" listItemId="a0d" listType="numbered">3<$text bold="true">.</$text>1</listItem>' +
'<listItem listIndent="2" listItemId="a0b" listType="bulleted">3.1.1</listItem>' +
'<listItem listIndent="3" listItemId="a09" listType="numbered">3.1.1.1</listItem>' +
'<listItem listIndent="3" listItemId="a0a" listType="bulleted">3.1.1.2</listItem>' +
'<listItem listIndent="2" listItemId="a0c" listType="bulleted">3.1.2</listItem>' +
'<listItem listIndent="0" listItemId="a11" listType="bulleted">xxx</listItem>' +
'<listItem listIndent="0" listItemId="a08" listType="bulleted">2</listItem>' +
'<paragraph>xxx</paragraph>' +
'<listItem listIndent="0" listItemId="a0d" listType="numbered">3<$text bold="true">.</$text>1</listItem>' +
'<listItem listIndent="1" listItemId="a0b" listType="bulleted">3.1.1</listItem>' +
'<listItem listIndent="2" listItemId="a09" listType="numbered">3.1.1.1</listItem>' +
'<listItem listIndent="2" listItemId="a0a" listType="bulleted">3.1.1.2</listItem>' +
'<listItem listIndent="1" listItemId="a0c" listType="bulleted">3.1.2</listItem>' +
'<listItem listIndent="0" listItemId="a10" listType="bulleted">xxx</listItem>' +
'<listItem listIndent="1" listItemId="a0e" listType="bulleted">3.2</listItem>' +
'<listItem listIndent="1" listItemId="a0f" listType="bulleted">3.3</listItem>' +
'<paragraph>xxx</paragraph>' +
'<paragraph>bar</paragraph>',

'<p>foo</p>' +
'<p>xxx</p>' +
'<ul>' +
'<li>1' +
'<li>1xxx' +
'<ul>' +
'<li>&nbsp;' +
'<ul>' +
Expand All @@ -1627,31 +1685,34 @@ describe( 'ListEditing (multiBlock=false) - converters - data pipeline', () => {
'</li>' +
'</ul>' +
'</li>' +
'<li>xxx</li>' +
'<li>2</li>' +
'<li>&nbsp;' +
'<ol>' +
'<li>3<strong>.</strong>1' +
'</ul>' +
'<p>xxx</p>' +
'<ol>' +
'<li>3<strong>.</strong>1' +
'<ul>' +
'<li>3.1.1' +
'<ol>' +
'<li>3.1.1.1</li>' +
'</ol>' +
'<ul>' +
'<li>3.1.1' +
'<ol>' +
'<li>3.1.1.1</li>' +
'</ol>' +
'<ul>' +
'<li>3.1.1.2</li>' +
'</ul>' +
'</li>' +
'<li>3.1.2</li>' +
'<li>3.1.1.2</li>' +
'</ul>' +
'</li>' +
'</ol>' +
'<li>3.1.2</li>' +
'</ul>' +
'</li>' +
'</ol>' +
'<ul>' +
'<li>xxx' +
'<ul>' +
'<li>3.2</li>' +
'<li>3.3</li>' +
'</ul>' +
'</li>' +
'</ul>' +
'<p>xxx</p>' +
'<p>bar</p>'
);
} );
Expand Down
67 changes: 45 additions & 22 deletions packages/ckeditor5-list/tests/list/converters-data.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,8 @@ describe( 'ListEditing - converters - data pipeline', () => {
);
} );

it( 'clears incorrect elements', () => {
// See https://github.com/ckeditor/ckeditor5/issues/16450.
it( 'does not clear incorrect elements (text inside ul/ol should not be removed)', () => {
test.data(
'<ul>' +
'x' +
Expand All @@ -260,14 +261,20 @@ describe( 'ListEditing - converters - data pipeline', () => {
'</ul>' +
'<p>c</p>',

'<paragraph>x</paragraph>' +
'<paragraph listIndent="0" listItemId="a00" listType="bulleted">a</paragraph>' +
'<paragraph listIndent="0" listItemId="a01" listType="bulleted">b</paragraph>' +
'<paragraph>xxx</paragraph>' +
'<paragraph>x</paragraph>' +
'<paragraph>c</paragraph>',

'<p>x</p>' +
'<ul>' +
'<li>a</li>' +
'<li>b</li>' +
'</ul>' +
'<p>xxx</p>' +
'<p>x</p>' +
'<p>c</p>'
);
} );
Expand Down Expand Up @@ -2076,6 +2083,7 @@ describe( 'ListEditing - converters - data pipeline', () => {
);
} );

// See https://github.com/ckeditor/ckeditor5/issues/16450.
it( 'mixed lists deep structure, white spaces, incorrect content, empty items', () => {
test.data(
'<p>foo</p>' +
Expand Down Expand Up @@ -2122,64 +2130,79 @@ describe( 'ListEditing - converters - data pipeline', () => {
'<p>bar</p>',

'<paragraph>foo</paragraph>' +
'<paragraph listIndent="0" listItemId="a07" listType="bulleted">1</paragraph>' +
'<paragraph>xxx</paragraph>' +
'<paragraph listIndent="0" listItemId="a07" listType="bulleted">1xxx</paragraph>' +
'<paragraph listIndent="1" listItemId="a04" listType="bulleted"></paragraph>' +
'<paragraph listIndent="2" listItemId="a00" listType="bulleted"></paragraph>' +
'<paragraph listIndent="2" listItemId="a01" listType="bulleted">1.1.2</paragraph>' +
'<paragraph listIndent="2" listItemId="a02" listType="numbered">1.1.3</paragraph>' +
'<paragraph listIndent="2" listItemId="a03" listType="numbered">1.1.4</paragraph>' +
'<paragraph listIndent="1" listItemId="a06" listType="bulleted"></paragraph>' +
'<paragraph listIndent="2" listItemId="a05" listType="bulleted">1.2.1</paragraph>' +
'<paragraph listIndent="0" listItemId="a07" listType="bulleted">xxx</paragraph>' +
'<paragraph listIndent="0" listItemId="a08" listType="bulleted">2</paragraph>' +
'<paragraph listIndent="0" listItemId="a10" listType="bulleted"></paragraph>' +
'<paragraph listIndent="1" listItemId="a0d" listType="numbered">' +
'3<$text bold="true">.</$text>1' +
'</paragraph>' +
'<paragraph listIndent="0" listItemId="a10" listType="bulleted">xxx</paragraph>' +
'<paragraph listIndent="1" listItemId="a0d" listType="numbered">3<$text bold="true">.</$text>1</paragraph>' +
'<paragraph listIndent="2" listItemId="a0b" listType="bulleted">3.1.1</paragraph>' +
'<paragraph listIndent="3" listItemId="a09" listType="numbered">3.1.1.1</paragraph>' +
'<paragraph listIndent="3" listItemId="a0a" listType="bulleted">3.1.1.2</paragraph>' +
'<paragraph listIndent="2" listItemId="a0c" listType="bulleted">3.1.2</paragraph>' +
'<paragraph listIndent="0" listItemId="a10" listType="bulleted">xxx</paragraph>' +
'<paragraph listIndent="1" listItemId="a0e" listType="bulleted">3.2</paragraph>' +
'<paragraph listIndent="1" listItemId="a0f" listType="bulleted">3.3</paragraph>' +
'<paragraph>xxx</paragraph>' +
'<paragraph>bar</paragraph>',

'<p>foo</p>' +
'<p>xxx</p>' +
'<ul>' +
'<li>' +
'1' +
'<p>1xxx</p>' +
'<ul>' +
'<li>' +
'&nbsp;' +
'<ul><li>&nbsp;</li><li>1.1.2</li></ul>' +
'<ol><li>1.1.3</li><li>1.1.4</li></ol>' +
'<li>&nbsp;' +
'<ul>' +
'<li>&nbsp;</li>' +
'<li>1.1.2</li>' +
'</ul>' +
'<ol>' +
'<li>1.1.3</li>' +
'<li>1.1.4</li>' +
'</ol>' +
'</li>' +
'<li>' +
'&nbsp;' +
'<ul><li>1.2.1</li></ul>' +
'<li>&nbsp;' +
'<ul>' +
'<li>1.2.1</li>' +
'</ul>' +
'</li>' +
'</ul>' +
'<p>xxx</p>' +
'</li>' +
'<li>2</li>' +
'<li>' +
'<p>&nbsp;</p>' +
'<p>xxx</p>' +
'<ol>' +
'<li>' +
'3<strong>.</strong>1' +
'<li>3<strong>.</strong>1' +
'<ul>' +
'<li>' +
'3.1.1' +
'<ol><li>3.1.1.1</li></ol>' +
'<ul><li>3.1.1.2</li></ul>' +
'<li>3.1.1' +
'<ol>' +
'<li>3.1.1.1</li>' +
'</ol>' +
'<ul>' +
'<li>3.1.1.2</li>' +
'</ul>' +
'</li>' +
'<li>3.1.2</li>' +
'</ul>' +
'</li>' +
'</ol>' +
'<p>xxx</p>' +
'<ul><li>3.2</li><li>3.3</li></ul>' +
'<ul>' +
'<li>3.2</li>' +
'<li>3.3</li>' +
'</ul>' +
'</li>' +
'</ul>' +
'<p>xxx</p>' +
'<p>bar</p>'
);
} );
Expand Down

0 comments on commit 09b31af

Please sign in to comment.