Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed ugly crashes when playing with List style feature #8075

Merged
merged 4 commits into from
Sep 17, 2020
Merged

Fixed ugly crashes when playing with List style feature #8075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
49dc52b
Fixed ugly crashes when playing with List style feature.
pomek Sep 14, 2020
3b28132
Fixed invalid undo stack after merging lists.
pomek Sep 15, 2020
4ba5a5c
Merge remote-tracking branch 'origin/master' into i/8026
mlewand Sep 15, 2020
c6bba37
Docs (link): Minor word adjustment in code docs.
mlewand Sep 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 32 additions & 12 deletions packages/ckeditor5-list/src/liststyleediting.js
View file
Open in desktop
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
Open in desktop
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', () => {
mlewand marked this conversation as resolved.
Show resolved Hide resolved
// ○ 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