From d0b7dc133a6e389ca74a5b2241e3b6c53c646f89 Mon Sep 17 00:00:00 2001 From: Kamil Piechaczek Date: Mon, 7 Sep 2020 09:54:53 +0200 Subject: [PATCH 1/5] Fix: LiThe lt style attributes hould be inherited for the same kind of the list. --- .../ckeditor5-list/src/liststyleediting.js | 8 ++++-- .../ckeditor5-list/tests/liststyleediting.js | 27 ++++++++++++++++--- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/packages/ckeditor5-list/src/liststyleediting.js b/packages/ckeditor5-list/src/liststyleediting.js index 8aef08f014f..eff407f8cde 100644 --- a/packages/ckeditor5-list/src/liststyleediting.js +++ b/packages/ckeditor5-list/src/liststyleediting.js @@ -474,7 +474,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 ); @@ -494,7 +494,7 @@ function fixListStyleAttributeOnListItemElements( editor ) { // // @param {module:engine/model/element~Element|null} baseItem // @returns {Boolean} - function shouldInheritListType( baseItem ) { + function shouldInheritListType( baseItem, itemToChange ) { if ( !baseItem ) { return false; } @@ -509,6 +509,10 @@ function fixListStyleAttributeOnListItemElements( editor ) { return false; } + if ( baseItem.getAttribute( 'listType' ) !== itemToChange.getAttribute( 'listType' ) ) { + return false; + } + return true; } } diff --git a/packages/ckeditor5-list/tests/liststyleediting.js b/packages/ckeditor5-list/tests/liststyleediting.js index a0b73695286..ec31752976a 100644 --- a/packages/ckeditor5-list/tests/liststyleediting.js +++ b/packages/ckeditor5-list/tests/liststyleediting.js @@ -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, 'Foo Bar.[]' + - 'Foo' + - 'Bar' + 'Foo' + + 'Bar' ); editor.execute( 'bulletedList' ); expect( getModelData( model ) ).to.equal( 'Foo Bar.[]' + - 'Foo' + - 'Bar' + 'Foo' + + 'Bar' ); } ); + it( + 'should not inherit the list style attribute when merging different kind of lists (from bottom, merge a single item)', + () => { + setModelData( model, + 'Foo' + + 'Bar' + + 'Foo Bar.[]' + ); + + editor.execute( 'bulletedList' ); + + expect( getModelData( model ) ).to.equal( + 'Foo' + + 'Bar' + + 'Foo Bar.[]' + ); + } + ); + it( 'should inherit the list style attribute when merging the same kind of lists (from bottom, merge a single item)', () => { setModelData( model, 'Foo' + From 76014e5a3570e0f3ab87bad0e5e961f0671f0980 Mon Sep 17 00:00:00 2001 From: Kamil Piechaczek Date: Mon, 7 Sep 2020 10:15:40 +0200 Subject: [PATCH 2/5] Fix: The list style attribute will be inherited when switching the "listType" attribue for existing the "listItem" element. --- .../ckeditor5-list/src/liststyleediting.js | 73 +++++++++++-------- .../ckeditor5-list/tests/liststyleediting.js | 69 ++++++++++++++++++ 2 files changed, 110 insertions(+), 32 deletions(-) diff --git a/packages/ckeditor5-list/src/liststyleediting.js b/packages/ckeditor5-list/src/liststyleediting.js index eff407f8cde..53fa2a82195 100644 --- a/packages/ckeditor5-list/src/liststyleediting.js +++ b/packages/ckeditor5-list/src/liststyleediting.js @@ -429,16 +429,12 @@ function fixListAfterOutdentListCommand( editor ) { 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; @@ -524,17 +520,11 @@ function fixListStyleAttributeOnListItemElements( 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; @@ -546,18 +536,6 @@ 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. @@ -577,3 +555,34 @@ function restoreDefaultListStyle( editor ) { } ); }; } + +// Returns `listItem` that were inserted or changed. +// +// @private +// @param {Array.} changes The changes list returned by the differ. +// @returns {Array.} +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; + } +} diff --git a/packages/ckeditor5-list/tests/liststyleediting.js b/packages/ckeditor5-list/tests/liststyleediting.js index ec31752976a..d14a05ca66e 100644 --- a/packages/ckeditor5-list/tests/liststyleediting.js +++ b/packages/ckeditor5-list/tests/liststyleediting.js @@ -522,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, + 'Foo Bar.[]' + + 'Foo' + + 'Bar' + ); + + editor.execute( 'bulletedList' ); + + expect( getModelData( model ) ).to.equal( + 'Foo Bar.[]' + + 'Foo' + + 'Bar' + ); + } ); + + it( 'should inherit the list style attribute when the modified list is the same kind of the list as previous sibling', () => { + setModelData( model, + 'Foo' + + 'Bar' + + 'Foo Bar.[]' + ); + + editor.execute( 'bulletedList' ); + + expect( getModelData( model ) ).to.equal( + 'Foo' + + 'Bar' + + 'Foo Bar.[]' + ); + } ); + + it( 'should not inherit the list style attribute when the modified list already has defined it (next sibling check)', () => { + setModelData( model, + 'Foo Bar.[]' + + 'Foo' + + 'Bar' + ); + + editor.execute( 'listStyle', { type: 'disc' } ); + + expect( getModelData( model ) ).to.equal( + 'Foo Bar.[]' + + 'Foo' + + 'Bar' + ); + } ); + + it( + 'should not inherit the list style attribute when the modified list already has defined it (previous sibling check)', + () => { + setModelData( model, + 'Foo' + + 'Bar' + + 'Foo Bar.[]' + ); + + editor.execute( 'listStyle', { type: 'disc' } ); + + expect( getModelData( model ) ).to.equal( + 'Foo' + + 'Bar' + + 'Foo Bar.[]' + ); + } + ); + } ); + describe( 'indenting lists', () => { it( 'should restore the default value for the list style attribute when indenting a single item', () => { setModelData( model, From 51cf5d2db426c01efbbebf21baf468f2a68a6ed5 Mon Sep 17 00:00:00 2001 From: Kamil Piechaczek Date: Mon, 7 Sep 2020 10:20:25 +0200 Subject: [PATCH 3/5] Added a missing param annotation. --- packages/ckeditor5-list/src/liststyleediting.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/ckeditor5-list/src/liststyleediting.js b/packages/ckeditor5-list/src/liststyleediting.js index 53fa2a82195..1f79e0705c1 100644 --- a/packages/ckeditor5-list/src/liststyleediting.js +++ b/packages/ckeditor5-list/src/liststyleediting.js @@ -489,6 +489,7 @@ 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, itemToChange ) { if ( !baseItem ) { From aa70315bd6dd6c59e0715ddf6af501c94c9ee26f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 9 Sep 2020 08:24:00 +0200 Subject: [PATCH 4/5] Remove private annotation from single-line comments as not parsed by JSDoc. --- packages/ckeditor5-list/src/liststyleediting.js | 8 -------- 1 file changed, 8 deletions(-) diff --git a/packages/ckeditor5-list/src/liststyleediting.js b/packages/ckeditor5-list/src/liststyleediting.js index 1f79e0705c1..a6391b3152a 100644 --- a/packages/ckeditor5-list/src/liststyleediting.js +++ b/packages/ckeditor5-list/src/liststyleediting.js @@ -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 => { @@ -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 => { @@ -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 ) { @@ -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 ) { @@ -423,7 +419,6 @@ function fixListAfterOutdentListCommand( editor ) { // ■ List item 2. // ... // ■ List item 3. // ... // -// @private // @param {module:core/editor/editor~Editor} editor // @returns {Function} function fixListStyleAttributeOnListItemElements( editor ) { @@ -516,7 +511,6 @@ function fixListStyleAttributeOnListItemElements( editor ) { // Removes the `listStyle` attribute from "todo" list items. // -// @private // @param {module:core/editor/editor~Editor} editor // @returns {Function} function removeListStyleAttributeFromTodoList( editor ) { @@ -541,7 +535,6 @@ function removeListStyleAttributeFromTodoList( editor ) { // Restores the `listStyle` attribute after changing the list type. // -// @private // @param {module:core/editor/editor~Editor} editor // @returns {Function} function restoreDefaultListStyle( editor ) { @@ -559,7 +552,6 @@ function restoreDefaultListStyle( editor ) { // Returns `listItem` that were inserted or changed. // -// @private // @param {Array.} changes The changes list returned by the differ. // @returns {Array.} function getChangedListItems( changes ) { From 819c3c7864305ab4cbf0a73f1e5da67dc8a3a290 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 9 Sep 2020 08:24:57 +0200 Subject: [PATCH 5/5] Remove internal function from the scope of the parent. --- .../ckeditor5-list/src/liststyleediting.js | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/packages/ckeditor5-list/src/liststyleediting.js b/packages/ckeditor5-list/src/liststyleediting.js index a6391b3152a..a4001296d09 100644 --- a/packages/ckeditor5-list/src/liststyleediting.js +++ b/packages/ckeditor5-list/src/liststyleediting.js @@ -566,16 +566,17 @@ function getChangedListItems( changes ) { } return items; +} - function getItemFromChange( change ) { - if ( change.type === 'attribute' ) { - return change.range.start.nodeAfter; - } - - if ( change.type === 'insert' ) { - return change.position.nodeAfter; - } +function getItemFromChange( change ) { + if ( change.type === 'attribute' ) { + return change.range.start.nodeAfter; + } - return null; + if ( change.type === 'insert' ) { + return change.position.nodeAfter; } + + return null; } +