Skip to content

Commit

Permalink
Merge pull request #8075 from ckeditor/i/8026
Browse files Browse the repository at this point in the history 
Fix (list): List styles plugin will no longer cause the editor to crash when indenting a list item that is the last element in the editor. Closes #8072.

Fix (list): Undo will restore the proper value of the list-style-type attribute in the View element after undoing merging different lists. Closes #7930.

Fix (list): Fixed a bug that prevented using the same value of the List style feature for nested lists. Closes #8081.

Internal (list): The mechanism that merges two lists and inherits their list styles will not crash while deleting content (lists with different values of the listIndent attribute) in a nested list. Closes #8073.

Internal (list): The listStyle attribute will be inherited when deleting nested lists. The previous implementation added in #7879 handled lists with the listIndent=0. See #8073.
  • Loading branch information
@mlewand
mlewand authored Sep 17, 2020
2 parents 501490a + c6bba37 commit 3e6ea99
Show file tree
Hide file tree
Showing 2 changed files with 216 additions and 16 deletions.
44 changes: 32 additions & 12 deletions packages/ckeditor5-list/src/liststyleediting.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,11 +141,29 @@ export default class ListStyleEditing extends Plugin {
return;
}

// Find the outermost list item based on the `listIndent` attribute. We can't assume that `listIndent=0`
// because the selection can be hooked in nested lists.
//
// <listItem listIndent="0" listType="bulleted" listStyle="square">UL List item 1</listItem>
// <listItem listIndent="1" listType="bulleted" listStyle="square">UL List [item 1.1</listItem>
// <listItem listIndent="0" listType="bulleted" listStyle="circle">[]UL List item 1.</listItem>
// <listItem listIndent="1" listType="bulleted" listStyle="circle">UL List ]item 1.1</listItem>
//
// After deleting the content, we would like to inherit the "square" attribute for the last element:
//
// <listItem listIndent="0" listType="bulleted" listStyle="square">UL List item 1</listItem>
// <listItem listIndent="1" listType="bulleted" listStyle="square">UL List []item 1.1</listItem>
const mostOuterItemList = getSiblingListItem( firstPosition.parent, {
sameIndent: true,
listIndent: 0
listIndent: nextSibling.getAttribute( 'listIndent' )
} );

// The outermost list item may not exist while removing elements between lists with different value
// of the `listIndent` attribute. In such a case we don't want to update anything. See: #8073.
if ( !mostOuterItemList ) {
return;
}

if ( mostOuterItemList.getAttribute( 'listType' ) === nextSibling.getAttribute( 'listType' ) ) {
firstMostOuterItem = mostOuterItemList;
}
Expand All @@ -167,7 +185,7 @@ export default class ListStyleEditing extends Plugin {
// <listItem listIndent="0" listType="bulleted" listStyle="circle">UL List item 2</listItem>
const secondListMostOuterItem = getSiblingListItem( firstMostOuterItem.nextSibling, {
sameIndent: true,
listIndent: 0,
listIndent: firstMostOuterItem.getAttribute( 'listIndent' ),
direction: 'forward'
} );

Expand Down Expand Up @@ -211,7 +229,6 @@ function downcastListStyleAttribute() {
dispatcher.on( 'attribute:listStyle:listItem', ( evt, data, conversionApi ) => {
const viewWriter = conversionApi.writer;
const currentElement = data.item;
const listStyle = data.attributeNewValue;

const previousElement = getSiblingListItem( currentElement.previousSibling, {
sameIndent: true,
Expand All @@ -221,25 +238,23 @@ function downcastListStyleAttribute() {

const viewItem = conversionApi.mapper.toViewElement( currentElement );

// Single item list.
if ( !previousElement ) {
setListStyle( viewWriter, listStyle, viewItem.parent );
} else if ( !areRepresentingSameList( previousElement, currentElement ) ) {
// A case when elements represent different lists. We need to separate their container.
if ( !areRepresentingSameList( currentElement, previousElement ) ) {
viewWriter.breakContainer( viewWriter.createPositionBefore( viewItem ) );
viewWriter.breakContainer( viewWriter.createPositionAfter( viewItem ) );

setListStyle( viewWriter, listStyle, viewItem.parent );
}

setListStyle( viewWriter, data.attributeNewValue, viewItem.parent );
}, { priority: 'low' } );
};

// Checks whether specified list items belong to the same list.
//
// @param {module:engine/model/element~Element} listItem1 The first list item to check.
// @param {module:engine/model/element~Element} listItem2 The second list item to check.
// @param {module:engine/model/element~Element|null} listItem2 The second list item to check.
// @returns {Boolean}
function areRepresentingSameList( listItem1, listItem2 ) {
return listItem1.getAttribute( 'listType' ) === listItem2.getAttribute( 'listType' ) &&
return listItem2 &&
listItem1.getAttribute( 'listType' ) === listItem2.getAttribute( 'listType' ) &&
listItem1.getAttribute( 'listIndent' ) === listItem2.getAttribute( 'listIndent' ) &&
listItem1.getAttribute( 'listStyle' ) === listItem2.getAttribute( 'listStyle' );
}
Expand Down Expand Up @@ -459,6 +474,11 @@ function fixListStyleAttributeOnListItemElements( editor ) {
// ■ Paragraph[] // <-- The inserted item.
while ( existingListItem.is( 'element', 'listItem' ) && existingListItem.getAttribute( 'listIndent' ) !== indent ) {
existingListItem = existingListItem.previousSibling;

// If the item does not exist, most probably there is no other content in the editor. See: #8072.
if ( !existingListItem ) {
break;
}
}
}
}
Expand Down
188 changes: 184 additions & 4 deletions packages/ckeditor5-list/tests/liststyleediting.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,41 @@ describe( 'ListStyleEditing', () => {
'</ul>'
);
} );

// See: #8081.
it( 'should convert properly nested list styles', () => {
// ■ Level 0
// ▶ Level 0.1
// ○ Level 0.1.1
// ▶ Level 0.2
// ○ Level 0.2.1
setModelData( model,
'<listItem listIndent="0" listType="bulleted" listStyle="default">Level 0</listItem>' +
'<listItem listIndent="1" listType="bulleted" listStyle="default">Level 0.1</listItem>' +
'<listItem listIndent="2" listType="bulleted" listStyle="circle">Level 0.1.1</listItem>' +
'<listItem listIndent="1" listType="bulleted" listStyle="default">Level 0.2</listItem>' +
'<listItem listIndent="2" listType="bulleted" listStyle="circle">Level 0.2.1</listItem>'
);

expect( getViewData( view, { withoutSelection: true } ) ).to.equal(
'<ul>' +
'<li>Level 0' +
'<ul>' +
'<li>Level 0.1' +
'<ul style="list-style-type:circle">' +
'<li>Level 0.1.1</li>' +
'</ul>' +
'</li>' +
'<li>Level 0.2' +
'<ul style="list-style-type:circle">' +
'<li>Level 0.2.1</li>' +
'</ul>' +
'</li>' +
'</ul>' +
'</li>' +
'</ul>'
);
} );
} );
} );

Expand Down Expand Up @@ -725,6 +760,21 @@ describe( 'ListStyleEditing', () => {
'<listItem listIndent="1" listStyle="decimal" listType="numbered">3.[]</listItem>'
);
} );

// See: #8072.
it( 'should not throw when indenting a list without any other content in the editor', () => {
setModelData( model,
'<listItem listIndent="0" listStyle="default" listType="bulleted">Foo</listItem>' +
'<listItem listIndent="0" listStyle="default" listType="bulleted">[]</listItem>'
);

editor.execute( 'indentList' );

expect( getModelData( model ) ).to.equal(
'<listItem listIndent="0" listStyle="default" listType="bulleted">Foo</listItem>' +
'<listItem listIndent="1" listStyle="default" listType="bulleted">[]</listItem>'
);
} );
} );

describe( 'outdenting lists', () => {
Expand Down Expand Up @@ -923,6 +973,97 @@ describe( 'ListStyleEditing', () => {
} );
} );

describe( 'delete + undo', () => {
let editor, model, view;

beforeEach( () => {
return VirtualTestEditor
.create( {
plugins: [ Paragraph, ListStyleEditing, Typing, UndoEditing ]
} )
.then( newEditor => {
editor = newEditor;
model = editor.model;
view = editor.editing.view;
} );
} );

afterEach( () => {
return editor.destroy();
} );

// See: #7930.
it( 'should restore proper list style attribute after undo merging lists', () => {
// ○ 1.
// ○ 2.
// ○ 3.
// <paragraph>
// ■ 1.
// ■ 2.
setModelData( model,
'<listItem listIndent="0" listStyle="circle" listType="bulleted">1.</listItem>' +
'<listItem listIndent="0" listStyle="circle" listType="bulleted">2.</listItem>' +
'<listItem listIndent="0" listStyle="circle" listType="bulleted">3.</listItem>' +
'<paragraph>[]</paragraph>' +
'<listItem listIndent="0" listStyle="square" listType="bulleted">1.</listItem>' +
'<listItem listIndent="0" listStyle="square" listType="bulleted">2.</listItem>'
);

expect( getViewData( view, { withoutSelection: true } ), 'initial data' ).to.equal(
'<ul style="list-style-type:circle">' +
'<li>1.</li>' +
'<li>2.</li>' +
'<li>3.</li>' +
'</ul>' +
'<p></p>' +
'<ul style="list-style-type:square">' +
'<li>1.</li>' +
'<li>2.</li>' +
'</ul>'
);

// After removing the paragraph.
// ○ 1.
// ○ 2.
// ○ 3.
// ○ 1.
// ○ 2.
editor.execute( 'delete' );

expect( getViewData( view, { withoutSelection: true } ), 'executing delete' ).to.equal(
'<ul style="list-style-type:circle">' +
'<li>1.</li>' +
'<li>2.</li>' +
'<li>3.</li>' +
'<li>1.</li>' +
'<li>2.</li>' +
'</ul>'
);

// After undo.
// ○ 1.
// ○ 2.
// ○ 3.
// <paragraph>
// ■ 1.
// ■ 2.
editor.execute( 'undo' );

expect( getViewData( view, { withoutSelection: true } ), 'initial data' ).to.equal(
'<ul style="list-style-type:circle">' +
'<li>1.</li>' +
'<li>2.</li>' +
'<li>3.</li>' +
'</ul>' +
'<p></p>' +
'<ul style="list-style-type:square">' +
'<li>1.</li>' +
'<li>2.</li>' +
'</ul>'
);
} );
} );

describe( 'todo list', () => {
let editor, model;

Expand Down Expand Up @@ -1096,8 +1237,8 @@ describe( 'ListStyleEditing', () => {
setModelData( model,
'<listItem listIndent="0" listStyle="square" listType="bulleted">1.</listItem>' +
'<listItem listIndent="0" listStyle="square" listType="bulleted">2.</listItem>' +
'<listItem listIndent="1" listStyle="numbered" listType="decimal">2.1.</listItem>' +
'<listItem listIndent="2" listStyle="default" listType="default">2.1.1</listItem>' +
'<listItem listIndent="1" listStyle="decimal" listType="numbered">2.1.</listItem>' +
'<listItem listIndent="2" listStyle="default" listType="numbered">2.1.1</listItem>' +
'<paragraph>[]</paragraph>' +
'<listItem listIndent="0" listStyle="circle" listType="bulleted">1.</listItem>' +
'<listItem listIndent="0" listStyle="circle" listType="bulleted">2.</listItem>'
Expand All @@ -1108,8 +1249,8 @@ describe( 'ListStyleEditing', () => {
expect( getModelData( model ) ).to.equal(
'<listItem listIndent="0" listStyle="square" listType="bulleted">1.</listItem>' +
'<listItem listIndent="0" listStyle="square" listType="bulleted">2.</listItem>' +
'<listItem listIndent="1" listStyle="numbered" listType="decimal">2.1.</listItem>' +
'<listItem listIndent="2" listStyle="default" listType="default">2.1.1[]</listItem>' +
'<listItem listIndent="1" listStyle="decimal" listType="numbered">2.1.</listItem>' +
'<listItem listIndent="2" listStyle="default" listType="numbered">2.1.1[]</listItem>' +
'<listItem listIndent="0" listStyle="square" listType="bulleted">1.</listItem>' +
'<listItem listIndent="0" listStyle="square" listType="bulleted">2.</listItem>'
);
Expand Down Expand Up @@ -1233,6 +1374,45 @@ describe( 'ListStyleEditing', () => {
);
} );

// See: #8073.
it( 'should not crash when removing a content between intended lists', () => {
setModelData( model,
'<listItem listIndent="0" listStyle="default" listType="bulleted">aaaa</listItem>' +
'<listItem listIndent="1" listStyle="default" listType="bulleted">bb[bb</listItem>' +
'<listItem listIndent="2" listStyle="default" listType="bulleted">cc]cc</listItem>' +
'<listItem listIndent="3" listStyle="default" listType="bulleted">dddd</listItem>'
);

editor.execute( 'delete' );

expect( getModelData( model ) ).to.equal(
'<listItem listIndent="0" listStyle="default" listType="bulleted">aaaa</listItem>' +
'<listItem listIndent="1" listStyle="default" listType="bulleted">bb[]cc</listItem>' +
'<listItem listIndent="2" listStyle="default" listType="bulleted">dddd</listItem>'
);
} );

it( 'should read the `listStyle` attribute from the most outer selected list while removing content between lists', () => {
setModelData( model,
'<listItem listIndent="0" listStyle="square" listType="bulleted">1.</listItem>' +
'<listItem listIndent="0" listStyle="square" listType="bulleted">2.</listItem>' +
'<listItem listIndent="1" listStyle="decimal" listType="numbered">2.1.</listItem>' +
'<listItem listIndent="2" listStyle="lower-latin" listType="numbered">2.1.1[foo</listItem>' +
'<paragraph>Foo</paragraph>' +
'<listItem listIndent="0" listStyle="circle" listType="bulleted">1.</listItem>' +
'<listItem listIndent="1" listStyle="circle" listType="bulleted">bar]2.</listItem>'
);

editor.execute( 'delete' );

expect( getModelData( model ) ).to.equal(
'<listItem listIndent="0" listStyle="square" listType="bulleted">1.</listItem>' +
'<listItem listIndent="0" listStyle="square" listType="bulleted">2.</listItem>' +
'<listItem listIndent="1" listStyle="decimal" listType="numbered">2.1.</listItem>' +
'<listItem listIndent="2" listStyle="lower-latin" listType="numbered">2.1.1[]2.</listItem>'
);
} );

function simulateTyping( text ) {
// While typing, every character is an atomic change.
text.split( '' ).forEach( character => {
Expand Down

0 comments on commit 3e6ea99

Please sign in to comment.