From d6c2bf97421ad3b40e72aa47862d41f0c2f6292c Mon Sep 17 00:00:00 2001 From: Illia Sheremetov Date: Tue, 23 Jul 2024 18:34:23 +0200 Subject: [PATCH 1/7] Fix todo list check attribute data lost. --- packages/ckeditor5-list/src/list/converters.ts | 17 ++++++++++++++++- .../tests/todolist/todolistediting.js | 12 ++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/packages/ckeditor5-list/src/list/converters.ts b/packages/ckeditor5-list/src/list/converters.ts index 1922058f701..f36cc6c5a8a 100644 --- a/packages/ckeditor5-list/src/list/converters.ts +++ b/packages/ckeditor5-list/src/list/converters.ts @@ -81,6 +81,7 @@ export function listItemUpcastConverter(): GetCallback { const listItemId = ListItemUid.next(); const listIndent = getIndent( data.viewItem ); + const todoListChecked = isTodoListChecked( items ); let listType = data.viewItem.parent && data.viewItem.parent.is( 'element', 'ol' ) ? 'numbered' : 'bulleted'; // Preserve list type if was already set (for example by to-do list feature). @@ -93,7 +94,10 @@ export function listItemUpcastConverter(): GetCallback { const attributes = { listItemId, listIndent, - listType + listType, + // In some cases we need to set value of `todoListChecked` attribute in to-do lists. + // See: https://github.com/ckeditor/ckeditor5/issues/15602#top. + ...( ( listType === 'todo' ) && todoListChecked ? { todoListChecked } : null ) }; for ( const item of items ) { @@ -711,3 +715,14 @@ function shouldUseBogusParagraph( return blocks.length < 2; } + +// Returns `true` if at least one item have `todoListChecked` attribute set on `true`. +function isTodoListChecked( items: Array ) { + for ( let i = 0; i < items.length; i++ ) { + if ( items[ i ].getAttribute( 'todoListChecked' ) ) { + return true; + } + } + + return false; +} diff --git a/packages/ckeditor5-list/tests/todolist/todolistediting.js b/packages/ckeditor5-list/tests/todolist/todolistediting.js index adeca0815e4..e19550c64cc 100644 --- a/packages/ckeditor5-list/tests/todolist/todolistediting.js +++ b/packages/ckeditor5-list/tests/todolist/todolistediting.js @@ -203,6 +203,18 @@ describe( 'TodoListEditing', () => { ); } ); + it( 'should convert li with a checkbox and a paragraph ( when checked )', () => { + testUpcast( + '
    ' + + '
  • ' + + '' + + '

    foo

    ' + + '
    • ' + + '
', + 'foo' + ); + } ); + it( 'should convert li with a checkbox and two paragraphs', () => { testUpcast( '
    ' + From 3295d6bdc129d9d41a713eafd03537930db8d1d5 Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Mon, 29 Jul 2024 12:04:53 +0200 Subject: [PATCH 2/7] Move todo list items upcast fix to to do editing --- .../ckeditor5-list/src/list/converters.ts | 17 +---------- .../src/todolist/todolistediting.ts | 29 +++++++++++++++++++ 2 files changed, 30 insertions(+), 16 deletions(-) diff --git a/packages/ckeditor5-list/src/list/converters.ts b/packages/ckeditor5-list/src/list/converters.ts index f36cc6c5a8a..1922058f701 100644 --- a/packages/ckeditor5-list/src/list/converters.ts +++ b/packages/ckeditor5-list/src/list/converters.ts @@ -81,7 +81,6 @@ export function listItemUpcastConverter(): GetCallback { const listItemId = ListItemUid.next(); const listIndent = getIndent( data.viewItem ); - const todoListChecked = isTodoListChecked( items ); let listType = data.viewItem.parent && data.viewItem.parent.is( 'element', 'ol' ) ? 'numbered' : 'bulleted'; // Preserve list type if was already set (for example by to-do list feature). @@ -94,10 +93,7 @@ export function listItemUpcastConverter(): GetCallback { const attributes = { listItemId, listIndent, - listType, - // In some cases we need to set value of `todoListChecked` attribute in to-do lists. - // See: https://github.com/ckeditor/ckeditor5/issues/15602#top. - ...( ( listType === 'todo' ) && todoListChecked ? { todoListChecked } : null ) + listType }; for ( const item of items ) { @@ -715,14 +711,3 @@ function shouldUseBogusParagraph( return blocks.length < 2; } - -// Returns `true` if at least one item have `todoListChecked` attribute set on `true`. -function isTodoListChecked( items: Array ) { - for ( let i = 0; i < items.length; i++ ) { - if ( items[ i ].getAttribute( 'todoListChecked' ) ) { - return true; - } - } - - return false; -} diff --git a/packages/ckeditor5-list/src/todolist/todolistediting.ts b/packages/ckeditor5-list/src/todolist/todolistediting.ts index 6acac47c42b..1f8e088409f 100644 --- a/packages/ckeditor5-list/src/todolist/todolistediting.ts +++ b/packages/ckeditor5-list/src/todolist/todolistediting.ts @@ -108,6 +108,12 @@ export default class TodoListEditing extends Plugin { dispatcher.on( 'element:span', elementUpcastConsumingConverter( { name: 'span', classes: 'todo-list__label__description' } ) ); + + // Priority is set to low to allow generic list item converter to run first. + dispatcher.on( 'element:li', todoListItemUpcastConverter(), { + priority: 'low' + } ); + dispatcher.on( 'element:ul', attributeUpcastConsumingConverter( { name: 'ul', classes: 'todo-list' } ) ); @@ -389,6 +395,29 @@ export default class TodoListEditing extends Plugin { } } +/** + * Returns an upcast converter for to-do list items. + */ +function todoListItemUpcastConverter(): GetCallback { + return ( evt, data, conversionApi ) => { + const { writer } = conversionApi; + + if ( data.modelRange ) { + const items = Array + .from( data.modelRange.getItems( { shallow: true } ) ) + .filter( ( item ): item is Element => item.getAttribute( 'listType' ) === 'todo' ); + + // In some cases we need to set value of `todoListChecked` attribute in to-do lists. + // See: https://github.com/ckeditor/ckeditor5/issues/15602#top. + if ( items.some( item => item.getAttribute( 'todoListChecked' ) ) ) { + for ( const item of items ) { + writer.setAttribute( 'todoListChecked', true, item ); + } + } + } + }; +} + /** * Returns an upcast converter that detects a to-do list checkbox and marks the list item as a to-do list. */ From 8f7e4aea0371ec2d1a1be917ac51f035ccf4d852 Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Tue, 30 Jul 2024 11:03:41 +0200 Subject: [PATCH 3/7] Fix typos --- .../src/todolist/todolistediting.ts | 32 ++++++++++--------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/packages/ckeditor5-list/src/todolist/todolistediting.ts b/packages/ckeditor5-list/src/todolist/todolistediting.ts index 1f8e088409f..4a65cd53383 100644 --- a/packages/ckeditor5-list/src/todolist/todolistediting.ts +++ b/packages/ckeditor5-list/src/todolist/todolistediting.ts @@ -98,6 +98,11 @@ export default class TodoListEditing extends Plugin { // Upcast of to-do list item is based on a checkbox at the beginning of a
  • to keep compatibility with markdown input. dispatcher.on( 'element:input', todoItemInputConverter() ); + // Priority is set to low to allow generic list item converter to run first. + dispatcher.on( 'element:li', todoListItemUpcastConverter(), { + priority: 'low' + } ); + // Consume other elements that are normally generated in data downcast, so they won't get captured by GHS. dispatcher.on( 'element:label', elementUpcastConsumingConverter( { name: 'label', classes: 'todo-list__label' } @@ -109,11 +114,6 @@ export default class TodoListEditing extends Plugin { { name: 'span', classes: 'todo-list__label__description' } ) ); - // Priority is set to low to allow generic list item converter to run first. - dispatcher.on( 'element:li', todoListItemUpcastConverter(), { - priority: 'low' - } ); - dispatcher.on( 'element:ul', attributeUpcastConsumingConverter( { name: 'ul', classes: 'todo-list' } ) ); @@ -402,17 +402,19 @@ function todoListItemUpcastConverter(): GetCallback { return ( evt, data, conversionApi ) => { const { writer } = conversionApi; - if ( data.modelRange ) { - const items = Array - .from( data.modelRange.getItems( { shallow: true } ) ) - .filter( ( item ): item is Element => item.getAttribute( 'listType' ) === 'todo' ); + if ( !data.modelRange ) { + return; + } - // In some cases we need to set value of `todoListChecked` attribute in to-do lists. - // See: https://github.com/ckeditor/ckeditor5/issues/15602#top. - if ( items.some( item => item.getAttribute( 'todoListChecked' ) ) ) { - for ( const item of items ) { - writer.setAttribute( 'todoListChecked', true, item ); - } + const items = Array + .from( data.modelRange.getItems( { shallow: true } ) ) + .filter( ( item ): item is Element => item.getAttribute( 'listType' ) === 'todo' ); + + // In some cases we need to set value of `todoListChecked` attribute in to-do lists. + // See: https://github.com/ckeditor/ckeditor5/issues/15602. + if ( items.some( item => item.getAttribute( 'todoListChecked' ) ) ) { + for ( const item of items ) { + writer.setAttribute( 'todoListChecked', true, item ); } } }; From aecde6f4b486dc05a73faf55c9d62aa581cab18b Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Tue, 30 Jul 2024 12:25:20 +0200 Subject: [PATCH 4/7] Fix wrong deserialization of nested checked flag --- .../src/todolist/todolistediting.ts | 38 +++++++++++++------ .../tests/todolist/todolistediting.js | 35 +++++++++++++++++ 2 files changed, 62 insertions(+), 11 deletions(-) diff --git a/packages/ckeditor5-list/src/todolist/todolistediting.ts b/packages/ckeditor5-list/src/todolist/todolistediting.ts index 4a65cd53383..77adcad74da 100644 --- a/packages/ckeditor5-list/src/todolist/todolistediting.ts +++ b/packages/ckeditor5-list/src/todolist/todolistediting.ts @@ -11,7 +11,6 @@ import { Matcher, type UpcastElementEvent, type Model, - type Element, type MatcherPattern, type ViewElement, type ViewDocumentKeyDownEvent, @@ -19,7 +18,8 @@ import { type MapperViewToModelPositionEvent, type ViewDocumentFragment, type SelectionChangeRangeEvent, - type DocumentFragment + type DocumentFragment, + type Element } from 'ckeditor5/src/engine.js'; import { @@ -400,21 +400,37 @@ export default class TodoListEditing extends Plugin { */ function todoListItemUpcastConverter(): GetCallback { return ( evt, data, conversionApi ) => { - const { writer } = conversionApi; + const { writer, schema } = conversionApi; if ( !data.modelRange ) { return; } - const items = Array + // Group to-do list items by their listItemId attribute to ensure that all items of the same list have the same checked state. + const groupedItems = Array .from( data.modelRange.getItems( { shallow: true } ) ) - .filter( ( item ): item is Element => item.getAttribute( 'listType' ) === 'todo' ); - - // In some cases we need to set value of `todoListChecked` attribute in to-do lists. - // See: https://github.com/ckeditor/ckeditor5/issues/15602. - if ( items.some( item => item.getAttribute( 'todoListChecked' ) ) ) { - for ( const item of items ) { - writer.setAttribute( 'todoListChecked', true, item ); + .filter( ( item ): item is Element => + item.getAttribute( 'listType' ) === 'todo' && schema.checkAttribute( item, 'listItemId' ) + ) + .reduce( ( acc, item ) => { + const listItemId = item.getAttribute( 'listItemId' ) as string; + const items = acc.get( listItemId ) || []; + + items.push( item ); + acc.set( listItemId, items ); + + return acc; + }, new Map>() ); + + // During the upcast, we need to ensure that all items of the same list have the same checked state. From time to time + // the checked state of the items can be different when the user pastes content from the clipboard with + // that has checked state set to true. In such cases, we need to ensure that all items of the same list have the same checked state. + // See more: https://github.com/ckeditor/ckeditor5/issues/15602 + for ( const [ , items ] of groupedItems.entries() ) { + if ( items.some( item => item.getAttribute( 'todoListChecked' ) ) ) { + for ( const item of items ) { + writer.setAttribute( 'todoListChecked', true, item ); + } } } }; diff --git a/packages/ckeditor5-list/tests/todolist/todolistediting.js b/packages/ckeditor5-list/tests/todolist/todolistediting.js index e19550c64cc..5083be60f8f 100644 --- a/packages/ckeditor5-list/tests/todolist/todolistediting.js +++ b/packages/ckeditor5-list/tests/todolist/todolistediting.js @@ -215,6 +215,41 @@ describe( 'TodoListEditing', () => { ); } ); + it( 'should convert nested li with a checkbox and a paragraph ( when checked )', () => { + testUpcast( + '
      ' + + '
    • ' + + '' + + '
        ' + + '
      • ' + + '' + + '

        foo

        ' + + '
        • ' + + '
      ' + + '
      • ' + + '
    ', + '' + + 'foo' + ); + } ); + + it( 'should not convert nested li if it was already consumed', () => { + editor.data.upcastDispatcher.on( 'element:li', ( evt, data, conversionApi ) => { + conversionApi.consumable.consume( data.viewItem, { name: true } ); + }, { priority: 'highest' } ); + + editor.setData( + '
      ' + + '
    • ' + + '' + + '

      foo

      ' + + '
      • ' + + '
    ' + ); + + expect( getModelData( model, { withoutSelection: true } ) ).to.equal( '' ); + } ); + it( 'should convert li with a checkbox and two paragraphs', () => { testUpcast( '
      ' + From ece2adedd53d9ffa4570e1b1f5c8179c8a6c9e8a Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Tue, 30 Jul 2024 12:34:42 +0200 Subject: [PATCH 5/7] Adjust doc --- packages/ckeditor5-list/src/todolist/todolistediting.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ckeditor5-list/src/todolist/todolistediting.ts b/packages/ckeditor5-list/src/todolist/todolistediting.ts index 77adcad74da..15d9cd6fd80 100644 --- a/packages/ckeditor5-list/src/todolist/todolistediting.ts +++ b/packages/ckeditor5-list/src/todolist/todolistediting.ts @@ -406,7 +406,7 @@ function todoListItemUpcastConverter(): GetCallback { return; } - // Group to-do list items by their listItemId attribute to ensure that all items of the same list have the same checked state. + // Group to-do list items by their listItemId attribute to ensure that all items of the same list item have the same checked state. const groupedItems = Array .from( data.modelRange.getItems( { shallow: true } ) ) .filter( ( item ): item is Element => From ae81fc1f120e8c152d106c7629ab356302f83c6f Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Mon, 5 Aug 2024 07:46:08 +0200 Subject: [PATCH 6/7] Use `getAllListItemBlocks` to pick siblings of list item --- .../ckeditor5-list/src/todolist/todolistediting.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/ckeditor5-list/src/todolist/todolistediting.ts b/packages/ckeditor5-list/src/todolist/todolistediting.ts index 15d9cd6fd80..11aff51cffb 100644 --- a/packages/ckeditor5-list/src/todolist/todolistediting.ts +++ b/packages/ckeditor5-list/src/todolist/todolistediting.ts @@ -32,7 +32,7 @@ import { import { Plugin } from 'ckeditor5/src/core.js'; -import { isFirstBlockOfListItem, isListItemBlock } from '../list/utils/model.js'; +import { getAllListItemBlocks, isFirstBlockOfListItem, isListItemBlock } from '../list/utils/model.js'; import ListEditing, { type ListEditingCheckElementEvent, type ListEditingPostFixerEvent @@ -414,10 +414,10 @@ function todoListItemUpcastConverter(): GetCallback { ) .reduce( ( acc, item ) => { const listItemId = item.getAttribute( 'listItemId' ) as string; - const items = acc.get( listItemId ) || []; - items.push( item ); - acc.set( listItemId, items ); + if ( !acc.has( listItemId ) ) { + acc.set( listItemId, getAllListItemBlocks( item ) ); + } return acc; }, new Map>() ); @@ -427,7 +427,7 @@ function todoListItemUpcastConverter(): GetCallback { // that has checked state set to true. In such cases, we need to ensure that all items of the same list have the same checked state. // See more: https://github.com/ckeditor/ckeditor5/issues/15602 for ( const [ , items ] of groupedItems.entries() ) { - if ( items.some( item => item.getAttribute( 'todoListChecked' ) ) ) { + if ( [ ...items ].some( item => item.getAttribute( 'todoListChecked' ) ) ) { for ( const item of items ) { writer.setAttribute( 'todoListChecked', true, item ); } From 39dd8632df8fda662eb35d6d2141ebb4048384b7 Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Mon, 5 Aug 2024 08:01:07 +0200 Subject: [PATCH 7/7] Fix unnecessary cloning array --- packages/ckeditor5-list/src/todolist/todolistediting.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ckeditor5-list/src/todolist/todolistediting.ts b/packages/ckeditor5-list/src/todolist/todolistediting.ts index 11aff51cffb..f981453a0c8 100644 --- a/packages/ckeditor5-list/src/todolist/todolistediting.ts +++ b/packages/ckeditor5-list/src/todolist/todolistediting.ts @@ -427,7 +427,7 @@ function todoListItemUpcastConverter(): GetCallback { // that has checked state set to true. In such cases, we need to ensure that all items of the same list have the same checked state. // See more: https://github.com/ckeditor/ckeditor5/issues/15602 for ( const [ , items ] of groupedItems.entries() ) { - if ( [ ...items ].some( item => item.getAttribute( 'todoListChecked' ) ) ) { + if ( items.some( item => item.getAttribute( 'todoListChecked' ) ) ) { for ( const item of items ) { writer.setAttribute( 'todoListChecked', true, item ); }