Skip to content

Commit

Permalink
Merge pull request #8037 from ckeditor/i/7932
Browse files Browse the repository at this point in the history 
Fix (list): The `listStyle` attribute should be inherited when inserting or replacing a `listItem` with the same kind of the list (the `listType` attribute for the inserted/modified item is equal to next/previous sibling list). Closes #7932.
  • Loading branch information
@jodator
jodator authored Sep 9, 2020
2 parents db9d1eb + 819c3c7 commit 03bf721
Show file tree
Hide file tree
Showing 2 changed files with 140 additions and 45 deletions.
89 changes: 48 additions & 41 deletions packages/ckeditor5-list/src/liststyleediting.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,6 @@ export default class ListStyleEditing extends Plugin {
// Returns a converter that consumes the `style` attribute and search for `list-style-type` definition.
// If not found, the `"default"` value will be used.
//
// @private
// @returns {Function}
function upcastListItemStyle() {
return dispatcher => {
Expand All @@ -206,7 +205,6 @@ function upcastListItemStyle() {
// Returns a converter that adds the `list-style-type` definition as a value for the `style` attribute.
// The `"default"` value is removed and not present in the view/data.
//
// @private
// @returns {Function}
function downcastListStyleAttribute() {
return dispatcher => {
Expand Down Expand Up @@ -271,7 +269,6 @@ function downcastListStyleAttribute() {
// ○ List item 2.[]
// ■ List item 3.
//
// @private
// @param {module:core/editor/editor~Editor} editor
// @returns {Function}
function fixListAfterIndentListCommand( editor ) {
Expand Down Expand Up @@ -323,7 +320,6 @@ function fixListAfterIndentListCommand( editor ) {
// ■ List item 2.[]
// ■ List item 3.
//
// @private
// @param {module:core/editor/editor~Editor} editor
// @returns {Function}
function fixListAfterOutdentListCommand( editor ) {
Expand Down Expand Up @@ -423,22 +419,17 @@ function fixListAfterOutdentListCommand( editor ) {
// ■ List item 2. // ...
// ■ List item 3. // ...
//
// @private
// @param {module:core/editor/editor~Editor} editor
// @returns {Function}
function fixListStyleAttributeOnListItemElements( editor ) {
return writer => {
let wasFixed = false;
let insertedListItems = [];

for ( const change of editor.model.document.differ.getChanges() ) {
if ( change.type == 'insert' && change.name == 'listItem' ) {
insertedListItems.push( change.position.nodeAfter );
}
}

// Don't touch todo lists.
insertedListItems = insertedListItems.filter( item => item.getAttribute( 'listType' ) !== 'todo' );
const insertedListItems = getChangedListItems( editor.model.document.differ.getChanges() )
.filter( item => {
// Don't touch todo lists. They are handled in another post-fixer.
return item.getAttribute( 'listType' ) !== 'todo';
} );

if ( !insertedListItems.length ) {
return wasFixed;
Expand Down Expand Up @@ -474,7 +465,7 @@ function fixListStyleAttributeOnListItemElements( editor ) {

for ( const item of insertedListItems ) {
if ( !item.hasAttribute( 'listStyle' ) ) {
if ( shouldInheritListType( existingListItem ) ) {
if ( shouldInheritListType( existingListItem, item ) ) {
writer.setAttribute( 'listStyle', existingListItem.getAttribute( 'listStyle' ), item );
} else {
writer.setAttribute( 'listStyle', DEFAULT_LIST_TYPE, item );
Expand All @@ -493,8 +484,9 @@ function fixListStyleAttributeOnListItemElements( editor ) {
// the value for the element is other than default in the base element.
//
// @param {module:engine/model/element~Element|null} baseItem
// @param {module:engine/model/element~Element} itemToChange
// @returns {Boolean}
function shouldInheritListType( baseItem ) {
function shouldInheritListType( baseItem, itemToChange ) {
if ( !baseItem ) {
return false;
}
Expand All @@ -509,28 +501,25 @@ function fixListStyleAttributeOnListItemElements( editor ) {
return false;
}

if ( baseItem.getAttribute( 'listType' ) !== itemToChange.getAttribute( 'listType' ) ) {
return false;
}

return true;
}
}

// Removes the `listStyle` attribute from "todo" list items.
//
// @private
// @param {module:core/editor/editor~Editor} editor
// @returns {Function}
function removeListStyleAttributeFromTodoList( editor ) {
return writer => {
let todoListItems = [];

for ( const change of editor.model.document.differ.getChanges() ) {
const item = getItemFromChange( change );

if ( item && item.is( 'element', 'listItem' ) && item.getAttribute( 'listType' ) === 'todo' ) {
todoListItems.push( item );
}
}

todoListItems = todoListItems.filter( item => item.hasAttribute( 'listStyle' ) );
const todoListItems = getChangedListItems( editor.model.document.differ.getChanges() )
.filter( item => {
// Handle the todo lists only. The rest is handled in another post-fixer.
return item.getAttribute( 'listType' ) === 'todo' && item.hasAttribute( 'listStyle' );
} );

if ( !todoListItems.length ) {
return false;
Expand All @@ -542,23 +531,10 @@ function removeListStyleAttributeFromTodoList( editor ) {

return true;
};

function getItemFromChange( change ) {
if ( change.type === 'attribute' ) {
return change.range.start.nodeAfter;
}

if ( change.type === 'insert' ) {
return change.position.nodeAfter;
}

return null;
}
}

// Restores the `listStyle` attribute after changing the list type.
//
// @private
// @param {module:core/editor/editor~Editor} editor
// @returns {Function}
function restoreDefaultListStyle( editor ) {
Expand All @@ -573,3 +549,34 @@ function restoreDefaultListStyle( editor ) {
} );
};
}

// Returns `listItem` that were inserted or changed.
//
// @param {Array.<Object>} changes The changes list returned by the differ.
// @returns {Array.<module:engine/model/element~Element>}
function getChangedListItems( changes ) {
const items = [];

for ( const change of changes ) {
const item = getItemFromChange( change );

if ( item && item.is( 'element', 'listItem' ) ) {
items.push( item );
}
}

return items;
}

function getItemFromChange( change ) {
if ( change.type === 'attribute' ) {
return change.range.start.nodeAfter;
}

if ( change.type === 'insert' ) {
return change.position.nodeAfter;
}

return null;
}

96 changes: 92 additions & 4 deletions packages/ckeditor5-list/tests/liststyleediting.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -452,19 +452,38 @@ describe( 'ListStyleEditing', () => {
it( 'should not inherit the list style attribute when merging different kind of lists (from top, merge a single item)', () => {
setModelData( model,
'<paragraph>Foo Bar.[]</paragraph>' +
'<listItem listIndent="0" listStyle="default" listType="numbered">Foo</listItem>' +
'<listItem listIndent="0" listStyle="default" listType="numbered">Bar</listItem>'
'<listItem listIndent="0" listStyle="decimal-leading-zero" listType="numbered">Foo</listItem>' +
'<listItem listIndent="0" listStyle="decimal-leading-zero" listType="numbered">Bar</listItem>'
);

editor.execute( 'bulletedList' );

expect( getModelData( model ) ).to.equal(
'<listItem listIndent="0" listStyle="default" listType="bulleted">Foo Bar.[]</listItem>' +
'<listItem listIndent="0" listStyle="default" listType="numbered">Foo</listItem>' +
'<listItem listIndent="0" listStyle="default" listType="numbered">Bar</listItem>'
'<listItem listIndent="0" listStyle="decimal-leading-zero" listType="numbered">Foo</listItem>' +
'<listItem listIndent="0" listStyle="decimal-leading-zero" listType="numbered">Bar</listItem>'
);
} );

it(
'should not inherit the list style attribute when merging different kind of lists (from bottom, merge a single item)',
() => {
setModelData( model,
'<listItem listIndent="0" listStyle="decimal-leading-zero" listType="numbered">Foo</listItem>' +
'<listItem listIndent="0" listStyle="decimal-leading-zero" listType="numbered">Bar</listItem>' +
'<paragraph>Foo Bar.[]</paragraph>'
);

editor.execute( 'bulletedList' );

expect( getModelData( model ) ).to.equal(
'<listItem listIndent="0" listStyle="decimal-leading-zero" listType="numbered">Foo</listItem>' +
'<listItem listIndent="0" listStyle="decimal-leading-zero" listType="numbered">Bar</listItem>' +
'<listItem listIndent="0" listStyle="default" listType="bulleted">Foo Bar.[]</listItem>'
);
}
);

it( 'should inherit the list style attribute when merging the same kind of lists (from bottom, merge a single item)', () => {
setModelData( model,
'<listItem listIndent="0" listStyle="circle" listType="bulleted">Foo</listItem>' +
Expand Down Expand Up @@ -503,6 +522,75 @@ describe( 'ListStyleEditing', () => {
);
} );

describe( 'modifying "listType" attribute', () => {
it( 'should inherit the list style attribute when the modified list is the same kind of the list as next sibling', () => {
setModelData( model,
'<listItem listIndent="0" listStyle="default" listType="numbered">Foo Bar.[]</listItem>' +
'<listItem listIndent="0" listStyle="circle" listType="bulleted">Foo</listItem>' +
'<listItem listIndent="0" listStyle="circle" listType="bulleted">Bar</listItem>'
);

editor.execute( 'bulletedList' );

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

it( 'should inherit the list style attribute when the modified list is the same kind of the list as previous sibling', () => {
setModelData( model,
'<listItem listIndent="0" listStyle="circle" listType="bulleted">Foo</listItem>' +
'<listItem listIndent="0" listStyle="circle" listType="bulleted">Bar</listItem>' +
'<listItem listIndent="0" listStyle="default" listType="numbered">Foo Bar.[]</listItem>'
);

editor.execute( 'bulletedList' );

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

it( 'should not inherit the list style attribute when the modified list already has defined it (next sibling check)', () => {
setModelData( model,
'<listItem listIndent="0" listStyle="default" listType="bulleted">Foo Bar.[]</listItem>' +
'<listItem listIndent="0" listStyle="circle" listType="bulleted">Foo</listItem>' +
'<listItem listIndent="0" listStyle="circle" listType="bulleted">Bar</listItem>'
);

editor.execute( 'listStyle', { type: 'disc' } );

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

it(
'should not inherit the list style attribute when the modified list already has defined it (previous sibling check)',
() => {
setModelData( model,
'<listItem listIndent="0" listStyle="circle" listType="bulleted">Foo</listItem>' +
'<listItem listIndent="0" listStyle="circle" listType="bulleted">Bar</listItem>' +
'<listItem listIndent="0" listStyle="default" listType="bulleted">Foo Bar.[]</listItem>'
);

editor.execute( 'listStyle', { type: 'disc' } );

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

describe( 'indenting lists', () => {
it( 'should restore the default value for the list style attribute when indenting a single item', () => {
setModelData( model,
Expand Down

0 comments on commit 03bf721

Please sign in to comment.