From c894905cb69b71a7e24411b8c9df878be6b56a83 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Wed, 16 Feb 2022 19:47:11 +0100 Subject: [PATCH 01/25] PoC WiP DocumentLists + GHS. --- .../ckeditor5-html-support/src/datafilter.js | 5 + .../src/generalhtmlsupport.js | 4 +- .../src/integrations/documentlist.js | 238 ++++++++++++++++++ .../tests/manual/documentlist.html | 16 ++ .../tests/manual/documentlist.js | 60 +++++ .../tests/manual/documentlist.md | 0 .../src/documentlist/utils/view.js | 16 +- 7 files changed, 336 insertions(+), 3 deletions(-) create mode 100644 packages/ckeditor5-html-support/src/integrations/documentlist.js create mode 100644 packages/ckeditor5-html-support/tests/manual/documentlist.html create mode 100644 packages/ckeditor5-html-support/tests/manual/documentlist.js create mode 100644 packages/ckeditor5-html-support/tests/manual/documentlist.md diff --git a/packages/ckeditor5-html-support/src/datafilter.js b/packages/ckeditor5-html-support/src/datafilter.js index 566e13da919..e512c4d1ce3 100644 --- a/packages/ckeditor5-html-support/src/datafilter.js +++ b/packages/ckeditor5-html-support/src/datafilter.js @@ -359,6 +359,11 @@ export default class DataFilter extends Plugin { const { view: viewName, model: modelName } = definition; if ( !schema.isRegistered( definition.model ) ) { + if ( !definition.modelSchema ) { + // TODO is this temporary? disabling listItem for li + return; + } + schema.register( definition.model, definition.modelSchema ); if ( !viewName ) { diff --git a/packages/ckeditor5-html-support/src/generalhtmlsupport.js b/packages/ckeditor5-html-support/src/generalhtmlsupport.js index d6f02478db5..7de10367d74 100644 --- a/packages/ckeditor5-html-support/src/generalhtmlsupport.js +++ b/packages/ckeditor5-html-support/src/generalhtmlsupport.js @@ -18,6 +18,7 @@ import MediaEmbedElementSupport from './integrations/mediaembed'; import ScriptElementSupport from './integrations/script'; import TableElementSupport from './integrations/table'; import StyleElementSupport from './integrations/style'; +import DocumentListElementSupport from './integrations/documentlist'; /** * The General HTML Support feature. @@ -48,7 +49,8 @@ export default class GeneralHtmlSupport extends Plugin { MediaEmbedElementSupport, ScriptElementSupport, TableElementSupport, - StyleElementSupport + StyleElementSupport, + DocumentListElementSupport ]; } diff --git a/packages/ckeditor5-html-support/src/integrations/documentlist.js b/packages/ckeditor5-html-support/src/integrations/documentlist.js new file mode 100644 index 00000000000..1a318c100b5 --- /dev/null +++ b/packages/ckeditor5-html-support/src/integrations/documentlist.js @@ -0,0 +1,238 @@ +/** + * @license Copyright (c) 2003-2022, CKSource Holding sp. z o.o. All rights reserved. + * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license + */ + +/** + * @module html-support/integrations/documentlist + */ + +import { Plugin } from 'ckeditor5/src/core'; +import { setViewAttributes } from '../conversionutils.js'; + +import DataFilter from '../datafilter'; +import { LIST_BASE_ATTRIBUTES } from '@ckeditor/ckeditor5-list/src/documentlist/utils/model'; +import { findMappedViewElement } from '@ckeditor/ckeditor5-list/src/documentlist/converters'; +import { createListElement, createListItemElement, isListView } from '@ckeditor/ckeditor5-list/src/documentlist/utils/view'; +import ListWalker from '@ckeditor/ckeditor5-list/src/documentlist/utils/listwalker'; + +/** + * Provides the General HTML Support integration with {@link module:list/documentlist~DocumentList Document List} feature. + * + * @extends module:core/plugin~Plugin + */ +export default class DocumentListElementSupport extends Plugin { + /** + * @inheritDoc + */ + static get requires() { + return [ DataFilter ]; + } + + /** + * @inheritDoc + */ + init() { + if ( !this.editor.plugins.has( 'DocumentListEditing' ) ) { + return; + } + + const dataFilter = this.editor.plugins.get( DataFilter ); + + dataFilter.on( 'register:li', registerFilter( { + attributeName: 'htmlLiAttributes', + viewElementName: 'li', + + appliesToListItem() { + return true; + } + + }, this.editor ) ); + + dataFilter.on( 'register:ul', registerFilter( { + attributeName: 'htmlUlAttributes', + viewElementName: 'ul', + + appliesToListItem( item ) { + return item.getAttribute( 'listType' ) != 'numbered'; + } + }, this.editor ) ); + + dataFilter.on( 'register:ol', registerFilter( { + attributeName: 'htmlOlAttributes', + viewElementName: 'ol', + + appliesToListItem( item ) { + return item.getAttribute( 'listType' ) == 'numbered'; + } + }, this.editor ) ); + } +} + +// TODO +function registerFilter( strategy, editor ) { + const { attributeName } = strategy; + + const schema = editor.model.schema; + const conversion = editor.conversion; + const dataFilter = editor.plugins.get( DataFilter ); + + return evt => { + if ( schema.checkAttribute( '$block', attributeName ) ) { + return; + } + + // Extend codeBlock to allow attributes required by attribute filtration. + schema.extend( '$block', { allowAttributes: [ attributeName ] } ); + schema.extend( '$blockObject', { allowAttributes: [ attributeName ] } ); + schema.extend( '$container', { allowAttributes: [ attributeName ] } ); + + conversion.for( 'upcast' ).add( viewToModelListAttributeConverter( strategy, dataFilter ) ); + conversion.for( 'downcast' ).add( modelToViewListAttributeConverter( strategy, editor.model ) ); + + evt.stop(); + }; +} + +// View-to-model conversion helper preserving allowed attributes on {@link TODO} +// feature model element. +// +// Attributes are preserved as a value of `htmlLiAttributes` model attribute. +// +// @private +// @param {String} attributeName +// @param {String} viewElementName +// @param {module:html-support/datafilter~DataFilter} dataFilter +// @returns {Function} Returns a conversion callback. +function viewToModelListAttributeConverter( strategy, dataFilter ) { + const { attributeName, viewElementName } = strategy; + + return dispatcher => { + dispatcher.on( `element:${ viewElementName }`, ( evt, data, conversionApi ) => { + const viewElement = data.viewItem; + const viewAttributes = dataFilter._consumeAllowedAttributes( viewElement, conversionApi ); + + if ( !viewAttributes ) { + return; + } + + for ( const item of data.modelRange.getItems( { shallow: true } ) ) { + // Apply only to list item blocks. + if ( !item.hasAttribute( 'listItemId' ) ) { + continue; + } + + // Apply only for the related list type. + if ( !strategy.appliesToListItem( item ) ) { + continue; + } + + // Set list attributes only on same level items, those nested deeper are already handled + // by the recursive conversion. + if ( item.hasAttribute( attributeName ) ) { + continue; + } + + conversionApi.writer.setAttribute( attributeName, viewAttributes || {}, item ); + } + }, { priority: 'low' } ); + }; +} + +// Model-to-view conversion helper applying attributes from {@link module:code-block/codeblock~CodeBlock Code Block} +// feature model element. +// +// @private +// @param {String} attributeName +// @returns {Function} Returns a conversion callback. +function modelToViewListAttributeConverter( strategy, model ) { + const { attributeName } = strategy; + + return dispatcher => { + dispatcher.on( `attribute:${ attributeName }`, ( evt, data, conversionApi ) => { + const { writer, mapper, consumable } = conversionApi; + const listItem = data.item; + + // Check and consume only the list properties attributes (the base list attributes are already consumed + // but should also trigger conversion of list properties). + if ( !LIST_BASE_ATTRIBUTES.includes( data.attributeKey ) && !consumable.consume( listItem, evt.name ) ) { + return; + } + + // Use positions mapping instead of mapper.toViewElement( listItem ) to find outermost view element. + // This is for cases when mapping is using inner view element like in the code blocks (pre > code). + const viewElement = findMappedViewElement( listItem, mapper, model ); + + // Unwrap element from current list wrappers. + // unwrapListItemBlock( viewElement, strategy, writer ); + + // Then wrap them with the new list wrappers. + wrapListItemBlock( listItem, writer.createRangeOn( viewElement ), strategy, writer ); + } ); + }; +} + +// Unwraps all ol, ul, and li attribute elements that are wrapping the provided view element. +function unwrapListItemBlock( viewElement, strategy, writer ) { + let attributeElement = viewElement.parent; + + while ( attributeElement.is( 'attributeElement' ) && [ 'ul', 'ol', 'li' ].includes( attributeElement.name ) ) { + const parentElement = attributeElement.parent; + const attributeValue = strategy.getAttributeOnUpcast( attributeElement ); + + // Unwrap only if the model attribute is really downcasted (it's not the default value). + if ( isListView( attributeElement ) && attributeValue !== strategy.defaultValue ) { + // Make a clone of an attribute element that only includes properties of list styles. + const element = writer.createAttributeElement( attributeElement.name, null, { + priority: attributeElement.priority, + id: attributeElement.id + } ); + + strategy.setAttributeOnDowncast( writer, attributeValue, element ); + writer.unwrap( writer.createRangeOn( viewElement ), element ); + } + + attributeElement = parentElement; + } +} + +// Wraps the given list item with appropriate attribute elements for ul, ol, and li. +function wrapListItemBlock( listItem, viewRange, strategy, writer ) { + if ( !listItem.hasAttribute( 'listIndent' ) ) { + return; + } + + const listItemIndent = listItem.getAttribute( 'listIndent' ); + let listType = listItem.getAttribute( 'listType' ); + let listItemId = listItem.getAttribute( 'listItemId' ); + let htmlAttributes = listItem.getAttribute( strategy.attributeName ); + + let currentListItem = listItem; + + for ( let indent = listItemIndent; indent >= 0; indent-- ) { + if ( htmlAttributes && strategy.appliesToListItem( currentListItem ) ) { + const listViewElement = strategy.viewElementName == 'li' ? + createListItemElement( writer, indent, listItemId ) : + createListElement( writer, indent, listType ); + + setViewAttributes( writer, htmlAttributes, listViewElement ); + viewRange = writer.wrap( viewRange, listViewElement ); + } + + if ( indent == 0 ) { + break; + } + + currentListItem = ListWalker.first( currentListItem, { lowerIndent: true } ); + + // There is no list item with lower indent, this means this is a document fragment containing + // only a part of nested list (like copy to clipboard) so we don't need to try to wrap it further. + if ( !currentListItem ) { + break; + } + + listType = currentListItem.getAttribute( 'listType' ); + listItemId = currentListItem.getAttribute( 'listItemId' ); + htmlAttributes = currentListItem.getAttribute( strategy.attributeName ); + } +} diff --git a/packages/ckeditor5-html-support/tests/manual/documentlist.html b/packages/ckeditor5-html-support/tests/manual/documentlist.html new file mode 100644 index 00000000000..07f4453bce3 --- /dev/null +++ b/packages/ckeditor5-html-support/tests/manual/documentlist.html @@ -0,0 +1,16 @@ + + + + +
+ +
diff --git a/packages/ckeditor5-html-support/tests/manual/documentlist.js b/packages/ckeditor5-html-support/tests/manual/documentlist.js new file mode 100644 index 00000000000..d43a557bb8f --- /dev/null +++ b/packages/ckeditor5-html-support/tests/manual/documentlist.js @@ -0,0 +1,60 @@ +/** + * @license Copyright (c) 2003-2022, CKSource Holding sp. z o.o. All rights reserved. + * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license + */ + +/* globals console:false, window, document */ + +import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor'; +import Essentials from '@ckeditor/ckeditor5-essentials/src/essentials'; +import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; +import Bold from '@ckeditor/ckeditor5-basic-styles/src/bold'; +import Italic from '@ckeditor/ckeditor5-basic-styles/src/italic'; +import Strikethrough from '@ckeditor/ckeditor5-basic-styles/src/strikethrough'; +import DocumentListProperties from '@ckeditor/ckeditor5-list/src/documentlistproperties'; +import SourceEditing from '@ckeditor/ckeditor5-source-editing/src/sourceediting'; +import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; + +import GeneralHtmlSupport from '../../src/generalhtmlsupport'; + +/** + * Client custom plugin extending HTML support for compatibility. + */ +class ExtendHTMLSupport extends Plugin { + static get requires() { + return [ GeneralHtmlSupport ]; + } + + init() { + const dataFilter = this.editor.plugins.get( 'DataFilter' ); + + dataFilter.allowElement( /^p$/ ); + dataFilter.allowAttributes( { name: /^p$/, attributes: true } ); + + dataFilter.allowElement( /^(ul|ol)$/ ); + dataFilter.allowAttributes( { name: /^(ul|ol)$/, attributes: true } ); + dataFilter.allowElement( /^(li)$/ ); + dataFilter.allowAttributes( { name: /^(li)$/, attributes: true } ); + } +} + +ClassicEditor + .create( document.querySelector( '#editor' ), { + plugins: [ + Bold, + DocumentListProperties, + Essentials, + ExtendHTMLSupport, + Italic, + Paragraph, + Strikethrough, + SourceEditing + ], + toolbar: [ 'sourceEditing', '|', 'bold', 'italic', 'strikethrough' ] + } ) + .then( editor => { + window.editor = editor; + } ) + .catch( err => { + console.error( err.stack ); + } ); diff --git a/packages/ckeditor5-html-support/tests/manual/documentlist.md b/packages/ckeditor5-html-support/tests/manual/documentlist.md new file mode 100644 index 00000000000..e69de29bb2d diff --git a/packages/ckeditor5-list/src/documentlist/utils/view.js b/packages/ckeditor5-list/src/documentlist/utils/view.js index b996477d746..31a0d3f90f8 100644 --- a/packages/ckeditor5-list/src/documentlist/utils/view.js +++ b/packages/ckeditor5-list/src/documentlist/utils/view.js @@ -117,10 +117,22 @@ export function createListElement( writer, indent, type ) { */ export function createListItemElement( writer, indent, id ) { // Negative priorities so that restricted editing attribute won't wrap list items. - return writer.createAttributeElement( 'li', null, { + const viewElement = writer.createAttributeElement( 'li', null, { priority: ( 2 * indent + 1 ) / 100 - 100, - id + // id } ); + + writer.setCustomProperty( 'listItemId', id, viewElement ); + + viewElement.isSimilar = function( otherElement ) { + if ( this.getCustomProperty( 'listItemId' ) == otherElement.getCustomProperty( 'listItemId' ) ) { + return true; + } + + return Object.getPrototypeOf( viewElement ).isSimilar.call( viewElement, otherElement ); + }; + + return viewElement; } /** From 3ace1fe37f62baa600459bfad7d1e730c9fa7a79 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Thu, 17 Feb 2022 19:41:22 +0100 Subject: [PATCH 02/25] WiP. --- .../src/view/downcastwriter.js | 6 ++- .../src/integrations/documentlist.js | 47 +++++++------------ .../src/documentlist/documentlistediting.js | 10 +++- .../src/documentlist/utils/view.js | 33 ++++++------- .../src/documentlistproperties/converters.js | 5 +- .../tests/documentlist/utils/view.js | 12 +++++ 6 files changed, 61 insertions(+), 52 deletions(-) diff --git a/packages/ckeditor5-engine/src/view/downcastwriter.js b/packages/ckeditor5-engine/src/view/downcastwriter.js index 92d992eebad..78c438ce673 100644 --- a/packages/ckeditor5-engine/src/view/downcastwriter.js +++ b/packages/ckeditor5-engine/src/view/downcastwriter.js @@ -2189,5 +2189,9 @@ function validateRangeContainer( range, errorContext ) { // @param {module:engine/view/element~Element} b // @returns {Boolean} function canBeJoined( a, b ) { - return a.id === null && b.id === null; + if ( a.id === null && b.id === null ) { + return true; + } + + return a.id === b.id; } diff --git a/packages/ckeditor5-html-support/src/integrations/documentlist.js b/packages/ckeditor5-html-support/src/integrations/documentlist.js index 1a318c100b5..5762b5c9ae9 100644 --- a/packages/ckeditor5-html-support/src/integrations/documentlist.js +++ b/packages/ckeditor5-html-support/src/integrations/documentlist.js @@ -41,30 +41,17 @@ export default class DocumentListElementSupport extends Plugin { dataFilter.on( 'register:li', registerFilter( { attributeName: 'htmlLiAttributes', - viewElementName: 'li', - - appliesToListItem() { - return true; - } - + viewElementNames: [ 'li' ] }, this.editor ) ); dataFilter.on( 'register:ul', registerFilter( { - attributeName: 'htmlUlAttributes', - viewElementName: 'ul', - - appliesToListItem( item ) { - return item.getAttribute( 'listType' ) != 'numbered'; - } + attributeName: 'htmlListAttributes', + viewElementNames: [ 'ul', 'ol' ] }, this.editor ) ); dataFilter.on( 'register:ol', registerFilter( { - attributeName: 'htmlOlAttributes', - viewElementName: 'ol', - - appliesToListItem( item ) { - return item.getAttribute( 'listType' ) == 'numbered'; - } + attributeName: 'htmlListAttributes', + viewElementNames: [ 'ul', 'ol' ] }, this.editor ) ); } } @@ -79,6 +66,8 @@ function registerFilter( strategy, editor ) { return evt => { if ( schema.checkAttribute( '$block', attributeName ) ) { + evt.stop(); + return; } @@ -105,28 +94,28 @@ function registerFilter( strategy, editor ) { // @param {module:html-support/datafilter~DataFilter} dataFilter // @returns {Function} Returns a conversion callback. function viewToModelListAttributeConverter( strategy, dataFilter ) { - const { attributeName, viewElementName } = strategy; + const { attributeName, viewElementNames } = strategy; return dispatcher => { - dispatcher.on( `element:${ viewElementName }`, ( evt, data, conversionApi ) => { + dispatcher.on( 'element', ( evt, data, conversionApi ) => { const viewElement = data.viewItem; - const viewAttributes = dataFilter._consumeAllowedAttributes( viewElement, conversionApi ); - if ( !viewAttributes ) { + if ( !viewElement.is( 'element' ) || !viewElementNames.includes( viewElement.name ) ) { return; } + if ( !data.modelRange ) { + return; + } + + const viewAttributes = dataFilter._consumeAllowedAttributes( viewElement, conversionApi ); + for ( const item of data.modelRange.getItems( { shallow: true } ) ) { // Apply only to list item blocks. if ( !item.hasAttribute( 'listItemId' ) ) { continue; } - // Apply only for the related list type. - if ( !strategy.appliesToListItem( item ) ) { - continue; - } - // Set list attributes only on same level items, those nested deeper are already handled // by the recursive conversion. if ( item.hasAttribute( attributeName ) ) { @@ -210,8 +199,8 @@ function wrapListItemBlock( listItem, viewRange, strategy, writer ) { let currentListItem = listItem; for ( let indent = listItemIndent; indent >= 0; indent-- ) { - if ( htmlAttributes && strategy.appliesToListItem( currentListItem ) ) { - const listViewElement = strategy.viewElementName == 'li' ? + if ( htmlAttributes ) { + const listViewElement = strategy.viewElementNames.includes( 'li' ) ? createListItemElement( writer, indent, listItemId ) : createListElement( writer, indent, listType ); diff --git a/packages/ckeditor5-list/src/documentlist/documentlistediting.js b/packages/ckeditor5-list/src/documentlist/documentlistediting.js index 4bb2d507c9b..6572a632d60 100644 --- a/packages/ckeditor5-list/src/documentlist/documentlistediting.js +++ b/packages/ckeditor5-list/src/documentlist/documentlistediting.js @@ -37,10 +37,13 @@ import { isListItemBlock, LIST_BASE_ATTRIBUTES } from './utils/model'; +import { + getViewElementIdForListType, + getViewElementNameForListType +} from './utils/view'; import ListWalker, { iterateSiblingListBlocks } from './utils/listwalker'; import '../../theme/documentlist.css'; -import { getViewElementNameForListType } from './utils/view'; /** * The editing part of the document-list feature. It handles creating, editing and removing lists and list items. @@ -352,7 +355,10 @@ export default class DocumentListEditing extends Plugin { // For UL and OL check if the name and ID of element is correct. this.on( 'refreshChecker:list', ( evt, { viewElement, modelAttributes } ) => { - if ( viewElement.name != getViewElementNameForListType( modelAttributes.listType ) ) { + if ( + viewElement.name != getViewElementNameForListType( modelAttributes.listType ) || + viewElement.id != getViewElementIdForListType( modelAttributes.listType, modelAttributes.listIndent ) + ) { evt.return = true; evt.stop(); } diff --git a/packages/ckeditor5-list/src/documentlist/utils/view.js b/packages/ckeditor5-list/src/documentlist/utils/view.js index 31a0d3f90f8..61621359f41 100644 --- a/packages/ckeditor5-list/src/documentlist/utils/view.js +++ b/packages/ckeditor5-list/src/documentlist/utils/view.js @@ -99,10 +99,11 @@ export function getIndent( listItem ) { * @param {'bulleted'|'numbered'} type The list type. * @returns {module:engine/view/attributeelement~AttributeElement} */ -export function createListElement( writer, indent, type ) { +export function createListElement( writer, indent, type, id = getViewElementIdForListType( type, indent ) ) { // Negative priorities so that restricted editing attribute won't wrap lists. return writer.createAttributeElement( getViewElementNameForListType( type ), null, { - priority: 2 * indent / 100 - 100 + priority: 2 * indent / 100 - 100, + id } ); } @@ -117,22 +118,10 @@ export function createListElement( writer, indent, type ) { */ export function createListItemElement( writer, indent, id ) { // Negative priorities so that restricted editing attribute won't wrap list items. - const viewElement = writer.createAttributeElement( 'li', null, { + return writer.createAttributeElement( 'li', null, { priority: ( 2 * indent + 1 ) / 100 - 100, - // id + id } ); - - writer.setCustomProperty( 'listItemId', id, viewElement ); - - viewElement.isSimilar = function( otherElement ) { - if ( this.getCustomProperty( 'listItemId' ) == otherElement.getCustomProperty( 'listItemId' ) ) { - return true; - } - - return Object.getPrototypeOf( viewElement ).isSimilar.call( viewElement, otherElement ); - }; - - return viewElement; } /** @@ -145,3 +134,15 @@ export function createListItemElement( writer, indent, id ) { export function getViewElementNameForListType( type ) { return type == 'numbered' ? 'ol' : 'ul'; } + +/** + * Returns a view element ID for the given list type and indent. + * + * @protected + * @param {'bulleted'|'numbered'} type The list type. + * @param {Number} indent The list indent level. + * @returns {String} + */ +export function getViewElementIdForListType( type, indent ) { + return `list-${ type }-${ indent }`; +} diff --git a/packages/ckeditor5-list/src/documentlistproperties/converters.js b/packages/ckeditor5-list/src/documentlistproperties/converters.js index d459857c410..d76ac24873c 100644 --- a/packages/ckeditor5-list/src/documentlistproperties/converters.js +++ b/packages/ckeditor5-list/src/documentlistproperties/converters.js @@ -32,10 +32,7 @@ export function listPropertiesUpcastConverter( strategy ) { } if ( !data.modelRange ) { - const { modelRange, modelCursor } = conversionApi.convertChildren( data.viewItem, data.modelCursor ); - - data.modelRange = modelRange; - data.modelCursor = modelCursor; + Object.assign( data, conversionApi.convertChildren( data.viewItem, data.modelCursor ) ); } let applied = false; diff --git a/packages/ckeditor5-list/tests/documentlist/utils/view.js b/packages/ckeditor5-list/tests/documentlist/utils/view.js index 00269aa9066..93bd2e32af8 100644 --- a/packages/ckeditor5-list/tests/documentlist/utils/view.js +++ b/packages/ckeditor5-list/tests/documentlist/utils/view.js @@ -7,6 +7,7 @@ import { createListElement, createListItemElement, getIndent, + getViewElementIdForListType, getViewElementNameForListType, isListItemView, isListView @@ -302,4 +303,15 @@ describe( 'DocumentList - utils - view', () => { expect( getViewElementNameForListType( 'sth' ) ).to.equal( 'ul' ); } ); } ); + + describe( 'getViewElementIdForListType()', () => { + it( 'should generate view element ID for the given list type and indent', () => { + expect( getViewElementIdForListType( 'bulleted', 0 ) ).to.equal( 'list-bulleted-0' ); + expect( getViewElementIdForListType( 'bulleted', 1 ) ).to.equal( 'list-bulleted-1' ); + expect( getViewElementIdForListType( 'bulleted', 2 ) ).to.equal( 'list-bulleted-2' ); + expect( getViewElementIdForListType( 'numbered', 0 ) ).to.equal( 'list-numbered-0' ); + expect( getViewElementIdForListType( 'numbered', 1 ) ).to.equal( 'list-numbered-1' ); + expect( getViewElementIdForListType( 'numbered', 2 ) ).to.equal( 'list-numbered-2' ); + } ); + } ); } ); From ad30c0bc4392b60b5cbca95d4bbee8ab6db859cb Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Fri, 18 Feb 2022 19:26:29 +0100 Subject: [PATCH 03/25] WiP. --- .../src/integrations/documentlist.js | 40 +++++++++---------- .../tests/manual/documentlist.html | 26 ++++++++++-- .../tests/manual/documentlist.js | 20 ++++++++-- 3 files changed, 58 insertions(+), 28 deletions(-) diff --git a/packages/ckeditor5-html-support/src/integrations/documentlist.js b/packages/ckeditor5-html-support/src/integrations/documentlist.js index 5762b5c9ae9..27a99ceddbd 100644 --- a/packages/ckeditor5-html-support/src/integrations/documentlist.js +++ b/packages/ckeditor5-html-support/src/integrations/documentlist.js @@ -105,7 +105,7 @@ function viewToModelListAttributeConverter( strategy, dataFilter ) { } if ( !data.modelRange ) { - return; + Object.assign( data, conversionApi.convertChildren( data.viewItem, data.modelCursor ) ); } const viewAttributes = dataFilter._consumeAllowedAttributes( viewElement, conversionApi ); @@ -135,29 +135,29 @@ function viewToModelListAttributeConverter( strategy, dataFilter ) { // @param {String} attributeName // @returns {Function} Returns a conversion callback. function modelToViewListAttributeConverter( strategy, model ) { - const { attributeName } = strategy; - return dispatcher => { - dispatcher.on( `attribute:${ attributeName }`, ( evt, data, conversionApi ) => { - const { writer, mapper, consumable } = conversionApi; - const listItem = data.item; - - // Check and consume only the list properties attributes (the base list attributes are already consumed - // but should also trigger conversion of list properties). - if ( !LIST_BASE_ATTRIBUTES.includes( data.attributeKey ) && !consumable.consume( listItem, evt.name ) ) { - return; - } + for ( const attributeName of [ ...LIST_BASE_ATTRIBUTES, strategy.attributeName ] ) { + dispatcher.on( `attribute:${ attributeName }`, ( evt, data, conversionApi ) => { + const { writer, mapper, consumable } = conversionApi; + const listItem = data.item; + + // Check and consume only the list properties attributes (the base list attributes are already consumed + // but should also trigger conversion of list properties). + if ( !LIST_BASE_ATTRIBUTES.includes( data.attributeKey ) && !consumable.consume( listItem, evt.name ) ) { + return; + } - // Use positions mapping instead of mapper.toViewElement( listItem ) to find outermost view element. - // This is for cases when mapping is using inner view element like in the code blocks (pre > code). - const viewElement = findMappedViewElement( listItem, mapper, model ); + // Use positions mapping instead of mapper.toViewElement( listItem ) to find outermost view element. + // This is for cases when mapping is using inner view element like in the code blocks (pre > code). + const viewElement = findMappedViewElement( listItem, mapper, model ); - // Unwrap element from current list wrappers. - // unwrapListItemBlock( viewElement, strategy, writer ); + // Unwrap element from current list wrappers. + // unwrapListItemBlock( viewElement, strategy, writer ); - // Then wrap them with the new list wrappers. - wrapListItemBlock( listItem, writer.createRangeOn( viewElement ), strategy, writer ); - } ); + // Then wrap them with the new list wrappers. + wrapListItemBlock( listItem, writer.createRangeOn( viewElement ), strategy, writer ); + } ); + } }; } diff --git a/packages/ckeditor5-html-support/tests/manual/documentlist.html b/packages/ckeditor5-html-support/tests/manual/documentlist.html index 07f4453bce3..f47a9a67096 100644 --- a/packages/ckeditor5-html-support/tests/manual/documentlist.html +++ b/packages/ckeditor5-html-support/tests/manual/documentlist.html @@ -4,13 +4,31 @@
-
    -
  • +
      +

    • foo

      +
+ + diff --git a/packages/ckeditor5-html-support/tests/manual/documentlist.js b/packages/ckeditor5-html-support/tests/manual/documentlist.js index d43a557bb8f..c64f97d0fde 100644 --- a/packages/ckeditor5-html-support/tests/manual/documentlist.js +++ b/packages/ckeditor5-html-support/tests/manual/documentlist.js @@ -29,12 +29,12 @@ class ExtendHTMLSupport extends Plugin { const dataFilter = this.editor.plugins.get( 'DataFilter' ); dataFilter.allowElement( /^p$/ ); - dataFilter.allowAttributes( { name: /^p$/, attributes: true } ); + dataFilter.allowAttributes( { name: /^p$/, attributes: true, styles: true } ); dataFilter.allowElement( /^(ul|ol)$/ ); - dataFilter.allowAttributes( { name: /^(ul|ol)$/, attributes: true } ); + dataFilter.allowAttributes( { name: /^(ul|ol)$/, attributes: true, styles: true } ); dataFilter.allowElement( /^(li)$/ ); - dataFilter.allowAttributes( { name: /^(li)$/, attributes: true } ); + dataFilter.allowAttributes( { name: /^(li)$/, attributes: true, styles: true } ); } } @@ -50,7 +50,19 @@ ClassicEditor Strikethrough, SourceEditing ], - toolbar: [ 'sourceEditing', '|', 'bold', 'italic', 'strikethrough' ] + toolbar: [ + 'sourceEditing', '|', + 'numberedList', 'bulletedList', '|', + 'outdent', 'indent', '|', + 'bold', 'italic', 'strikethrough' + ], + list: { + properties: { + styles: true, + startIndex: true, + reversed: true + } + } } ) .then( editor => { window.editor = editor; From b89ad73501c27a4bea10395f4f1bfe98599ec6b0 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Sun, 20 Feb 2022 23:55:43 +0100 Subject: [PATCH 04/25] WiP. --- .../src/integrations/documentlist.js | 4 +-- .../src/documentlist/documentlistediting.js | 36 +++++++++++++++++++ .../src/documentlistproperties/converters.js | 2 +- .../documentlistpropertiesediting.js | 2 +- 4 files changed, 40 insertions(+), 4 deletions(-) diff --git a/packages/ckeditor5-html-support/src/integrations/documentlist.js b/packages/ckeditor5-html-support/src/integrations/documentlist.js index 27a99ceddbd..08d661503d4 100644 --- a/packages/ckeditor5-html-support/src/integrations/documentlist.js +++ b/packages/ckeditor5-html-support/src/integrations/documentlist.js @@ -136,14 +136,14 @@ function viewToModelListAttributeConverter( strategy, dataFilter ) { // @returns {Function} Returns a conversion callback. function modelToViewListAttributeConverter( strategy, model ) { return dispatcher => { - for ( const attributeName of [ ...LIST_BASE_ATTRIBUTES, strategy.attributeName ] ) { + for ( const attributeName of [ /*...LIST_BASE_ATTRIBUTES,*/ strategy.attributeName ] ) { dispatcher.on( `attribute:${ attributeName }`, ( evt, data, conversionApi ) => { const { writer, mapper, consumable } = conversionApi; const listItem = data.item; // Check and consume only the list properties attributes (the base list attributes are already consumed // but should also trigger conversion of list properties). - if ( !LIST_BASE_ATTRIBUTES.includes( data.attributeKey ) && !consumable.consume( listItem, evt.name ) ) { + if ( /*!LIST_BASE_ATTRIBUTES.includes( data.attributeKey ) &&*/ !consumable.consume( listItem, evt.name ) ) { return; } diff --git a/packages/ckeditor5-list/src/documentlist/documentlistediting.js b/packages/ckeditor5-list/src/documentlist/documentlistediting.js index 6572a632d60..ec3c43fe73d 100644 --- a/packages/ckeditor5-list/src/documentlist/documentlistediting.js +++ b/packages/ckeditor5-list/src/documentlist/documentlistediting.js @@ -341,6 +341,42 @@ export default class DocumentListEditing extends Plugin { for ( const attributeName of LIST_BASE_ATTRIBUTES ) { dispatcher.on( `attribute:${ attributeName }`, listItemDowncastConverter( LIST_BASE_ATTRIBUTES, model ) ); } + + dispatcher.on( 'reduceChanges', ( evt, data ) => { + const reducedChanges = []; + const attributeNames = [ + ...LIST_BASE_ATTRIBUTES, + ...this.getSameListDefiningAttributes(), + 'htmlListAttributes', + 'htmlLiAttributes' + ]; + const reWrappedItems = new Set(); + + for ( const change of data.changes ) { + if ( change.type == 'attribute' && attributeNames.includes( change.attributeKey ) ) { + const node = change.range.start.nodeAfter; + + if ( reWrappedItems.has( node ) ) { + continue; + } + + reWrappedItems.add( node ); + + for ( const key of attributeNames ) { + reducedChanges.push( { + type: 'attribute', + attributeKey: key, + attributeNewValue: node.getAttribute( key ), + range: change.range + } ); + } + } else { + reducedChanges.push( change ); + } + } + + data.changes = reducedChanges; + } ); } ); this.listenTo( model.document, 'change:data', reconvertItemsOnDataChange( model, editor.editing, this ) ); diff --git a/packages/ckeditor5-list/src/documentlistproperties/converters.js b/packages/ckeditor5-list/src/documentlistproperties/converters.js index d76ac24873c..7ab8c005045 100644 --- a/packages/ckeditor5-list/src/documentlistproperties/converters.js +++ b/packages/ckeditor5-list/src/documentlistproperties/converters.js @@ -77,7 +77,7 @@ export function listPropertiesDowncastConverter( strategy, model ) { // Check and consume only the list properties attributes (the base list attributes are already consumed // but should also trigger conversion of list properties). - if ( !LIST_BASE_ATTRIBUTES.includes( data.attributeKey ) && !consumable.consume( listItem, evt.name ) ) { + if ( /*!LIST_BASE_ATTRIBUTES.includes( data.attributeKey ) &&*/ !consumable.consume( listItem, evt.name ) ) { return; } diff --git a/packages/ckeditor5-list/src/documentlistproperties/documentlistpropertiesediting.js b/packages/ckeditor5-list/src/documentlistproperties/documentlistpropertiesediting.js index a4ba041cbcf..c89ff3d20b1 100644 --- a/packages/ckeditor5-list/src/documentlistproperties/documentlistpropertiesediting.js +++ b/packages/ckeditor5-list/src/documentlistproperties/documentlistpropertiesediting.js @@ -84,7 +84,7 @@ export default class DocumentListPropertiesEditing extends Plugin { } ); editor.conversion.for( 'downcast' ).add( dispatcher => { for ( const strategy of strategies ) { - for ( const attributeName of [ ...LIST_BASE_ATTRIBUTES, strategy.attributeName ] ) { + for ( const attributeName of [ /*...LIST_BASE_ATTRIBUTES,*/ strategy.attributeName ] ) { dispatcher.on( `attribute:${ attributeName }`, listPropertiesDowncastConverter( strategy, model ) ); } } From 4b60aaac49eaf59914d1a9ecc6dfc5443e5fcf58 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Mon, 21 Feb 2022 18:51:19 +0100 Subject: [PATCH 05/25] WiP. --- .../src/integrations/documentlist.js | 34 ++++++++----------- .../src/documentlist/documentlistediting.js | 4 +-- .../src/documentlist/utils/view.js | 2 +- .../src/documentlistproperties/converters.js | 4 +-- .../documentlistpropertiesediting.js | 5 ++- .../documentlistproperties/converters.js | 18 ++++++++++ 6 files changed, 39 insertions(+), 28 deletions(-) diff --git a/packages/ckeditor5-html-support/src/integrations/documentlist.js b/packages/ckeditor5-html-support/src/integrations/documentlist.js index 08d661503d4..5d9af1a30b7 100644 --- a/packages/ckeditor5-html-support/src/integrations/documentlist.js +++ b/packages/ckeditor5-html-support/src/integrations/documentlist.js @@ -136,28 +136,24 @@ function viewToModelListAttributeConverter( strategy, dataFilter ) { // @returns {Function} Returns a conversion callback. function modelToViewListAttributeConverter( strategy, model ) { return dispatcher => { - for ( const attributeName of [ /*...LIST_BASE_ATTRIBUTES,*/ strategy.attributeName ] ) { - dispatcher.on( `attribute:${ attributeName }`, ( evt, data, conversionApi ) => { - const { writer, mapper, consumable } = conversionApi; - const listItem = data.item; - - // Check and consume only the list properties attributes (the base list attributes are already consumed - // but should also trigger conversion of list properties). - if ( /*!LIST_BASE_ATTRIBUTES.includes( data.attributeKey ) &&*/ !consumable.consume( listItem, evt.name ) ) { - return; - } + dispatcher.on( `attribute:${ strategy.attributeName }`, ( evt, data, conversionApi ) => { + const { writer, mapper, consumable } = conversionApi; + const listItem = data.item; + + if ( !consumable.consume( listItem, evt.name ) ) { + return; + } - // Use positions mapping instead of mapper.toViewElement( listItem ) to find outermost view element. - // This is for cases when mapping is using inner view element like in the code blocks (pre > code). - const viewElement = findMappedViewElement( listItem, mapper, model ); + // Use positions mapping instead of mapper.toViewElement( listItem ) to find outermost view element. + // This is for cases when mapping is using inner view element like in the code blocks (pre > code). + const viewElement = findMappedViewElement( listItem, mapper, model ); - // Unwrap element from current list wrappers. - // unwrapListItemBlock( viewElement, strategy, writer ); + // Unwrap element from current list wrappers. + // unwrapListItemBlock( viewElement, strategy, writer ); - // Then wrap them with the new list wrappers. - wrapListItemBlock( listItem, writer.createRangeOn( viewElement ), strategy, writer ); - } ); - } + // Then wrap them with the new list wrappers. + wrapListItemBlock( listItem, writer.createRangeOn( viewElement ), strategy, writer ); + } ); }; } diff --git a/packages/ckeditor5-list/src/documentlist/documentlistediting.js b/packages/ckeditor5-list/src/documentlist/documentlistediting.js index ec3c43fe73d..b6d57b7c179 100644 --- a/packages/ckeditor5-list/src/documentlist/documentlistediting.js +++ b/packages/ckeditor5-list/src/documentlist/documentlistediting.js @@ -392,8 +392,8 @@ export default class DocumentListEditing extends Plugin { // For UL and OL check if the name and ID of element is correct. this.on( 'refreshChecker:list', ( evt, { viewElement, modelAttributes } ) => { if ( - viewElement.name != getViewElementNameForListType( modelAttributes.listType ) || - viewElement.id != getViewElementIdForListType( modelAttributes.listType, modelAttributes.listIndent ) + viewElement.name != getViewElementNameForListType( modelAttributes.listType ) /* || + viewElement.id != getViewElementIdForListType( modelAttributes.listType, modelAttributes.listIndent ) */ ) { evt.return = true; evt.stop(); diff --git a/packages/ckeditor5-list/src/documentlist/utils/view.js b/packages/ckeditor5-list/src/documentlist/utils/view.js index 61621359f41..1fb5c4643d2 100644 --- a/packages/ckeditor5-list/src/documentlist/utils/view.js +++ b/packages/ckeditor5-list/src/documentlist/utils/view.js @@ -99,7 +99,7 @@ export function getIndent( listItem ) { * @param {'bulleted'|'numbered'} type The list type. * @returns {module:engine/view/attributeelement~AttributeElement} */ -export function createListElement( writer, indent, type, id = getViewElementIdForListType( type, indent ) ) { +export function createListElement( writer, indent, type, id /* = getViewElementIdForListType( type, indent ) */ ) { // Negative priorities so that restricted editing attribute won't wrap lists. return writer.createAttributeElement( getViewElementNameForListType( type ), null, { priority: 2 * indent / 100 - 100, diff --git a/packages/ckeditor5-list/src/documentlistproperties/converters.js b/packages/ckeditor5-list/src/documentlistproperties/converters.js index 7ab8c005045..7ce81fcbcf6 100644 --- a/packages/ckeditor5-list/src/documentlistproperties/converters.js +++ b/packages/ckeditor5-list/src/documentlistproperties/converters.js @@ -75,9 +75,7 @@ export function listPropertiesDowncastConverter( strategy, model ) { const { writer, mapper, consumable } = conversionApi; const listItem = data.item; - // Check and consume only the list properties attributes (the base list attributes are already consumed - // but should also trigger conversion of list properties). - if ( /*!LIST_BASE_ATTRIBUTES.includes( data.attributeKey ) &&*/ !consumable.consume( listItem, evt.name ) ) { + if ( !consumable.consume( listItem, evt.name ) ) { return; } diff --git a/packages/ckeditor5-list/src/documentlistproperties/documentlistpropertiesediting.js b/packages/ckeditor5-list/src/documentlistproperties/documentlistpropertiesediting.js index c89ff3d20b1..83e423eae24 100644 --- a/packages/ckeditor5-list/src/documentlistproperties/documentlistpropertiesediting.js +++ b/packages/ckeditor5-list/src/documentlistproperties/documentlistpropertiesediting.js @@ -82,11 +82,10 @@ export default class DocumentListPropertiesEditing extends Plugin { dispatcher.on( 'element:ul', listPropertiesUpcastConverter( strategy ) ); } } ); + editor.conversion.for( 'downcast' ).add( dispatcher => { for ( const strategy of strategies ) { - for ( const attributeName of [ /*...LIST_BASE_ATTRIBUTES,*/ strategy.attributeName ] ) { - dispatcher.on( `attribute:${ attributeName }`, listPropertiesDowncastConverter( strategy, model ) ); - } + dispatcher.on( `attribute:${ strategy.attributeName }`, listPropertiesDowncastConverter( strategy, model ) ); } } ); diff --git a/packages/ckeditor5-list/tests/documentlistproperties/converters.js b/packages/ckeditor5-list/tests/documentlistproperties/converters.js index 7de0f0ef518..833c1e54e18 100644 --- a/packages/ckeditor5-list/tests/documentlistproperties/converters.js +++ b/packages/ckeditor5-list/tests/documentlistproperties/converters.js @@ -537,6 +537,24 @@ describe( 'DocumentListPropertiesEditing - converters', () => { expect( test.reconvertSpy.callCount ).to.equal( 0 ); } ); + + it( 'insert with attributes in a specific order', () => { + test.insert( + modelList( ` + p + [a + b + c] + ` ), + + '

p

' + + '
    ' + + '
  • a
    • ' + + '
  • b
    • ' + + '
  • c
    • ' + + '
' + ); + } ); } ); describe( 'remove', () => { From 1c105b7d672bfb557bd3354299a08a8f301a2cb2 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Tue, 22 Feb 2022 19:18:37 +0100 Subject: [PATCH 06/25] WiP. --- .../src/view/downcastwriter.js | 55 +++++++++++++++++-- .../src/integrations/documentlist.js | 33 +---------- .../src/documentlist/converters.js | 12 ++-- .../src/documentlist/documentlistediting.js | 17 +++++- .../src/documentlist/utils/view.js | 2 +- .../src/documentlistproperties/converters.js | 27 --------- 6 files changed, 74 insertions(+), 72 deletions(-) diff --git a/packages/ckeditor5-engine/src/view/downcastwriter.js b/packages/ckeditor5-engine/src/view/downcastwriter.js index 78c438ce673..b85bafbfa10 100644 --- a/packages/ckeditor5-engine/src/view/downcastwriter.js +++ b/packages/ckeditor5-engine/src/view/downcastwriter.js @@ -672,6 +672,9 @@ export default class DowncastWriter { const count = nodeBefore.childCount; nodeBefore._appendChild( nodeAfter.getChildren() ); + // Merge the element attributes in case those have a same ID (other attributes might differ). + this._mergeElementAttributes( nodeAfter, nodeBefore ); + nodeAfter._remove(); this._removeFromClonedElementsGroup( nodeAfter ); @@ -1626,10 +1629,21 @@ export default class DowncastWriter { * @returns {Boolean} Returns `true` if elements are merged. */ _wrapAttributeElement( wrapper, toWrap ) { - if ( !canBeJoined( wrapper, toWrap ) ) { + // if ( !canBeJoined( wrapper, toWrap ) ) { + // return false; + // } + + if ( wrapper.id !== toWrap.id ) { return false; } + if ( wrapper.id !== null ) { + // Move all attributes/classes/styles from wrapper to wrapped AttributeElement. + this._mergeElementAttributes( wrapper, toWrap ); + + return true; + } + // Can't merge if name or priority differs. if ( wrapper.name !== toWrap.name || wrapper.priority !== toWrap.priority ) { return false; @@ -1656,6 +1670,17 @@ export default class DowncastWriter { } // Move all attributes/classes/styles from wrapper to wrapped AttributeElement. + this._mergeElementAttributes( wrapper, toWrap ); + + return true; + } + + /** + * TODO + * + * @private + */ + _mergeElementAttributes( wrapper, toWrap ) { for ( const key of wrapper.getAttributeKeys() ) { // Classes and styles should be checked separately. if ( key === 'class' || key === 'style' ) { @@ -1679,8 +1704,6 @@ export default class DowncastWriter { this.addClass( key, toWrap ); } } - - return true; } /** @@ -1694,10 +1717,21 @@ export default class DowncastWriter { * @returns {Boolean} Returns `true` if elements are unwrapped. **/ _unwrapAttributeElement( wrapper, toUnwrap ) { - if ( !canBeJoined( wrapper, toUnwrap ) ) { + // if ( !canBeJoined( wrapper, toUnwrap ) ) { + // return false; + // } + + if ( wrapper.id !== toUnwrap.id ) { return false; } + if ( wrapper.id !== null ) { + // Remove all wrapper's attributes from unwrapped element. + this._unmergeElementAttributes( wrapper, toUnwrap ); + + return true; + } + // Can't unwrap if name or priority differs. if ( wrapper.name !== toUnwrap.name || wrapper.priority !== toUnwrap.priority ) { return false; @@ -1730,6 +1764,17 @@ export default class DowncastWriter { } // Remove all wrapper's attributes from unwrapped element. + this._unmergeElementAttributes( wrapper, toUnwrap ); + + return true; + } + + /** + * TODO + * + * @private + */ + _unmergeElementAttributes( wrapper, toUnwrap ) { for ( const key of wrapper.getAttributeKeys() ) { // Classes and styles should be checked separately. if ( key === 'class' || key === 'style' ) { @@ -1744,8 +1789,6 @@ export default class DowncastWriter { // Remove all wrapper's styles from unwrapped element. this.removeStyle( Array.from( wrapper.getStyleNames() ), toUnwrap ); - - return true; } /** diff --git a/packages/ckeditor5-html-support/src/integrations/documentlist.js b/packages/ckeditor5-html-support/src/integrations/documentlist.js index 5d9af1a30b7..6c0afbe5492 100644 --- a/packages/ckeditor5-html-support/src/integrations/documentlist.js +++ b/packages/ckeditor5-html-support/src/integrations/documentlist.js @@ -148,39 +148,12 @@ function modelToViewListAttributeConverter( strategy, model ) { // This is for cases when mapping is using inner view element like in the code blocks (pre > code). const viewElement = findMappedViewElement( listItem, mapper, model ); - // Unwrap element from current list wrappers. - // unwrapListItemBlock( viewElement, strategy, writer ); - // Then wrap them with the new list wrappers. wrapListItemBlock( listItem, writer.createRangeOn( viewElement ), strategy, writer ); } ); }; } -// Unwraps all ol, ul, and li attribute elements that are wrapping the provided view element. -function unwrapListItemBlock( viewElement, strategy, writer ) { - let attributeElement = viewElement.parent; - - while ( attributeElement.is( 'attributeElement' ) && [ 'ul', 'ol', 'li' ].includes( attributeElement.name ) ) { - const parentElement = attributeElement.parent; - const attributeValue = strategy.getAttributeOnUpcast( attributeElement ); - - // Unwrap only if the model attribute is really downcasted (it's not the default value). - if ( isListView( attributeElement ) && attributeValue !== strategy.defaultValue ) { - // Make a clone of an attribute element that only includes properties of list styles. - const element = writer.createAttributeElement( attributeElement.name, null, { - priority: attributeElement.priority, - id: attributeElement.id - } ); - - strategy.setAttributeOnDowncast( writer, attributeValue, element ); - writer.unwrap( writer.createRangeOn( viewElement ), element ); - } - - attributeElement = parentElement; - } -} - // Wraps the given list item with appropriate attribute elements for ul, ol, and li. function wrapListItemBlock( listItem, viewRange, strategy, writer ) { if ( !listItem.hasAttribute( 'listIndent' ) ) { @@ -196,12 +169,12 @@ function wrapListItemBlock( listItem, viewRange, strategy, writer ) { for ( let indent = listItemIndent; indent >= 0; indent-- ) { if ( htmlAttributes ) { - const listViewElement = strategy.viewElementNames.includes( 'li' ) ? + const viewElement = strategy.viewElementNames.includes( 'li' ) ? createListItemElement( writer, indent, listItemId ) : createListElement( writer, indent, listType ); - setViewAttributes( writer, htmlAttributes, listViewElement ); - viewRange = writer.wrap( viewRange, listViewElement ); + setViewAttributes( writer, htmlAttributes, viewElement ); + viewRange = writer.wrap( viewRange, viewElement ); } if ( indent == 0 ) { diff --git a/packages/ckeditor5-list/src/documentlist/converters.js b/packages/ckeditor5-list/src/documentlist/converters.js index 151a6021a3c..d27a66dc12e 100644 --- a/packages/ckeditor5-list/src/documentlist/converters.js +++ b/packages/ckeditor5-list/src/documentlist/converters.js @@ -303,11 +303,13 @@ export function listItemDowncastConverter( attributes, model ) { // This is for cases when mapping is using inner view element like in the code blocks (pre > code). const viewElement = findMappedViewElement( listItem, mapper, model ); - // Unwrap element from current list wrappers. - unwrapListItemBlock( viewElement, writer ); - - // Then wrap them with the new list wrappers. - wrapListItemBlock( listItem, writer.createRangeOn( viewElement ), writer ); + if ( data.attributeKey == 'listIndent' && data.attributeNewValue === null ) { + // Unwrap element from current list wrappers. + unwrapListItemBlock( viewElement, writer ); + } else { + // Then wrap them with the new list wrappers. + wrapListItemBlock( listItem, writer.createRangeOn( viewElement ), writer ); + } }; } diff --git a/packages/ckeditor5-list/src/documentlist/documentlistediting.js b/packages/ckeditor5-list/src/documentlist/documentlistediting.js index b6d57b7c179..f20f81b151d 100644 --- a/packages/ckeditor5-list/src/documentlist/documentlistediting.js +++ b/packages/ckeditor5-list/src/documentlist/documentlistediting.js @@ -346,7 +346,9 @@ export default class DocumentListEditing extends Plugin { const reducedChanges = []; const attributeNames = [ ...LIST_BASE_ATTRIBUTES, - ...this.getSameListDefiningAttributes(), + 'listStyle', + 'listStart', + 'listReversed', 'htmlListAttributes', 'htmlLiAttributes' ]; @@ -362,10 +364,19 @@ export default class DocumentListEditing extends Plugin { reWrappedItems.add( node ); + reducedChanges.push( { + type: 'attribute', + attributeKey: 'listIndent', + attributeOldValue: node.getAttribute( 'listIndent' ), + attributeNewValue: null, + range: change.range + } ); + for ( const key of attributeNames ) { reducedChanges.push( { type: 'attribute', attributeKey: key, + attributeOldValue: null, attributeNewValue: node.getAttribute( key ), range: change.range } ); @@ -392,8 +403,8 @@ export default class DocumentListEditing extends Plugin { // For UL and OL check if the name and ID of element is correct. this.on( 'refreshChecker:list', ( evt, { viewElement, modelAttributes } ) => { if ( - viewElement.name != getViewElementNameForListType( modelAttributes.listType ) /* || - viewElement.id != getViewElementIdForListType( modelAttributes.listType, modelAttributes.listIndent ) */ + viewElement.name != getViewElementNameForListType( modelAttributes.listType ) || + viewElement.id != getViewElementIdForListType( modelAttributes.listType, modelAttributes.listIndent ) ) { evt.return = true; evt.stop(); diff --git a/packages/ckeditor5-list/src/documentlist/utils/view.js b/packages/ckeditor5-list/src/documentlist/utils/view.js index 1fb5c4643d2..61621359f41 100644 --- a/packages/ckeditor5-list/src/documentlist/utils/view.js +++ b/packages/ckeditor5-list/src/documentlist/utils/view.js @@ -99,7 +99,7 @@ export function getIndent( listItem ) { * @param {'bulleted'|'numbered'} type The list type. * @returns {module:engine/view/attributeelement~AttributeElement} */ -export function createListElement( writer, indent, type, id /* = getViewElementIdForListType( type, indent ) */ ) { +export function createListElement( writer, indent, type, id = getViewElementIdForListType( type, indent ) ) { // Negative priorities so that restricted editing attribute won't wrap lists. return writer.createAttributeElement( getViewElementNameForListType( type ), null, { priority: 2 * indent / 100 - 100, diff --git a/packages/ckeditor5-list/src/documentlistproperties/converters.js b/packages/ckeditor5-list/src/documentlistproperties/converters.js index 7ce81fcbcf6..658067c51dd 100644 --- a/packages/ckeditor5-list/src/documentlistproperties/converters.js +++ b/packages/ckeditor5-list/src/documentlistproperties/converters.js @@ -83,38 +83,11 @@ export function listPropertiesDowncastConverter( strategy, model ) { // This is for cases when mapping is using inner view element like in the code blocks (pre > code). const viewElement = findMappedViewElement( listItem, mapper, model ); - // Unwrap element from current list wrappers. - unwrapListItemBlock( viewElement, strategy, writer ); - // Then wrap them with the new list wrappers. wrapListItemBlock( listItem, writer.createRangeOn( viewElement ), strategy, writer ); }; } -// Unwraps all ol, ul, and li attribute elements that are wrapping the provided view element. -function unwrapListItemBlock( viewElement, strategy, writer ) { - let attributeElement = viewElement.parent; - - while ( attributeElement.is( 'attributeElement' ) && [ 'ul', 'ol', 'li' ].includes( attributeElement.name ) ) { - const parentElement = attributeElement.parent; - const attributeValue = strategy.getAttributeOnUpcast( attributeElement ); - - // Unwrap only if the model attribute is really downcasted (it's not the default value). - if ( isListView( attributeElement ) && attributeValue !== strategy.defaultValue ) { - // Make a clone of an attribute element that only includes properties of list styles. - const element = writer.createAttributeElement( attributeElement.name, null, { - priority: attributeElement.priority, - id: attributeElement.id - } ); - - strategy.setAttributeOnDowncast( writer, attributeValue, element ); - writer.unwrap( writer.createRangeOn( viewElement ), element ); - } - - attributeElement = parentElement; - } -} - // Wraps the given list item with appropriate attribute elements for ul, ol, and li. function wrapListItemBlock( listItem, viewRange, strategy, writer ) { if ( !listItem.hasAttribute( 'listIndent' ) ) { From 74260f858712c47d50f3209ef5d36daaaa83fb92 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Wed, 23 Feb 2022 14:18:01 +0100 Subject: [PATCH 07/25] Cleaning manual test. --- .../tests/manual/documentlist-properties.html | 16 ++++++++-------- .../tests/manual/documentlist-properties.js | 16 ++++++++-------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/packages/ckeditor5-list/tests/manual/documentlist-properties.html b/packages/ckeditor5-list/tests/manual/documentlist-properties.html index c5e9823e7ae..1c4ac27e57c 100644 --- a/packages/ckeditor5-list/tests/manual/documentlist-properties.html +++ b/packages/ckeditor5-list/tests/manual/documentlist-properties.html @@ -1,7 +1,7 @@

With styles and other properties

Styles + Start index + Reversed

-
+
  1. a
  2. b
    x
    3. @@ -26,7 +26,7 @@

    Styles + Start index + Reversed

Styles + Start index

-
+
  1. a
  2. b
    x
    3. @@ -51,7 +51,7 @@

    Styles + Start index

Styles + Reversed

-
+
  1. a
  2. b
    x
    3. @@ -78,7 +78,7 @@

    Styles + Reversed

    Without styles, just extra properties

    Start index + Reversed

    -
    +
    1. a
    2. b
      x
      3. @@ -103,7 +103,7 @@

      Start index + Reversed

    Start index

    -
    +
    1. a
    2. b
      x
      3. @@ -128,7 +128,7 @@

      Start index

    Reversed

    -
    +
    1. a
    2. b
      x
      3. @@ -154,7 +154,7 @@

      Reversed

      Just styles

      -
      +
      1. a
      2. b
        x
        3. @@ -180,7 +180,7 @@

        Just styles

        No properties enabled

        -
        +
        1. a
        2. b
          x
          3. diff --git a/packages/ckeditor5-list/tests/manual/documentlist-properties.js b/packages/ckeditor5-list/tests/manual/documentlist-properties.js index 0a404841bc7..6f7beec6f91 100644 --- a/packages/ckeditor5-list/tests/manual/documentlist-properties.js +++ b/packages/ckeditor5-list/tests/manual/documentlist-properties.js @@ -121,49 +121,49 @@ function createEditor( idSuffix, properties ) { } ); } -createEditor( 'a', { +createEditor( 'all', { styles: true, startIndex: true, reversed: true } ); -createEditor( 'b', { +createEditor( 'style-start', { styles: true, startIndex: true, reversed: false } ); -createEditor( 'c', { +createEditor( 'style-reversed', { styles: true, startIndex: false, reversed: true } ); -createEditor( 'd', { +createEditor( 'start-reversed', { styles: false, startIndex: true, reversed: true } ); -createEditor( 'e', { +createEditor( 'start', { styles: false, startIndex: true, reversed: false } ); -createEditor( 'f', { +createEditor( 'reversed', { styles: false, startIndex: false, reversed: true } ); -createEditor( 'g', { +createEditor( 'style', { styles: true, startIndex: false, reversed: false } ); -createEditor( 'h', { +createEditor( 'none', { styles: false, startIndex: false, reversed: false From 60952088700ca8857652fb5e9ecaa686855b6cc1 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Mon, 28 Feb 2022 20:51:56 +0100 Subject: [PATCH 08/25] WiP. --- .../src/view/downcastwriter.js | 2 +- .../src/documentlist/converters.js | 26 ++-- .../src/documentlist/documentlistediting.js | 127 ++++++++++++++---- 3 files changed, 110 insertions(+), 45 deletions(-) diff --git a/packages/ckeditor5-engine/src/view/downcastwriter.js b/packages/ckeditor5-engine/src/view/downcastwriter.js index b85bafbfa10..906e34fe641 100644 --- a/packages/ckeditor5-engine/src/view/downcastwriter.js +++ b/packages/ckeditor5-engine/src/view/downcastwriter.js @@ -673,7 +673,7 @@ export default class DowncastWriter { nodeBefore._appendChild( nodeAfter.getChildren() ); // Merge the element attributes in case those have a same ID (other attributes might differ). - this._mergeElementAttributes( nodeAfter, nodeBefore ); + // this._mergeElementAttributes( nodeAfter, nodeBefore ); nodeAfter._remove(); this._removeFromClonedElementsGroup( nodeAfter ); diff --git a/packages/ckeditor5-list/src/documentlist/converters.js b/packages/ckeditor5-list/src/documentlist/converters.js index 309e0b85338..d53fd376f05 100644 --- a/packages/ckeditor5-list/src/documentlist/converters.js +++ b/packages/ckeditor5-list/src/documentlist/converters.js @@ -106,14 +106,12 @@ export function listUpcastCleanList() { * @protected * @param {module:engine/model/model~Model} model The editor model. * @param {module:engine/controller/editingcontroller~EditingController} editing The editing controller. - * @param {module:utils/emittermixin~Emitter} emitter The emitter that will fire events for checking whether given - * view element requires refresh. + * @param {module:list/documentlist/documentlistediting~DocumentListEditing} documentListEditing The document list editing plugin. * @return {Function} */ -export function reconvertItemsOnDataChange( model, editing, emitter ) { +export function reconvertItemsOnDataChange( model, editing, documentListEditing ) { return () => { const changes = model.document.differ.getChanges(); - const itemsToRefresh = []; const itemToListHead = new Map(); const changedItems = new Set(); @@ -144,7 +142,7 @@ export function reconvertItemsOnDataChange( model, editing, emitter ) { // Check if paragraph should be converted from bogus to plain paragraph. if ( doesItemParagraphRequiresRefresh( item ) ) { - itemsToRefresh.push( item ); + editing.reconvertItem( item ); } } else { changedItems.add( item ); @@ -153,23 +151,18 @@ export function reconvertItemsOnDataChange( model, editing, emitter ) { // Some other attribute was changed on the list item, // check if paragraph does not need to be converted to bogus or back. if ( doesItemParagraphRequiresRefresh( item ) ) { - itemsToRefresh.push( item ); + editing.reconvertItem( item ); } } } } for ( const listHead of itemToListHead.values() ) { - itemsToRefresh.push( ...collectListItemsToRefresh( listHead, changedItems ) ); - } - - for ( const item of new Set( itemsToRefresh ) ) { - editing.reconvertItem( item ); + collectListItemsToRefresh( listHead, changedItems ); } }; function collectListItemsToRefresh( listHead, changedItems ) { - const itemsToRefresh = []; const visited = new Set(); const stack = []; @@ -199,16 +192,14 @@ export function reconvertItemsOnDataChange( model, editing, emitter ) { // Check if bogus vs plain paragraph needs refresh. if ( doesItemParagraphRequiresRefresh( block, blocks ) ) { - itemsToRefresh.push( block ); + editing.reconvertItem( block ); } // Check if wrapping with UL, OL, LIs needs refresh. else if ( doesItemWrappingRequiresRefresh( block, stack, changedItems ) ) { - itemsToRefresh.push( block ); + documentListEditing._markToReWrapNode( block ); } } } - - return itemsToRefresh; } function doesItemParagraphRequiresRefresh( item, blocks ) { @@ -255,7 +246,8 @@ export function reconvertItemsOnDataChange( model, editing, emitter ) { continue; } - const needsRefresh = emitter.fire( `checkAttributes:${ isListItemElement ? 'item' : 'list' }`, { + const eventName = `checkAttributes:${ isListItemElement ? 'item' : 'list' }`; + const needsRefresh = documentListEditing.fire( eventName, { viewElement: element, modelAttributes: stack[ indent ] } ); diff --git a/packages/ckeditor5-list/src/documentlist/documentlistediting.js b/packages/ckeditor5-list/src/documentlist/documentlistediting.js index 1ff6fda8983..1e1964f0ebb 100644 --- a/packages/ckeditor5-list/src/documentlist/documentlistediting.js +++ b/packages/ckeditor5-list/src/documentlist/documentlistediting.js @@ -77,6 +77,14 @@ export default class DocumentListEditing extends Plugin { */ this._sameListDefiningAttributes = [ 'listType' ]; + /** + * TODO + * + * @private + * @type {Set.} + */ + this._nodesToRewrap = new Set(); + const editor = this.editor; const model = editor.model; @@ -157,6 +165,15 @@ export default class DocumentListEditing extends Plugin { return this._sameListDefiningAttributes; } + /** + * TODO + * + * @protected + */ + _markToReWrapNode( node ) { + this._nodesToRewrap.add( node ); + } + /** * Attaches the listener to the {@link module:engine/view/document~Document#event:delete} event and handles backspace/delete * keys in and around document lists. @@ -364,6 +381,15 @@ export default class DocumentListEditing extends Plugin { converterPriority: 'high' } ); + const attributeNames = [ + ...LIST_BASE_ATTRIBUTES, + 'listStyle', + 'listStart', + 'listReversed', + 'htmlListAttributes', + 'htmlLiAttributes' + ]; + editor.conversion.for( 'downcast' ) .add( dispatcher => { for ( const attributeName of LIST_BASE_ATTRIBUTES ) { @@ -372,50 +398,85 @@ export default class DocumentListEditing extends Plugin { dispatcher.on( 'reduceChanges', ( evt, data ) => { const reducedChanges = []; - const attributeNames = [ - ...LIST_BASE_ATTRIBUTES, - 'listStyle', - 'listStart', - 'listReversed', - 'htmlListAttributes', - 'htmlLiAttributes' - ]; const reWrappedItems = new Set(); + const rangesToReWrap = Array.from( this._nodesToRewrap.values() ) + .map( node => model.createRange( model.createPositionBefore( node ), model.createPositionAt( node, 0 ) ) ) + .sort( compareRangeOrder ); + let rangesToReWrapIndex = 0; + for ( const change of data.changes ) { + const position = change.position || change.range.start; + + while ( rangesToReWrapIndex < rangesToReWrap.length ) { + const range = rangesToReWrap[ rangesToReWrapIndex ]; + + if ( !position.isAfter( range.start ) ) { + break; + } + + const node = range.start.nodeAfter; + + if ( !reWrappedItems.has( node ) ) { + reWrappedItems.add( node ); + reducedChanges.push( ...createChanges( range, node ) ); + } + + rangesToReWrapIndex++; + } + if ( change.type == 'attribute' && attributeNames.includes( change.attributeKey ) ) { - const node = change.range.start.nodeAfter; + const node = position.nodeAfter; if ( reWrappedItems.has( node ) ) { continue; } reWrappedItems.add( node ); - - reducedChanges.push( { - type: 'attribute', - attributeKey: 'listIndent', - attributeOldValue: node.getAttribute( 'listIndent' ), - attributeNewValue: null, - range: change.range - } ); - - for ( const key of attributeNames ) { - reducedChanges.push( { - type: 'attribute', - attributeKey: key, - attributeOldValue: null, - attributeNewValue: node.getAttribute( key ), - range: change.range - } ); - } + reducedChanges.push( ...createChanges( change.range, node ) ); } else { reducedChanges.push( change ); } } + while ( rangesToReWrapIndex < rangesToReWrap.length ) { + const range = rangesToReWrap[ rangesToReWrapIndex ]; + const node = range.start.nodeAfter; + + if ( !reWrappedItems.has( node ) ) { + reWrappedItems.add( node ); + reducedChanges.push( ...createChanges( range, node ) ); + } + + rangesToReWrapIndex++; + } + data.changes = reducedChanges; } ); + + function createChanges( range, node ) { + const changes = [ { + type: 'attribute', + attributeKey: 'listIndent', + attributeOldValue: node.getAttribute( 'listIndent' ), + attributeNewValue: null, + range + } ]; + + for ( const [ attributeKey, attributeValue ] of node.getAttributes() ) { + if ( attributeNames.includes( attributeKey ) ) { + changes.push( { + type: 'attribute', + attributeKey, + attributeOldValue: null, + attributeNewValue: attributeValue, + range + } ); + } + } + + return changes; + } } ); this.listenTo( model.document, 'change:data', reconvertItemsOnDataChange( model, editor.editing, this ) ); @@ -653,3 +714,15 @@ function shouldMergeOnBlocksContentLevel( model, direction ) { return isSingleListItem( [ positionParent, previousSibling ] ); } + +// TODO this is copy from tableutils - should be extracted to utils package. +function compareRangeOrder( rangeA, rangeB ) { + // Since table cell ranges are disjoint, it's enough to check their start positions. + const posA = rangeA.start; + const posB = rangeB.start; + + // Checking for equal position (returning 0) is not needed because this would be either: + // a. Intersecting range (not allowed by model) + // b. Collapsed range on the same position (allowed by model but should not happen). + return posA.isBefore( posB ) ? -1 : 1; +} From 209a4d4e5f607a1e2f603e485263e8aa68c231a1 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Tue, 1 Mar 2022 21:12:05 +0100 Subject: [PATCH 09/25] Moved down-cast conversion of list attributes to the single converter with multiple strategies. --- .../src/integrations/documentlist.js | 137 +++++------------- .../tests/manual/documentlist.html | 2 - .../src/documentlist/converters.js | 63 ++++---- .../src/documentlist/documentlistediting.js | 120 ++------------- .../src/documentlistproperties/converters.js | 68 --------- .../documentlistpropertiesediting.js | 17 +-- 6 files changed, 88 insertions(+), 319 deletions(-) diff --git a/packages/ckeditor5-html-support/src/integrations/documentlist.js b/packages/ckeditor5-html-support/src/integrations/documentlist.js index 6c0afbe5492..3e24a7dce39 100644 --- a/packages/ckeditor5-html-support/src/integrations/documentlist.js +++ b/packages/ckeditor5-html-support/src/integrations/documentlist.js @@ -11,10 +11,6 @@ import { Plugin } from 'ckeditor5/src/core'; import { setViewAttributes } from '../conversionutils.js'; import DataFilter from '../datafilter'; -import { LIST_BASE_ATTRIBUTES } from '@ckeditor/ckeditor5-list/src/documentlist/utils/model'; -import { findMappedViewElement } from '@ckeditor/ckeditor5-list/src/documentlist/converters'; -import { createListElement, createListItemElement, isListView } from '@ckeditor/ckeditor5-list/src/documentlist/utils/view'; -import ListWalker from '@ckeditor/ckeditor5-list/src/documentlist/utils/listwalker'; /** * Provides the General HTML Support integration with {@link module:list/documentlist~DocumentList Document List} feature. @@ -40,18 +36,21 @@ export default class DocumentListElementSupport extends Plugin { const dataFilter = this.editor.plugins.get( DataFilter ); dataFilter.on( 'register:li', registerFilter( { + scope: 'item', attributeName: 'htmlLiAttributes', - viewElementNames: [ 'li' ] + setAttributeOnDowncast: setViewAttributes }, this.editor ) ); dataFilter.on( 'register:ul', registerFilter( { + scope: 'list', attributeName: 'htmlListAttributes', - viewElementNames: [ 'ul', 'ol' ] + setAttributeOnDowncast: setViewAttributes }, this.editor ) ); dataFilter.on( 'register:ol', registerFilter( { + scope: 'list', attributeName: 'htmlListAttributes', - viewElementNames: [ 'ul', 'ol' ] + setAttributeOnDowncast: setViewAttributes }, this.editor ) ); } } @@ -65,9 +64,9 @@ function registerFilter( strategy, editor ) { const dataFilter = editor.plugins.get( DataFilter ); return evt => { - if ( schema.checkAttribute( '$block', attributeName ) ) { - evt.stop(); + evt.stop(); + if ( schema.checkAttribute( '$block', attributeName ) ) { return; } @@ -76,10 +75,17 @@ function registerFilter( strategy, editor ) { schema.extend( '$blockObject', { allowAttributes: [ attributeName ] } ); schema.extend( '$container', { allowAttributes: [ attributeName ] } ); - conversion.for( 'upcast' ).add( viewToModelListAttributeConverter( strategy, dataFilter ) ); - conversion.for( 'downcast' ).add( modelToViewListAttributeConverter( strategy, editor.model ) ); + conversion.for( 'upcast' ).add( dispatcher => { + if ( strategy.scope == 'list' ) { + dispatcher.on( 'element:ul', viewToModelListAttributeConverter( strategy, dataFilter ), { priority: 'low' } ); + dispatcher.on( 'element:ol', viewToModelListAttributeConverter( strategy, dataFilter ), { priority: 'low' } ); + } else /* if ( strategy.scope == 'item' ) */ { + dispatcher.on( 'element:li', viewToModelListAttributeConverter( strategy, dataFilter ), { priority: 'low' } ); + } + } ); - evt.stop(); + // Register downcast strategy. + editor.plugins.get( 'DocumentListEditing' ).registerDowncastStrategy( strategy ); }; } @@ -94,103 +100,30 @@ function registerFilter( strategy, editor ) { // @param {module:html-support/datafilter~DataFilter} dataFilter // @returns {Function} Returns a conversion callback. function viewToModelListAttributeConverter( strategy, dataFilter ) { - const { attributeName, viewElementNames } = strategy; - - return dispatcher => { - dispatcher.on( 'element', ( evt, data, conversionApi ) => { - const viewElement = data.viewItem; - - if ( !viewElement.is( 'element' ) || !viewElementNames.includes( viewElement.name ) ) { - return; - } - - if ( !data.modelRange ) { - Object.assign( data, conversionApi.convertChildren( data.viewItem, data.modelCursor ) ); - } + const { attributeName } = strategy; - const viewAttributes = dataFilter._consumeAllowedAttributes( viewElement, conversionApi ); + return ( evt, data, conversionApi ) => { + const viewElement = data.viewItem; - for ( const item of data.modelRange.getItems( { shallow: true } ) ) { - // Apply only to list item blocks. - if ( !item.hasAttribute( 'listItemId' ) ) { - continue; - } + if ( !data.modelRange ) { + Object.assign( data, conversionApi.convertChildren( data.viewItem, data.modelCursor ) ); + } - // Set list attributes only on same level items, those nested deeper are already handled - // by the recursive conversion. - if ( item.hasAttribute( attributeName ) ) { - continue; - } + const viewAttributes = dataFilter._consumeAllowedAttributes( viewElement, conversionApi ); - conversionApi.writer.setAttribute( attributeName, viewAttributes || {}, item ); + for ( const item of data.modelRange.getItems( { shallow: true } ) ) { + // Apply only to list item blocks. + if ( !item.hasAttribute( 'listItemId' ) ) { + continue; } - }, { priority: 'low' } ); - }; -} -// Model-to-view conversion helper applying attributes from {@link module:code-block/codeblock~CodeBlock Code Block} -// feature model element. -// -// @private -// @param {String} attributeName -// @returns {Function} Returns a conversion callback. -function modelToViewListAttributeConverter( strategy, model ) { - return dispatcher => { - dispatcher.on( `attribute:${ strategy.attributeName }`, ( evt, data, conversionApi ) => { - const { writer, mapper, consumable } = conversionApi; - const listItem = data.item; - - if ( !consumable.consume( listItem, evt.name ) ) { - return; + // Set list attributes only on same level items, those nested deeper are already handled + // by the recursive conversion. + if ( item.hasAttribute( attributeName ) ) { + continue; } - // Use positions mapping instead of mapper.toViewElement( listItem ) to find outermost view element. - // This is for cases when mapping is using inner view element like in the code blocks (pre > code). - const viewElement = findMappedViewElement( listItem, mapper, model ); - - // Then wrap them with the new list wrappers. - wrapListItemBlock( listItem, writer.createRangeOn( viewElement ), strategy, writer ); - } ); - }; -} - -// Wraps the given list item with appropriate attribute elements for ul, ol, and li. -function wrapListItemBlock( listItem, viewRange, strategy, writer ) { - if ( !listItem.hasAttribute( 'listIndent' ) ) { - return; - } - - const listItemIndent = listItem.getAttribute( 'listIndent' ); - let listType = listItem.getAttribute( 'listType' ); - let listItemId = listItem.getAttribute( 'listItemId' ); - let htmlAttributes = listItem.getAttribute( strategy.attributeName ); - - let currentListItem = listItem; - - for ( let indent = listItemIndent; indent >= 0; indent-- ) { - if ( htmlAttributes ) { - const viewElement = strategy.viewElementNames.includes( 'li' ) ? - createListItemElement( writer, indent, listItemId ) : - createListElement( writer, indent, listType ); - - setViewAttributes( writer, htmlAttributes, viewElement ); - viewRange = writer.wrap( viewRange, viewElement ); - } - - if ( indent == 0 ) { - break; + conversionApi.writer.setAttribute( attributeName, viewAttributes || {}, item ); } - - currentListItem = ListWalker.first( currentListItem, { lowerIndent: true } ); - - // There is no list item with lower indent, this means this is a document fragment containing - // only a part of nested list (like copy to clipboard) so we don't need to try to wrap it further. - if ( !currentListItem ) { - break; - } - - listType = currentListItem.getAttribute( 'listType' ); - listItemId = currentListItem.getAttribute( 'listItemId' ); - htmlAttributes = currentListItem.getAttribute( strategy.attributeName ); - } + }; } diff --git a/packages/ckeditor5-html-support/tests/manual/documentlist.html b/packages/ckeditor5-html-support/tests/manual/documentlist.html index f47a9a67096..9b99dc92928 100644 --- a/packages/ckeditor5-html-support/tests/manual/documentlist.html +++ b/packages/ckeditor5-html-support/tests/manual/documentlist.html @@ -7,13 +7,11 @@

          • foo

            -
        diff --git a/packages/ckeditor5-list/src/documentlist/converters.js b/packages/ckeditor5-list/src/documentlist/converters.js index d53fd376f05..c4e164391a5 100644 --- a/packages/ckeditor5-list/src/documentlist/converters.js +++ b/packages/ckeditor5-list/src/documentlist/converters.js @@ -112,6 +112,7 @@ export function listUpcastCleanList() { export function reconvertItemsOnDataChange( model, editing, documentListEditing ) { return () => { const changes = model.document.differ.getChanges(); + const itemsToRefresh = []; const itemToListHead = new Map(); const changedItems = new Set(); @@ -142,7 +143,7 @@ export function reconvertItemsOnDataChange( model, editing, documentListEditing // Check if paragraph should be converted from bogus to plain paragraph. if ( doesItemParagraphRequiresRefresh( item ) ) { - editing.reconvertItem( item ); + itemsToRefresh.push( item ); } } else { changedItems.add( item ); @@ -151,18 +152,23 @@ export function reconvertItemsOnDataChange( model, editing, documentListEditing // Some other attribute was changed on the list item, // check if paragraph does not need to be converted to bogus or back. if ( doesItemParagraphRequiresRefresh( item ) ) { - editing.reconvertItem( item ); + itemsToRefresh.push( item ); } } } } for ( const listHead of itemToListHead.values() ) { - collectListItemsToRefresh( listHead, changedItems ); + itemsToRefresh.push( ...collectListItemsToRefresh( listHead, changedItems ) ); + } + + for ( const item of new Set( itemsToRefresh ) ) { + editing.reconvertItem( item ); } }; function collectListItemsToRefresh( listHead, changedItems ) { + const itemsToRefresh = []; const visited = new Set(); const stack = []; @@ -192,14 +198,16 @@ export function reconvertItemsOnDataChange( model, editing, documentListEditing // Check if bogus vs plain paragraph needs refresh. if ( doesItemParagraphRequiresRefresh( block, blocks ) ) { - editing.reconvertItem( block ); + itemsToRefresh.push( block ); } // Check if wrapping with UL, OL, LIs needs refresh. else if ( doesItemWrappingRequiresRefresh( block, stack, changedItems ) ) { - documentListEditing._markToReWrapNode( block ); + itemsToRefresh.push( block ); } } } + + return itemsToRefresh; } function doesItemParagraphRequiresRefresh( item, blocks ) { @@ -275,10 +283,11 @@ export function reconvertItemsOnDataChange( model, editing, documentListEditing * * @protected * @param {Array.} attributes A list of attribute names that should be converted if are set. + * @param {TODO} strategies TODO * @param {module:engine/model/model~Model} model The model. * @returns {Function} */ -export function listItemDowncastConverter( attributes, model ) { +export function listItemDowncastConverter( attributes, strategies, model ) { const consumer = createAttributesConsumer( attributes ); return ( evt, data, conversionApi ) => { @@ -295,13 +304,11 @@ export function listItemDowncastConverter( attributes, model ) { // This is for cases when mapping is using inner view element like in the code blocks (pre > code). const viewElement = findMappedViewElement( listItem, mapper, model ); - if ( data.attributeKey == 'listIndent' && data.attributeNewValue === null ) { - // Unwrap element from current list wrappers. - unwrapListItemBlock( viewElement, writer ); - } else { - // Then wrap them with the new list wrappers. - wrapListItemBlock( listItem, writer.createRangeOn( viewElement ), writer ); - } + // Unwrap element from current list wrappers. + unwrapListItemBlock( viewElement, writer ); + + // Then wrap them with the new list wrappers. + wrapListItemBlock( listItem, writer.createRangeOn( viewElement ), strategies, writer ); }; } @@ -354,33 +361,34 @@ function unwrapListItemBlock( viewElement, viewWriter ) { while ( attributeElement.is( 'attributeElement' ) && [ 'ul', 'ol', 'li' ].includes( attributeElement.name ) ) { const parentElement = attributeElement.parent; - // Make a clone of an attribute element that only includes properties of generic list (i.e., without list styles). - const element = viewWriter.createAttributeElement( attributeElement.name, null, { - priority: attributeElement.priority, - id: attributeElement.id - } ); - - viewWriter.unwrap( viewWriter.createRangeOn( viewElement ), element ); + viewWriter.unwrap( viewWriter.createRangeOn( viewElement ), attributeElement ); attributeElement = parentElement; } } // Wraps the given list item with appropriate attribute elements for ul, ol, and li. -function wrapListItemBlock( listItem, viewRange, writer ) { +function wrapListItemBlock( listItem, viewRange, strategies, writer ) { if ( !listItem.hasAttribute( 'listIndent' ) ) { return; } const listItemIndent = listItem.getAttribute( 'listIndent' ); - let listItemId = listItem.getAttribute( 'listItemId' ); - let listType = listItem.getAttribute( 'listType' ); - let currentListItem = listItem; for ( let indent = listItemIndent; indent >= 0; indent-- ) { - const listItemViewElement = createListItemElement( writer, indent, listItemId ); - const listViewElement = createListElement( writer, indent, listType ); + const listItemViewElement = createListItemElement( writer, indent, currentListItem.getAttribute( 'listItemId' ) ); + const listViewElement = createListElement( writer, indent, currentListItem.getAttribute( 'listType' ) ); + + for ( const strategy of strategies ) { + if ( currentListItem.hasAttribute( strategy.attributeName ) ) { + strategy.setAttributeOnDowncast( + writer, + currentListItem.getAttribute( strategy.attributeName ), + strategy.scope == 'list' ? listViewElement : listItemViewElement + ); + } + } viewRange = writer.wrap( viewRange, listItemViewElement ); viewRange = writer.wrap( viewRange, listViewElement ); @@ -396,9 +404,6 @@ function wrapListItemBlock( listItem, viewRange, writer ) { if ( !currentListItem ) { break; } - - listItemId = currentListItem.getAttribute( 'listItemId' ); - listType = currentListItem.getAttribute( 'listType' ); } } diff --git a/packages/ckeditor5-list/src/documentlist/documentlistediting.js b/packages/ckeditor5-list/src/documentlist/documentlistediting.js index 1e1964f0ebb..5c520196d9a 100644 --- a/packages/ckeditor5-list/src/documentlist/documentlistediting.js +++ b/packages/ckeditor5-list/src/documentlist/documentlistediting.js @@ -81,9 +81,8 @@ export default class DocumentListEditing extends Plugin { * TODO * * @private - * @type {Set.} */ - this._nodesToRewrap = new Set(); + this._downcastStrategies = []; const editor = this.editor; const model = editor.model; @@ -118,7 +117,6 @@ export default class DocumentListEditing extends Plugin { editor.commands.add( 'splitListItemAfter', new DocumentListSplitCommand( editor, 'after' ) ); this._setupModelPostFixing(); - this._setupConversion(); this._setupDeleteIntegration(); this._setupEnterIntegration(); this._setupTabIntegration(); @@ -144,6 +142,9 @@ export default class DocumentListEditing extends Plugin { // First we want to allow user to outdent all indendations from other features then he can oudent list item. outdent.registerChildCommand( commands.get( 'outdentList' ), { priority: 'lowest' } ); } + + // Register conversion after other plugins had a chance to register their attributes. + this._setupConversion(); } /** @@ -167,11 +168,9 @@ export default class DocumentListEditing extends Plugin { /** * TODO - * - * @protected */ - _markToReWrapNode( node ) { - this._nodesToRewrap.add( node ); + registerDowncastStrategy( strategy ) { + this._downcastStrategies.push( strategy ); } /** @@ -383,99 +382,16 @@ export default class DocumentListEditing extends Plugin { const attributeNames = [ ...LIST_BASE_ATTRIBUTES, - 'listStyle', - 'listStart', - 'listReversed', - 'htmlListAttributes', - 'htmlLiAttributes' + ...this._downcastStrategies.map( strategy => strategy.attributeName ) ]; editor.conversion.for( 'downcast' ) .add( dispatcher => { - for ( const attributeName of LIST_BASE_ATTRIBUTES ) { - dispatcher.on( `attribute:${ attributeName }`, listItemDowncastConverter( LIST_BASE_ATTRIBUTES, model ) ); - } - - dispatcher.on( 'reduceChanges', ( evt, data ) => { - const reducedChanges = []; - const reWrappedItems = new Set(); - - const rangesToReWrap = Array.from( this._nodesToRewrap.values() ) - .map( node => model.createRange( model.createPositionBefore( node ), model.createPositionAt( node, 0 ) ) ) - .sort( compareRangeOrder ); - let rangesToReWrapIndex = 0; - - for ( const change of data.changes ) { - const position = change.position || change.range.start; - - while ( rangesToReWrapIndex < rangesToReWrap.length ) { - const range = rangesToReWrap[ rangesToReWrapIndex ]; - - if ( !position.isAfter( range.start ) ) { - break; - } - - const node = range.start.nodeAfter; - - if ( !reWrappedItems.has( node ) ) { - reWrappedItems.add( node ); - reducedChanges.push( ...createChanges( range, node ) ); - } - - rangesToReWrapIndex++; - } - - if ( change.type == 'attribute' && attributeNames.includes( change.attributeKey ) ) { - const node = position.nodeAfter; - - if ( reWrappedItems.has( node ) ) { - continue; - } - - reWrappedItems.add( node ); - reducedChanges.push( ...createChanges( change.range, node ) ); - } else { - reducedChanges.push( change ); - } - } - - while ( rangesToReWrapIndex < rangesToReWrap.length ) { - const range = rangesToReWrap[ rangesToReWrapIndex ]; - const node = range.start.nodeAfter; - - if ( !reWrappedItems.has( node ) ) { - reWrappedItems.add( node ); - reducedChanges.push( ...createChanges( range, node ) ); - } - - rangesToReWrapIndex++; - } - - data.changes = reducedChanges; - } ); - - function createChanges( range, node ) { - const changes = [ { - type: 'attribute', - attributeKey: 'listIndent', - attributeOldValue: node.getAttribute( 'listIndent' ), - attributeNewValue: null, - range - } ]; - - for ( const [ attributeKey, attributeValue ] of node.getAttributes() ) { - if ( attributeNames.includes( attributeKey ) ) { - changes.push( { - type: 'attribute', - attributeKey, - attributeOldValue: null, - attributeNewValue: attributeValue, - range - } ); - } - } - - return changes; + for ( const attributeName of attributeNames ) { + dispatcher.on( + `attribute:${ attributeName }`, + listItemDowncastConverter( attributeNames, this._downcastStrategies, model ) + ); } } ); @@ -714,15 +630,3 @@ function shouldMergeOnBlocksContentLevel( model, direction ) { return isSingleListItem( [ positionParent, previousSibling ] ); } - -// TODO this is copy from tableutils - should be extracted to utils package. -function compareRangeOrder( rangeA, rangeB ) { - // Since table cell ranges are disjoint, it's enough to check their start positions. - const posA = rangeA.start; - const posB = rangeB.start; - - // Checking for equal position (returning 0) is not needed because this would be either: - // a. Intersecting range (not allowed by model) - // b. Collapsed range on the same position (allowed by model but should not happen). - return posA.isBefore( posB ) ? -1 : 1; -} diff --git a/packages/ckeditor5-list/src/documentlistproperties/converters.js b/packages/ckeditor5-list/src/documentlistproperties/converters.js index b2fdab280e9..dce1c55a208 100644 --- a/packages/ckeditor5-list/src/documentlistproperties/converters.js +++ b/packages/ckeditor5-list/src/documentlistproperties/converters.js @@ -7,10 +7,6 @@ * @module list/documentlistproperties/converters */ -import ListWalker from '../documentlist/utils/listwalker'; -import { findMappedViewElement } from '../documentlist/converters'; -import { createListElement } from '../documentlist/utils/view'; - /** * Returns a converter that consumes the `style`, `reversed`, and `start` attributes. * In `style`, it searches for the `list-style-type` definition. @@ -59,67 +55,3 @@ export function listPropertiesUpcastConverter( strategy ) { } }; } - -/** - * Returns a converter that adds `reversed`, `start` attributes and adds `list-style-type` definition as a value for the `style` attribute. - * The `"default"` values are removed and not present in the view/data. - * - * @protected - * @param {module:list/documentlistproperties/documentlistpropertiesediting~AttributeStrategy} strategy - * @param {module:engine/model/model~Model} model The model. - * @returns {Function} - */ -export function listPropertiesDowncastConverter( strategy, model ) { - return ( evt, data, conversionApi ) => { - const { writer, mapper, consumable } = conversionApi; - const listItem = data.item; - - if ( !consumable.consume( listItem, evt.name ) ) { - return; - } - - // Use positions mapping instead of mapper.toViewElement( listItem ) to find outermost view element. - // This is for cases when mapping is using inner view element like in the code blocks (pre > code). - const viewElement = findMappedViewElement( listItem, mapper, model ); - - // Then wrap them with the new list wrappers. - wrapListItemBlock( listItem, writer.createRangeOn( viewElement ), strategy, writer ); - }; -} - -// Wraps the given list item with appropriate attribute elements for ul, ol, and li. -function wrapListItemBlock( listItem, viewRange, strategy, writer ) { - if ( !listItem.hasAttribute( 'listIndent' ) ) { - return; - } - - const listItemIndent = listItem.getAttribute( 'listIndent' ); - let listType = listItem.getAttribute( 'listType' ); - let listProperty = listItem.getAttribute( strategy.attributeName ); - - let currentListItem = listItem; - - for ( let indent = listItemIndent; indent >= 0; indent-- ) { - if ( strategy.appliesToListItem( currentListItem ) ) { - const listViewElement = createListElement( writer, indent, listType ); - - strategy.setAttributeOnDowncast( writer, listProperty, listViewElement ); - viewRange = writer.wrap( viewRange, listViewElement ); - } - - if ( indent == 0 ) { - break; - } - - currentListItem = ListWalker.first( currentListItem, { lowerIndent: true } ); - - // There is no list item with lower indent, this means this is a document fragment containing - // only a part of nested list (like copy to clipboard) so we don't need to try to wrap it further. - if ( !currentListItem ) { - break; - } - - listType = currentListItem.getAttribute( 'listType' ); - listProperty = currentListItem.getAttribute( strategy.attributeName ); - } -} diff --git a/packages/ckeditor5-list/src/documentlistproperties/documentlistpropertiesediting.js b/packages/ckeditor5-list/src/documentlistproperties/documentlistpropertiesediting.js index 2ee784089c2..cfca2fe91e3 100644 --- a/packages/ckeditor5-list/src/documentlistproperties/documentlistpropertiesediting.js +++ b/packages/ckeditor5-list/src/documentlistproperties/documentlistpropertiesediting.js @@ -13,10 +13,7 @@ import DocumentListEditing from '../documentlist/documentlistediting'; import DocumentListStartCommand from './documentliststartcommand'; import DocumentListStyleCommand from './documentliststylecommand'; import DocumentListReversedCommand from './documentlistreversedcommand'; -import { - listPropertiesDowncastConverter, - listPropertiesUpcastConverter -} from './converters'; +import { listPropertiesUpcastConverter } from './converters'; import { iterateSiblingListBlocks } from '../documentlist/utils/listwalker'; import { getListTypeFromListStyleType } from './utils/style'; @@ -76,6 +73,9 @@ export default class DocumentListPropertiesEditing extends Plugin { model.schema.extend( '$container', { allowAttributes: strategy.attributeName } ); model.schema.extend( '$block', { allowAttributes: strategy.attributeName } ); model.schema.extend( '$blockObject', { allowAttributes: strategy.attributeName } ); + + // Register downcast strategy. + documentListEditing.registerDowncastStrategy( strategy ); } // Set up conversion. @@ -86,12 +86,6 @@ export default class DocumentListPropertiesEditing extends Plugin { } } ); - editor.conversion.for( 'downcast' ).add( dispatcher => { - for ( const strategy of strategies ) { - dispatcher.on( `attribute:${ strategy.attributeName }`, listPropertiesDowncastConverter( strategy, model ) ); - } - } ); - // Verify if the list view element (ul or ol) requires refreshing. documentListEditing.on( 'checkAttributes:list', ( evt, { viewElement, modelAttributes } ) => { for ( const strategy of strategies ) { @@ -210,6 +204,7 @@ function createAttributeStrategies( enabledProperties ) { if ( enabledProperties.styles ) { strategies.push( { + scope: 'list', attributeName: 'listStyle', defaultValue: DEFAULT_LIST_TYPE, viewConsumables: { styles: 'list-style-type' }, @@ -252,6 +247,7 @@ function createAttributeStrategies( enabledProperties ) { if ( enabledProperties.reversed ) { strategies.push( { + scope: 'list', attributeName: 'listReversed', defaultValue: false, viewConsumables: { attributes: 'reversed' }, @@ -284,6 +280,7 @@ function createAttributeStrategies( enabledProperties ) { if ( enabledProperties.startIndex ) { strategies.push( { + scope: 'list', attributeName: 'listStart', defaultValue: 1, viewConsumables: { attributes: 'start' }, From e751b65f27367cefd9de581c04e7a4df3d6c751a Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Wed, 2 Mar 2022 16:42:54 +0100 Subject: [PATCH 10/25] Code cleaning. --- .../src/view/downcastwriter.js | 28 +----- .../ckeditor5-html-support/src/datafilter.js | 5 - .../src/integrations/documentlist.js | 96 ++++++++----------- .../tests/manual/documentlist.js | 36 +++---- .../src/documentlist/converters.js | 18 ++-- .../src/documentlist/documentlistediting.js | 52 ++++++---- .../src/documentlist/utils/model.js | 7 -- 7 files changed, 98 insertions(+), 144 deletions(-) diff --git a/packages/ckeditor5-engine/src/view/downcastwriter.js b/packages/ckeditor5-engine/src/view/downcastwriter.js index 906e34fe641..170c867fe7c 100644 --- a/packages/ckeditor5-engine/src/view/downcastwriter.js +++ b/packages/ckeditor5-engine/src/view/downcastwriter.js @@ -672,9 +672,6 @@ export default class DowncastWriter { const count = nodeBefore.childCount; nodeBefore._appendChild( nodeAfter.getChildren() ); - // Merge the element attributes in case those have a same ID (other attributes might differ). - // this._mergeElementAttributes( nodeAfter, nodeBefore ); - nodeAfter._remove(); this._removeFromClonedElementsGroup( nodeAfter ); @@ -1629,14 +1626,11 @@ export default class DowncastWriter { * @returns {Boolean} Returns `true` if elements are merged. */ _wrapAttributeElement( wrapper, toWrap ) { - // if ( !canBeJoined( wrapper, toWrap ) ) { - // return false; - // } - if ( wrapper.id !== toWrap.id ) { return false; } + // Those elements have the same ID so just wrap those without further checking. if ( wrapper.id !== null ) { // Move all attributes/classes/styles from wrapper to wrapped AttributeElement. this._mergeElementAttributes( wrapper, toWrap ); @@ -1717,14 +1711,11 @@ export default class DowncastWriter { * @returns {Boolean} Returns `true` if elements are unwrapped. **/ _unwrapAttributeElement( wrapper, toUnwrap ) { - // if ( !canBeJoined( wrapper, toUnwrap ) ) { - // return false; - // } - if ( wrapper.id !== toUnwrap.id ) { return false; } + // Those elements have the same ID so just unwrap those without further checking. if ( wrapper.id !== null ) { // Remove all wrapper's attributes from unwrapped element. this._unmergeElementAttributes( wrapper, toUnwrap ); @@ -2223,18 +2214,3 @@ function validateRangeContainer( range, errorContext ) { throw new CKEditorError( 'view-writer-invalid-range-container', errorContext ); } } - -// Checks if two attribute elements can be joined together. Elements can be joined together if, and only if -// they do not have ids specified. -// -// @private -// @param {module:engine/view/element~Element} a -// @param {module:engine/view/element~Element} b -// @returns {Boolean} -function canBeJoined( a, b ) { - if ( a.id === null && b.id === null ) { - return true; - } - - return a.id === b.id; -} diff --git a/packages/ckeditor5-html-support/src/datafilter.js b/packages/ckeditor5-html-support/src/datafilter.js index e512c4d1ce3..566e13da919 100644 --- a/packages/ckeditor5-html-support/src/datafilter.js +++ b/packages/ckeditor5-html-support/src/datafilter.js @@ -359,11 +359,6 @@ export default class DataFilter extends Plugin { const { view: viewName, model: modelName } = definition; if ( !schema.isRegistered( definition.model ) ) { - if ( !definition.modelSchema ) { - // TODO is this temporary? disabling listItem for li - return; - } - schema.register( definition.model, definition.modelSchema ); if ( !viewName ) { diff --git a/packages/ckeditor5-html-support/src/integrations/documentlist.js b/packages/ckeditor5-html-support/src/integrations/documentlist.js index 3e24a7dce39..104940e5182 100644 --- a/packages/ckeditor5-html-support/src/integrations/documentlist.js +++ b/packages/ckeditor5-html-support/src/integrations/documentlist.js @@ -29,79 +29,63 @@ export default class DocumentListElementSupport extends Plugin { * @inheritDoc */ init() { - if ( !this.editor.plugins.has( 'DocumentListEditing' ) ) { + const editor = this.editor; + + if ( !editor.plugins.has( 'DocumentListEditing' ) ) { return; } - const dataFilter = this.editor.plugins.get( DataFilter ); - - dataFilter.on( 'register:li', registerFilter( { - scope: 'item', - attributeName: 'htmlLiAttributes', - setAttributeOnDowncast: setViewAttributes - }, this.editor ) ); - - dataFilter.on( 'register:ul', registerFilter( { - scope: 'list', - attributeName: 'htmlListAttributes', - setAttributeOnDowncast: setViewAttributes - }, this.editor ) ); - - dataFilter.on( 'register:ol', registerFilter( { - scope: 'list', - attributeName: 'htmlListAttributes', - setAttributeOnDowncast: setViewAttributes - }, this.editor ) ); - } -} - -// TODO -function registerFilter( strategy, editor ) { - const { attributeName } = strategy; + const schema = editor.model.schema; + const conversion = editor.conversion; + const dataFilter = editor.plugins.get( DataFilter ); + const documentListEditing = editor.plugins.get( 'DocumentListEditing' ); - const schema = editor.model.schema; - const conversion = editor.conversion; - const dataFilter = editor.plugins.get( DataFilter ); - - return evt => { - evt.stop(); + dataFilter.on( 'register', ( evt, definition ) => { + if ( ![ 'ul', 'ol', 'li' ].includes( definition.view ) ) { + return; + } - if ( schema.checkAttribute( '$block', attributeName ) ) { - return; - } + evt.stop(); - // Extend codeBlock to allow attributes required by attribute filtration. - schema.extend( '$block', { allowAttributes: [ attributeName ] } ); - schema.extend( '$blockObject', { allowAttributes: [ attributeName ] } ); - schema.extend( '$container', { allowAttributes: [ attributeName ] } ); - - conversion.for( 'upcast' ).add( dispatcher => { - if ( strategy.scope == 'list' ) { - dispatcher.on( 'element:ul', viewToModelListAttributeConverter( strategy, dataFilter ), { priority: 'low' } ); - dispatcher.on( 'element:ol', viewToModelListAttributeConverter( strategy, dataFilter ), { priority: 'low' } ); - } else /* if ( strategy.scope == 'item' ) */ { - dispatcher.on( 'element:li', viewToModelListAttributeConverter( strategy, dataFilter ), { priority: 'low' } ); + // Do not register same converters twice. + if ( schema.checkAttribute( '$block', 'htmlListAttributes' ) ) { + return; } - } ); - // Register downcast strategy. - editor.plugins.get( 'DocumentListEditing' ).registerDowncastStrategy( strategy ); - }; + schema.extend( '$block', { allowAttributes: [ 'htmlListAttributes', 'htmlLiAttributes' ] } ); + schema.extend( '$blockObject', { allowAttributes: [ 'htmlListAttributes', 'htmlLiAttributes' ] } ); + schema.extend( '$container', { allowAttributes: [ 'htmlListAttributes', 'htmlLiAttributes' ] } ); + + conversion.for( 'upcast' ).add( dispatcher => { + dispatcher.on( 'element:ul', viewToModelListAttributeConverter( 'htmlListAttributes', dataFilter ), { priority: 'low' } ); + dispatcher.on( 'element:ol', viewToModelListAttributeConverter( 'htmlListAttributes', dataFilter ), { priority: 'low' } ); + dispatcher.on( 'element:li', viewToModelListAttributeConverter( 'htmlLiAttributes', dataFilter ), { priority: 'low' } ); + } ); + + // Register downcast strategy. + documentListEditing.registerDowncastStrategy( { + scope: 'item', + attributeName: 'htmlLiAttributes', + setAttributeOnDowncast: setViewAttributes + } ); + + documentListEditing.registerDowncastStrategy( { + scope: 'list', + attributeName: 'htmlListAttributes', + setAttributeOnDowncast: setViewAttributes + } ); + } ); + } } // View-to-model conversion helper preserving allowed attributes on {@link TODO} // feature model element. // -// Attributes are preserved as a value of `htmlLiAttributes` model attribute. -// // @private // @param {String} attributeName -// @param {String} viewElementName // @param {module:html-support/datafilter~DataFilter} dataFilter // @returns {Function} Returns a conversion callback. -function viewToModelListAttributeConverter( strategy, dataFilter ) { - const { attributeName } = strategy; - +function viewToModelListAttributeConverter( attributeName, dataFilter ) { return ( evt, data, conversionApi ) => { const viewElement = data.viewItem; diff --git a/packages/ckeditor5-html-support/tests/manual/documentlist.js b/packages/ckeditor5-html-support/tests/manual/documentlist.js index c64f97d0fde..87957aae174 100644 --- a/packages/ckeditor5-html-support/tests/manual/documentlist.js +++ b/packages/ckeditor5-html-support/tests/manual/documentlist.js @@ -13,42 +13,20 @@ import Italic from '@ckeditor/ckeditor5-basic-styles/src/italic'; import Strikethrough from '@ckeditor/ckeditor5-basic-styles/src/strikethrough'; import DocumentListProperties from '@ckeditor/ckeditor5-list/src/documentlistproperties'; import SourceEditing from '@ckeditor/ckeditor5-source-editing/src/sourceediting'; -import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; import GeneralHtmlSupport from '../../src/generalhtmlsupport'; -/** - * Client custom plugin extending HTML support for compatibility. - */ -class ExtendHTMLSupport extends Plugin { - static get requires() { - return [ GeneralHtmlSupport ]; - } - - init() { - const dataFilter = this.editor.plugins.get( 'DataFilter' ); - - dataFilter.allowElement( /^p$/ ); - dataFilter.allowAttributes( { name: /^p$/, attributes: true, styles: true } ); - - dataFilter.allowElement( /^(ul|ol)$/ ); - dataFilter.allowAttributes( { name: /^(ul|ol)$/, attributes: true, styles: true } ); - dataFilter.allowElement( /^(li)$/ ); - dataFilter.allowAttributes( { name: /^(li)$/, attributes: true, styles: true } ); - } -} - ClassicEditor .create( document.querySelector( '#editor' ), { plugins: [ Bold, DocumentListProperties, Essentials, - ExtendHTMLSupport, Italic, Paragraph, Strikethrough, - SourceEditing + SourceEditing, + GeneralHtmlSupport ], toolbar: [ 'sourceEditing', '|', @@ -62,6 +40,16 @@ ClassicEditor startIndex: true, reversed: true } + }, + htmlSupport: { + allow: [ + { + name: /^.*$/, + styles: true, + attributes: true, + classes: true + } + ] } } ) .then( editor => { diff --git a/packages/ckeditor5-list/src/documentlist/converters.js b/packages/ckeditor5-list/src/documentlist/converters.js index c4e164391a5..c39651aa335 100644 --- a/packages/ckeditor5-list/src/documentlist/converters.js +++ b/packages/ckeditor5-list/src/documentlist/converters.js @@ -106,10 +106,11 @@ export function listUpcastCleanList() { * @protected * @param {module:engine/model/model~Model} model The editor model. * @param {module:engine/controller/editingcontroller~EditingController} editing The editing controller. + * @param {Array.} attributeNames The list of all model list attributes (including registered strategies). * @param {module:list/documentlist/documentlistediting~DocumentListEditing} documentListEditing The document list editing plugin. * @return {Function} */ -export function reconvertItemsOnDataChange( model, editing, documentListEditing ) { +export function reconvertItemsOnDataChange( model, editing, attributeNames, documentListEditing ) { return () => { const changes = model.document.differ.getChanges(); const itemsToRefresh = []; @@ -135,7 +136,7 @@ export function reconvertItemsOnDataChange( model, editing, documentListEditing else if ( entry.type == 'attribute' ) { const item = entry.range.start.nodeAfter; - if ( entry.attributeKey.startsWith( 'list' ) ) { + if ( attributeNames.includes( entry.attributeKey ) ) { findAndAddListHeadToMap( entry.range.start, itemToListHead ); if ( entry.attributeNewValue === null ) { @@ -187,7 +188,7 @@ export function reconvertItemsOnDataChange( model, editing, documentListEditing // Update the stack for the current indent level. stack[ itemIndent ] = Object.fromEntries( Array.from( node.getAttributes() ) - .filter( ( [ key ] ) => key.startsWith( 'list' ) ) + .filter( ( [ key ] ) => attributeNames.includes( key ) ) ); // Find all blocks of the current node. @@ -221,7 +222,7 @@ export function reconvertItemsOnDataChange( model, editing, documentListEditing return false; } - const useBogus = shouldUseBogusParagraph( item, blocks ); + const useBogus = shouldUseBogusParagraph( item, attributeNames, blocks ); if ( useBogus && viewElement.is( 'element', 'p' ) ) { return true; @@ -316,14 +317,15 @@ export function listItemDowncastConverter( attributes, strategies, model ) { * Returns the bogus paragraph view element creator. A bogus paragraph is used if a list item contains only a single block or nested list. * * @protected + * @param {Array.} attributeNames The list of all model list attributes (including registered strategies). * @param {Object} [options] * @param {Boolean} [options.dataPipeline=false] * @returns {Function} */ -export function bogusParagraphCreator( { dataPipeline } = {} ) { +export function bogusParagraphCreator( attributeNames, { dataPipeline } = {} ) { return ( modelElement, { writer } ) => { // Convert only if a bogus paragraph should be used. - if ( !shouldUseBogusParagraph( modelElement ) ) { + if ( !shouldUseBogusParagraph( modelElement, attributeNames ) ) { return; } @@ -430,7 +432,7 @@ function createAttributesConsumer( attributes ) { } // Whether the given item should be rendered as a bogus paragraph. -function shouldUseBogusParagraph( item, blocks = getAllListItemBlocks( item ) ) { +function shouldUseBogusParagraph( item, attributeNames, blocks = getAllListItemBlocks( item ) ) { if ( !isListItemBlock( item ) ) { return false; } @@ -442,7 +444,7 @@ function shouldUseBogusParagraph( item, blocks = getAllListItemBlocks( item ) ) } // Don't use bogus paragraph if there are attributes from other features. - if ( !attributeKey.startsWith( 'list' ) ) { + if ( !attributeNames.includes( attributeKey ) ) { return false; } } diff --git a/packages/ckeditor5-list/src/documentlist/documentlistediting.js b/packages/ckeditor5-list/src/documentlist/documentlistediting.js index 5c520196d9a..d7339caba81 100644 --- a/packages/ckeditor5-list/src/documentlist/documentlistediting.js +++ b/packages/ckeditor5-list/src/documentlist/documentlistediting.js @@ -34,8 +34,7 @@ import { isLastBlockOfListItem, isSingleListItem, getSelectedBlockObject, - isListItemBlock, - LIST_BASE_ATTRIBUTES + isListItemBlock } from './utils/model'; import { getViewElementIdForListType, @@ -45,6 +44,13 @@ import ListWalker, { iterateSiblingListBlocks } from './utils/listwalker'; import '../../theme/documentlist.css'; +/** + * A list of base list model attributes. + * + * @private + */ +const LIST_BASE_ATTRIBUTES = [ 'listType', 'listIndent', 'listItemId' ]; + /** * The editing part of the document-list feature. It handles creating, editing and removing lists and list items. * @@ -116,7 +122,6 @@ export default class DocumentListEditing extends Plugin { editor.commands.add( 'splitListItemBefore', new DocumentListSplitCommand( editor, 'before' ) ); editor.commands.add( 'splitListItemAfter', new DocumentListSplitCommand( editor, 'after' ) ); - this._setupModelPostFixing(); this._setupDeleteIntegration(); this._setupEnterIntegration(); this._setupTabIntegration(); @@ -143,7 +148,8 @@ export default class DocumentListEditing extends Plugin { outdent.registerChildCommand( commands.get( 'outdentList' ), { priority: 'lowest' } ); } - // Register conversion after other plugins had a chance to register their attributes. + // Register conversion and model post-fixer after other plugins had a chance to register their attribute strategies. + this._setupModelPostFixing(); this._setupConversion(); } @@ -173,6 +179,18 @@ export default class DocumentListEditing extends Plugin { this._downcastStrategies.push( strategy ); } + /** + * TODO + * + * @private + */ + _getListAttributeNames() { + return [ + ...LIST_BASE_ATTRIBUTES, + ...this._downcastStrategies.map( strategy => strategy.attributeName ) + ]; + } + /** * Attaches the listener to the {@link module:engine/view/document~Document#event:delete} event and handles backspace/delete * keys in and around document lists. @@ -357,6 +375,7 @@ export default class DocumentListEditing extends Plugin { _setupConversion() { const editor = this.editor; const model = editor.model; + const attributeNames = this._getListAttributeNames(); editor.conversion.for( 'upcast' ) .elementToElement( { view: 'li', model: 'paragraph' } ) @@ -369,22 +388,17 @@ export default class DocumentListEditing extends Plugin { editor.conversion.for( 'editingDowncast' ) .elementToElement( { model: 'paragraph', - view: bogusParagraphCreator(), + view: bogusParagraphCreator( attributeNames ), converterPriority: 'high' } ); editor.conversion.for( 'dataDowncast' ) .elementToElement( { model: 'paragraph', - view: bogusParagraphCreator( { dataPipeline: true } ), + view: bogusParagraphCreator( attributeNames, { dataPipeline: true } ), converterPriority: 'high' } ); - const attributeNames = [ - ...LIST_BASE_ATTRIBUTES, - ...this._downcastStrategies.map( strategy => strategy.attributeName ) - ]; - editor.conversion.for( 'downcast' ) .add( dispatcher => { for ( const attributeName of attributeNames ) { @@ -395,7 +409,7 @@ export default class DocumentListEditing extends Plugin { } } ); - this.listenTo( model.document, 'change:data', reconvertItemsOnDataChange( model, editor.editing, this ) ); + this.listenTo( model.document, 'change:data', reconvertItemsOnDataChange( model, editor.editing, attributeNames, this ) ); // For LI verify if an ID of the attribute element is correct. this.on( 'checkAttributes:item', ( evt, { viewElement, modelAttributes } ) => { @@ -424,10 +438,11 @@ export default class DocumentListEditing extends Plugin { */ _setupModelPostFixing() { const model = this.editor.model; + const attributeNames = this._getListAttributeNames(); // Register list fixing. // First the low level handler. - model.document.registerPostFixer( writer => modelChangePostFixer( model, writer, this ) ); + model.document.registerPostFixer( writer => modelChangePostFixer( model, writer, attributeNames, this ) ); // Then the callbacks for the specific lists. // The indentation fixing must be the first one... @@ -463,9 +478,10 @@ export default class DocumentListEditing extends Plugin { // // @param {module:engine/model/model~Model} model The data model. // @param {module:engine/model/writer~Writer} writer The writer to do changes with. -// @param {module:utils/emittermixin~Emitter} emitter The emitter that will fire events for fixing a list. +// @param {Array.} attributeNames The list of all model list attributes (including registered strategies). +// @param {module:list/documentlist/documentlistediting~DocumentListEditing} documentListEditing The document list editing plugin. // @returns {Boolean} `true` if any change has been applied, `false` otherwise. -function modelChangePostFixer( model, writer, emitter ) { +function modelChangePostFixer( model, writer, attributeNames, documentListEditing ) { const changes = model.document.differ.getChanges(); const itemToListHead = new Map(); @@ -478,7 +494,7 @@ function modelChangePostFixer( model, writer, emitter ) { // Remove attributes in case of renamed element. if ( !model.schema.checkAttribute( item, 'listItemId' ) ) { for ( const attributeName of Array.from( item.getAttributeKeys() ) ) { - if ( attributeName.startsWith( 'list' ) ) { + if ( attributeNames.includes( attributeName ) ) { writer.removeAttribute( attributeName, item ); applied = true; @@ -505,7 +521,7 @@ function modelChangePostFixer( model, writer, emitter ) { findAndAddListHeadToMap( entry.position, itemToListHead ); } // Changed list item indent or type. - else if ( entry.type == 'attribute' && entry.attributeKey.startsWith( 'list' ) ) { + else if ( entry.type == 'attribute' && attributeNames.includes( entry.attributeKey ) ) { findAndAddListHeadToMap( entry.range.start, itemToListHead ); if ( entry.attributeNewValue === null ) { @@ -518,7 +534,7 @@ function modelChangePostFixer( model, writer, emitter ) { const seenIds = new Set(); for ( const listHead of itemToListHead.values() ) { - applied = emitter.fire( 'postFixer', { listHead, writer, seenIds } ) || applied; + applied = documentListEditing.fire( 'postFixer', { listHead, writer, seenIds } ) || applied; } return applied; diff --git a/packages/ckeditor5-list/src/documentlist/utils/model.js b/packages/ckeditor5-list/src/documentlist/utils/model.js index 6b34cdd27b2..1563666273f 100644 --- a/packages/ckeditor5-list/src/documentlist/utils/model.js +++ b/packages/ckeditor5-list/src/documentlist/utils/model.js @@ -28,13 +28,6 @@ export class ListItemUid { } } -/** - * A list of base list model attributes. - * - * @protected - */ -export const LIST_BASE_ATTRIBUTES = [ 'listType', 'listIndent', 'listItemId' ]; - /** * Returns true if the given model node is a list item block. * From 47cffde45d42d1222a7d0c2fe3020fe33d0d3abf Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Wed, 2 Mar 2022 20:36:17 +0100 Subject: [PATCH 11/25] Added post-fixer for Document Lists in GHS. --- .../src/integrations/documentlist.js | 56 +++++++++++++++++++ .../src/documentlist/documentlistediting.js | 12 ++-- .../src/documentlist/utils/listwalker.js | 15 ++++- .../src/documentlist/utils/postfixers.js | 12 ++-- .../documentlistpropertiesediting.js | 9 ++- .../tests/documentlist/utils/postfixers.js | 35 ++++++------ 6 files changed, 104 insertions(+), 35 deletions(-) diff --git a/packages/ckeditor5-html-support/src/integrations/documentlist.js b/packages/ckeditor5-html-support/src/integrations/documentlist.js index 104940e5182..844923e8ba2 100644 --- a/packages/ckeditor5-html-support/src/integrations/documentlist.js +++ b/packages/ckeditor5-html-support/src/integrations/documentlist.js @@ -74,6 +74,62 @@ export default class DocumentListElementSupport extends Plugin { attributeName: 'htmlListAttributes', setAttributeOnDowncast: setViewAttributes } ); + + // Make sure that all items in a single list (items at the same level & listType) have the same properties. + // Note: This is almost exact copy from DocumentListPropertiesEditing. + documentListEditing.on( 'postFixer', ( evt, { listNodes, writer } ) => { + const previousNodesByIndent = []; // Last seen nodes of lower indented lists. + + for ( const { node, previous } of listNodes ) { + // For the first list block there is nothing to compare with. + if ( !previous ) { + continue; + } + + const nodeIndent = node.getAttribute( 'listIndent' ); + const previousNodeIndent = previous.getAttribute( 'listIndent' ); + + let previousNodeInList = null; // It's like `previous` but has the same indent as current node. + + // Let's find previous node for the same indent. + // We're going to need that when we get back to previous indent. + if ( nodeIndent > previousNodeIndent ) { + previousNodesByIndent[ previousNodeIndent ] = previous; + } + // Restore the one for given indent. + else if ( nodeIndent < previousNodeIndent ) { + previousNodeInList = previousNodesByIndent[ nodeIndent ]; + previousNodesByIndent.length = nodeIndent; + } + // Same indent. + else { + previousNodeInList = previous; + } + + // This is a first item of a nested list. + if ( !previousNodeInList ) { + continue; + } + + if ( previousNodeInList.getAttribute( 'listType' ) == node.getAttribute( 'listType' ) ) { + const value = previousNodeInList.getAttribute( 'htmlListAttributes' ); + + if ( node.getAttribute( 'htmlListAttributes' ) != value ) { + writer.setAttribute( 'htmlListAttributes', value, node ); + evt.return = true; + } + } + + if ( previousNodeInList.getAttribute( 'listItemId' ) == node.getAttribute( 'listItemId' ) ) { + const value = previousNodeInList.getAttribute( 'htmlLiAttributes' ); + + if ( node.getAttribute( 'htmlLiAttributes' ) != value ) { + writer.setAttribute( 'htmlLiAttributes', value, node ); + evt.return = true; + } + } + } + } ); } ); } } diff --git a/packages/ckeditor5-list/src/documentlist/documentlistediting.js b/packages/ckeditor5-list/src/documentlist/documentlistediting.js index d7339caba81..38af161b600 100644 --- a/packages/ckeditor5-list/src/documentlist/documentlistediting.js +++ b/packages/ckeditor5-list/src/documentlist/documentlistediting.js @@ -446,13 +446,13 @@ export default class DocumentListEditing extends Plugin { // Then the callbacks for the specific lists. // The indentation fixing must be the first one... - this.on( 'postFixer', ( evt, { listHead, writer } ) => { - evt.return = fixListIndents( listHead, writer ) || evt.return; + this.on( 'postFixer', ( evt, { listNodes, writer } ) => { + evt.return = fixListIndents( listNodes, writer ) || evt.return; }, { priority: 'high' } ); // ...then the item ids... and after that other fixers that rely on the correct indentation and ids. - this.on( 'postFixer', ( evt, { listHead, writer, seenIds } ) => { - evt.return = fixListItemIds( listHead, seenIds, writer ) || evt.return; + this.on( 'postFixer', ( evt, { listNodes, writer, seenIds } ) => { + evt.return = fixListItemIds( listNodes, seenIds, writer ) || evt.return; }, { priority: 'high' } ); } } @@ -534,7 +534,9 @@ function modelChangePostFixer( model, writer, attributeNames, documentListEditin const seenIds = new Set(); for ( const listHead of itemToListHead.values() ) { - applied = documentListEditing.fire( 'postFixer', { listHead, writer, seenIds } ) || applied; + const listNodes = { [ Symbol.iterator ]: () => iterateSiblingListBlocks( listHead, 'forward' ) }; + + applied = documentListEditing.fire( 'postFixer', { listHead, writer, seenIds, listNodes } ) || applied; } return applied; diff --git a/packages/ckeditor5-list/src/documentlist/utils/listwalker.js b/packages/ckeditor5-list/src/documentlist/utils/listwalker.js index b9126bf4d9d..230c7977a4f 100644 --- a/packages/ckeditor5-list/src/documentlist/utils/listwalker.js +++ b/packages/ckeditor5-list/src/documentlist/utils/listwalker.js @@ -209,10 +209,11 @@ export default class ListWalker { * * @protected * @param {module:engine/model/node~Node} node The model node. - * @param {'backward'|'forward'} direction Iteration direction. - * @returns {Iterable.} The object with `node` and `previous` {@link module:engine/model/element~Element blocks}. + * @param {'backward'|'forward'} [direction='forward'] Iteration direction. + * @returns {Iterable.} The object with `node` and `previous` + * {@link module:engine/model/element~Element blocks}. */ -export function* iterateSiblingListBlocks( node, direction ) { +export function* iterateSiblingListBlocks( node, direction = 'forward' ) { const isForward = direction == 'forward'; let previous = null; @@ -223,3 +224,11 @@ export function* iterateSiblingListBlocks( node, direction ) { node = isForward ? node.nextSibling : node.previousSibling; } } + +/** + * Object returned by `iterateSiblingListBlocks()` when traversing a list. + * + * @typedef {Object} module:list/documentlist/utils/listwalker~ListIteratorValue + * @property {module:engine/model/node~Node} node The current list node. + * @property {module:engine/model/node~Node} previous The previous list node. + */ diff --git a/packages/ckeditor5-list/src/documentlist/utils/postfixers.js b/packages/ckeditor5-list/src/documentlist/utils/postfixers.js index 9c7b19a2f68..cd8042f204e 100644 --- a/packages/ckeditor5-list/src/documentlist/utils/postfixers.js +++ b/packages/ckeditor5-list/src/documentlist/utils/postfixers.js @@ -44,17 +44,17 @@ export function findAndAddListHeadToMap( position, itemToListHead ) { * Scans the list starting from the given list head element and fixes items' indentation. * * @protected - * @param {module:engine/model/element~Element} listHead The list head model element. + * @param {Iterable.} listNodes The iterable of list nodes. * @param {module:engine/model/writer~Writer} writer The model writer. * @returns {Boolean} Whether the model was modified. */ -export function fixListIndents( listHead, writer ) { +export function fixListIndents( listNodes, writer ) { let maxIndent = 0; // Guards local sublist max indents that need fixing. let prevIndent = -1; // Previous item indent. let fixBy = null; let applied = false; - for ( const { node } of iterateSiblingListBlocks( listHead, 'forward' ) ) { + for ( const { node } of listNodes ) { const itemIndent = node.getAttribute( 'listIndent' ); if ( itemIndent > maxIndent ) { @@ -93,16 +93,16 @@ export function fixListIndents( listHead, writer ) { * Scans the list starting from the given list head element and fixes items' types. * * @protected - * @param {module:engine/model/element~Element} listHead The list head model element. + * @param {Iterable.} listNodes The iterable of list nodes. * @param {Set.} seenIds The set of already known IDs. * @param {module:engine/model/writer~Writer} writer The model writer. * @returns {Boolean} Whether the model was modified. */ -export function fixListItemIds( listHead, seenIds, writer ) { +export function fixListItemIds( listNodes, seenIds, writer ) { const visited = new Set(); let applied = false; - for ( const { node } of iterateSiblingListBlocks( listHead, 'forward' ) ) { + for ( const { node } of listNodes ) { if ( visited.has( node ) ) { continue; } diff --git a/packages/ckeditor5-list/src/documentlistproperties/documentlistpropertiesediting.js b/packages/ckeditor5-list/src/documentlistproperties/documentlistpropertiesediting.js index cfca2fe91e3..de8dc2f3d14 100644 --- a/packages/ckeditor5-list/src/documentlistproperties/documentlistpropertiesediting.js +++ b/packages/ckeditor5-list/src/documentlistproperties/documentlistpropertiesediting.js @@ -14,7 +14,6 @@ import DocumentListStartCommand from './documentliststartcommand'; import DocumentListStyleCommand from './documentliststylecommand'; import DocumentListReversedCommand from './documentlistreversedcommand'; import { listPropertiesUpcastConverter } from './converters'; -import { iterateSiblingListBlocks } from '../documentlist/utils/listwalker'; import { getListTypeFromListStyleType } from './utils/style'; const DEFAULT_LIST_TYPE = 'default'; @@ -97,8 +96,8 @@ export default class DocumentListPropertiesEditing extends Plugin { } ); // Add or remove list properties attributes depending on the list type. - documentListEditing.on( 'postFixer', ( evt, { listHead, writer } ) => { - for ( const { node } of iterateSiblingListBlocks( listHead, 'forward' ) ) { + documentListEditing.on( 'postFixer', ( evt, { listNodes, writer } ) => { + for ( const { node } of listNodes ) { for ( const strategy of strategies ) { // Check if attribute is valid. if ( strategy.hasValidAttribute( node ) ) { @@ -120,10 +119,10 @@ export default class DocumentListPropertiesEditing extends Plugin { } ); // Make sure that all items in a single list (items at the same level & listType) have the same properties. - documentListEditing.on( 'postFixer', ( evt, { listHead, writer } ) => { + documentListEditing.on( 'postFixer', ( evt, { listNodes, writer } ) => { const previousNodesByIndent = []; // Last seen nodes of lower indented lists. - for ( const { node, previous } of iterateSiblingListBlocks( listHead, 'forward' ) ) { + for ( const { node, previous } of listNodes ) { // For the first list block there is nothing to compare with. if ( !previous ) { continue; diff --git a/packages/ckeditor5-list/tests/documentlist/utils/postfixers.js b/packages/ckeditor5-list/tests/documentlist/utils/postfixers.js index 3ceef722ffd..3de9fbdd6f5 100644 --- a/packages/ckeditor5-list/tests/documentlist/utils/postfixers.js +++ b/packages/ckeditor5-list/tests/documentlist/utils/postfixers.js @@ -8,6 +8,9 @@ import { fixListIndents, fixListItemIds } from '../../../src/documentlist/utils/postfixers'; +import { + iterateSiblingListBlocks +} from '../../../src/documentlist/utils/listwalker'; import stubUid from '../_utils/uid'; import { modelList } from '../_utils/utils'; @@ -204,7 +207,7 @@ describe( 'DocumentList - utils - postfixers', () => { const fragment = parseModel( input, schema ); model.change( writer => { - fixListIndents( fragment.getChild( 1 ), writer ); + fixListIndents( iterateSiblingListBlocks( fragment.getChild( 1 ) ), writer ); } ); expect( stringifyModel( fragment ) ).to.equal( modelList( [ @@ -223,7 +226,7 @@ describe( 'DocumentList - utils - postfixers', () => { const fragment = parseModel( input, schema ); model.change( writer => { - fixListIndents( fragment.getChild( 0 ), writer ); + fixListIndents( iterateSiblingListBlocks( fragment.getChild( 0 ) ), writer ); } ); expect( stringifyModel( fragment ) ).to.equal( modelList( [ @@ -243,7 +246,7 @@ describe( 'DocumentList - utils - postfixers', () => { const fragment = parseModel( input, schema ); model.change( writer => { - fixListIndents( fragment.getChild( 0 ), writer ); + fixListIndents( iterateSiblingListBlocks( fragment.getChild( 0 ) ), writer ); } ); expect( stringifyModel( fragment ) ).to.equal( modelList( [ @@ -263,7 +266,7 @@ describe( 'DocumentList - utils - postfixers', () => { const fragment = parseModel( input, schema ); model.change( writer => { - fixListIndents( fragment.getChild( 0 ), writer ); + fixListIndents( iterateSiblingListBlocks( fragment.getChild( 0 ) ), writer ); } ); expect( stringifyModel( fragment ) ).to.equal( modelList( [ @@ -284,7 +287,7 @@ describe( 'DocumentList - utils - postfixers', () => { const fragment = parseModel( input, schema ); model.change( writer => { - fixListIndents( fragment.getChild( 0 ), writer ); + fixListIndents( iterateSiblingListBlocks( fragment.getChild( 0 ) ), writer ); } ); expect( stringifyModel( fragment ) ).to.equal( modelList( [ @@ -309,7 +312,7 @@ describe( 'DocumentList - utils - postfixers', () => { const fragment = parseModel( input, schema ); model.change( writer => { - fixListIndents( fragment.getChild( 0 ), writer ); + fixListIndents( iterateSiblingListBlocks( fragment.getChild( 0 ) ), writer ); } ); expect( stringifyModel( fragment ) ).to.equal( modelList( [ @@ -336,7 +339,7 @@ describe( 'DocumentList - utils - postfixers', () => { const fragment = parseModel( input, schema ); model.change( writer => { - fixListIndents( fragment.getChild( 0 ), writer ); + fixListIndents( iterateSiblingListBlocks( fragment.getChild( 0 ) ), writer ); } ); expect( stringifyModel( fragment ) ).to.equal( modelList( [ @@ -362,7 +365,7 @@ describe( 'DocumentList - utils - postfixers', () => { const fragment = parseModel( input, schema ); model.change( writer => { - fixListIndents( fragment.getChild( 1 ).getChild( 0 ), writer ); + fixListIndents( iterateSiblingListBlocks( fragment.getChild( 1 ).getChild( 0 ) ), writer ); } ); expect( stringifyModel( fragment ) ).to.equal( @@ -390,7 +393,7 @@ describe( 'DocumentList - utils - postfixers', () => { stubUid(); model.change( writer => { - fixListItemIds( fragment.getChild( 0 ), seenIds, writer ); + fixListItemIds( iterateSiblingListBlocks( fragment.getChild( 0 ) ), seenIds, writer ); } ); expect( stringifyModel( fragment ) ).to.equal( modelList( [ @@ -412,7 +415,7 @@ describe( 'DocumentList - utils - postfixers', () => { stubUid(); model.change( writer => { - fixListItemIds( fragment.getChild( 0 ), seenIds, writer ); + fixListItemIds( iterateSiblingListBlocks( fragment.getChild( 0 ) ), seenIds, writer ); } ); expect( stringifyModel( fragment ) ).to.equal( modelList( [ @@ -435,7 +438,7 @@ describe( 'DocumentList - utils - postfixers', () => { stubUid(); model.change( writer => { - fixListItemIds( fragment.getChild( 0 ), seenIds, writer ); + fixListItemIds( iterateSiblingListBlocks( fragment.getChild( 0 ) ), seenIds, writer ); } ); expect( stringifyModel( fragment ) ).to.equal( modelList( [ @@ -458,7 +461,7 @@ describe( 'DocumentList - utils - postfixers', () => { stubUid(); model.change( writer => { - fixListItemIds( fragment.getChild( 0 ), seenIds, writer ); + fixListItemIds( iterateSiblingListBlocks( fragment.getChild( 0 ) ), seenIds, writer ); } ); expect( stringifyModel( fragment ) ).to.equalMarkup( modelList( [ @@ -481,7 +484,7 @@ describe( 'DocumentList - utils - postfixers', () => { stubUid(); model.change( writer => { - fixListItemIds( fragment.getChild( 0 ), seenIds, writer ); + fixListItemIds( iterateSiblingListBlocks( fragment.getChild( 0 ) ), seenIds, writer ); } ); expect( stringifyModel( fragment ) ).to.equalMarkup( modelList( [ @@ -505,7 +508,7 @@ describe( 'DocumentList - utils - postfixers', () => { stubUid(); model.change( writer => { - fixListItemIds( fragment.getChild( 0 ), seenIds, writer ); + fixListItemIds( iterateSiblingListBlocks( fragment.getChild( 0 ) ), seenIds, writer ); } ); expect( stringifyModel( fragment ) ).to.equalMarkup( modelList( [ @@ -534,7 +537,7 @@ describe( 'DocumentList - utils - postfixers', () => { stubUid(); model.change( writer => { - fixListItemIds( fragment.getChild( 0 ), seenIds, writer ); + fixListItemIds( iterateSiblingListBlocks( fragment.getChild( 0 ) ), seenIds, writer ); } ); expect( stringifyModel( fragment ) ).to.equalMarkup( modelList( [ @@ -564,7 +567,7 @@ describe( 'DocumentList - utils - postfixers', () => { seenIds.add( 'b' ); model.change( writer => { - fixListItemIds( fragment.getChild( 0 ), seenIds, writer ); + fixListItemIds( iterateSiblingListBlocks( fragment.getChild( 0 ) ), seenIds, writer ); } ); expect( stringifyModel( fragment ) ).to.equalMarkup( modelList( [ From c609e1641fb5ac10342eb09aa71326a3fd143228 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Wed, 2 Mar 2022 20:55:21 +0100 Subject: [PATCH 12/25] Code cleaning. --- .../src/documentlist/documentlistcommand.js | 4 +-- .../src/documentlist/documentlistediting.js | 29 +--------------- .../src/documentlist/utils/listwalker.js | 8 ++--- .../src/documentlist/utils/model.js | 18 +++++----- .../documentlistreversedcommand.js | 4 +-- .../documentliststartcommand.js | 4 +-- .../documentliststylecommand.js | 4 +-- .../tests/documentlist/_utils/utils.js | 2 +- .../tests/documentlist/documentlistcommand.js | 15 +-------- .../tests/documentlist/documentlistediting.js | 22 ------------- .../tests/documentlist/utils/listwalker.js | 14 ++++---- .../tests/documentlist/utils/model.js | 21 +----------- .../documentlistreversedcommand.js | 15 +-------- .../documentliststartcommand.js | 33 +------------------ .../documentliststylecommand.js | 33 +------------------ 15 files changed, 30 insertions(+), 196 deletions(-) diff --git a/packages/ckeditor5-list/src/documentlist/documentlistcommand.js b/packages/ckeditor5-list/src/documentlist/documentlistcommand.js index 9a63ae2a104..e063b9f98e6 100644 --- a/packages/ckeditor5-list/src/documentlist/documentlistcommand.js +++ b/packages/ckeditor5-list/src/documentlist/documentlistcommand.js @@ -103,9 +103,7 @@ export default class DocumentListCommand extends Command { } // Turning on the list items for a collapsed selection inside a list item. else if ( ( selectedBlockObject || document.selection.isCollapsed ) && blocks[ 0 ].hasAttribute( 'listType' ) ) { - const documentListEditingPlugin = this.editor.plugins.get( 'DocumentListEditing' ); - const sameListDefiningAttributes = documentListEditingPlugin.getSameListDefiningAttributes(); - const changedBlocks = getListItems( selectedBlockObject || blocks[ 0 ], sameListDefiningAttributes ); + const changedBlocks = getListItems( selectedBlockObject || blocks[ 0 ] ); for ( const block of changedBlocks ) { writer.setAttribute( 'listType', this.type, block ); diff --git a/packages/ckeditor5-list/src/documentlist/documentlistediting.js b/packages/ckeditor5-list/src/documentlist/documentlistediting.js index 38af161b600..f75e4b4b9f6 100644 --- a/packages/ckeditor5-list/src/documentlist/documentlistediting.js +++ b/packages/ckeditor5-list/src/documentlist/documentlistediting.js @@ -75,14 +75,6 @@ export default class DocumentListEditing extends Plugin { * @inheritDoc */ init() { - /** - * The list of attributes that must be consistent among all items in the same list. - * - * @private - * @type {Array.} - */ - this._sameListDefiningAttributes = [ 'listType' ]; - /** * TODO * @@ -153,25 +145,6 @@ export default class DocumentListEditing extends Plugin { this._setupConversion(); } - /** - * Register attribute that must be that must be consistent among all items in the same list. - * If list items have different values of registered attributes, they belong to different lists. - * - * @param {String} attributeName - */ - registerSameListDefiningAttributes( attributeName ) { - this._sameListDefiningAttributes.push( attributeName ); - } - - /** - * Gets the list of attributes that must be consistent among all items in the same list. - * - * @returns {Array.} - */ - getSameListDefiningAttributes() { - return this._sameListDefiningAttributes; - } - /** * TODO */ @@ -220,7 +193,7 @@ export default class DocumentListEditing extends Plugin { } const previousBlock = ListWalker.first( positionParent, { - sameListAttributes: this.getSameListDefiningAttributes(), + sameAttributes: 'listType', sameIndent: true } ); diff --git a/packages/ckeditor5-list/src/documentlist/utils/listwalker.js b/packages/ckeditor5-list/src/documentlist/utils/listwalker.js index 230c7977a4f..76233851e44 100644 --- a/packages/ckeditor5-list/src/documentlist/utils/listwalker.js +++ b/packages/ckeditor5-list/src/documentlist/utils/listwalker.js @@ -21,7 +21,7 @@ export default class ListWalker { * @param {Object} options * @param {'forward'|'backward'} [options.direction='backward'] The iterating direction. * @param {Boolean} [options.includeSelf=false] Whether start block should be included in the result (if it's matching other criteria). - * @param {Array.|String} [options.sameListAttributes=[]] Additional attributes that must be the same for each block. + * @param {Array.|String} [options.sameAttributes=[]] Additional attributes that must be the same for each block. * @param {Boolean} [options.sameIndent=false] Whether blocks with the same indent level as the start block should be included * in the result. * @param {Boolean} [options.lowerIndent=false] Whether blocks with a lower indent level than the start block should be included @@ -68,7 +68,7 @@ export default class ListWalker { * @private * @type {Array.} */ - this._sameListAttributes = toArray( options.sameListAttributes || [] ); + this._sameAttributes = toArray( options.sameAttributes || [] ); /** * Whether blocks with the same indent level as the start block should be included in the result. @@ -102,7 +102,7 @@ export default class ListWalker { * @param {Object} options * @param {'forward'|'backward'} [options.direction='backward'] The iterating direction. * @param {Boolean} [options.includeSelf=false] Whether start block should be included in the result (if it's matching other criteria). - * @param {Array.|String} [options.sameListAttributes=[]] Additional attributes that must be the same for each block. + * @param {Array.|String} [options.sameAttributes=[]] Additional attributes that must be the same for each block. * @param {Boolean} [options.sameIndent=false] Whether blocks with the same indent level as the start block should be included * in the result. * @param {Boolean} [options.lowerIndent=false] Whether blocks with a lower indent level than the start block should be included @@ -172,7 +172,7 @@ export default class ListWalker { } // Abort if item has any additionally specified attribute different. - if ( this._sameListAttributes.some( attr => node.getAttribute( attr ) !== this._startElement.getAttribute( attr ) ) ) { + if ( this._sameAttributes.some( attr => node.getAttribute( attr ) !== this._startElement.getAttribute( attr ) ) ) { break; } } diff --git a/packages/ckeditor5-list/src/documentlist/utils/model.js b/packages/ckeditor5-list/src/documentlist/utils/model.js index 1563666273f..b8c60b68d70 100644 --- a/packages/ckeditor5-list/src/documentlist/utils/model.js +++ b/packages/ckeditor5-list/src/documentlist/utils/model.js @@ -80,7 +80,7 @@ export function getListItemBlocks( listItem, options = {} ) { ...options, includeSelf: isForward, sameIndent: true, - sameListAttributes: 'listItemId' + sameAttributes: 'listItemId' } ) ); return isForward ? items : items.reverse(); @@ -105,18 +105,17 @@ export function getNestedListBlocks( listItem ) { * * @protected * @param {module:engine/model/element~Element} listItem Starting list item element. - * @param {Array.} sameListDefiningAttributes The list of attributes that must be consistent among all items in the same list. * @returns {Array.} */ -export function getListItems( listItem, sameListDefiningAttributes ) { +export function getListItems( listItem ) { const backwardBlocks = new ListWalker( listItem, { sameIndent: true, - sameListAttributes: sameListDefiningAttributes + sameAttributes: 'listType' } ); const forwardBlocks = new ListWalker( listItem, { sameIndent: true, - sameListAttributes: sameListDefiningAttributes, + sameAttributes: 'listType', includeSelf: true, direction: 'forward' } ); @@ -137,7 +136,7 @@ export function getListItems( listItem, sameListDefiningAttributes ) { export function isFirstBlockOfListItem( listBlock ) { const previousSibling = ListWalker.first( listBlock, { sameIndent: true, - sameListAttributes: 'listItemId' + sameAttributes: 'listItemId' } ); if ( !previousSibling ) { @@ -158,7 +157,7 @@ export function isLastBlockOfListItem( listBlock ) { const nextSibling = ListWalker.first( listBlock, { direction: 'forward', sameIndent: true, - sameListAttributes: 'listItemId' + sameAttributes: 'listItemId' } ); if ( !nextSibling ) { @@ -197,16 +196,15 @@ export function expandListBlocksToCompleteItems( blocks, options = {} ) { * * @protected * @param {module:engine/model/element~Element|Array.} blocks The list of selected blocks. - * @param {Array.} sameListDefiningAttributes The list of attributes that must be consistent among all items in the same list. * @returns {Array.} */ -export function expandListBlocksToCompleteList( blocks, sameListDefiningAttributes ) { +export function expandListBlocksToCompleteList( blocks ) { blocks = toArray( blocks ); const allBlocks = new Set(); for ( const block of blocks ) { - for ( const itemBlock of getListItems( block, sameListDefiningAttributes ) ) { + for ( const itemBlock of getListItems( block ) ) { allBlocks.add( itemBlock ); } } diff --git a/packages/ckeditor5-list/src/documentlistproperties/documentlistreversedcommand.js b/packages/ckeditor5-list/src/documentlistproperties/documentlistreversedcommand.js index 3c2149ce079..9409949095f 100644 --- a/packages/ckeditor5-list/src/documentlistproperties/documentlistreversedcommand.js +++ b/packages/ckeditor5-list/src/documentlistproperties/documentlistreversedcommand.js @@ -46,9 +46,7 @@ export default class DocumentListReversedCommand extends Command { let blocks = Array.from( document.selection.getSelectedBlocks() ) .filter( block => isListItemBlock( block ) && block.getAttribute( 'listType' ) == 'numbered' ); - const documentListEditingPlugin = this.editor.plugins.get( 'DocumentListEditing' ); - - blocks = expandListBlocksToCompleteList( blocks, documentListEditingPlugin.getSameListDefiningAttributes() ); + blocks = expandListBlocksToCompleteList( blocks ); model.change( writer => { for ( const block of blocks ) { diff --git a/packages/ckeditor5-list/src/documentlistproperties/documentliststartcommand.js b/packages/ckeditor5-list/src/documentlistproperties/documentliststartcommand.js index b674ed3dd5f..027523eec2c 100644 --- a/packages/ckeditor5-list/src/documentlistproperties/documentliststartcommand.js +++ b/packages/ckeditor5-list/src/documentlistproperties/documentliststartcommand.js @@ -46,9 +46,7 @@ export default class DocumentListStartCommand extends Command { let blocks = Array.from( document.selection.getSelectedBlocks() ) .filter( block => isListItemBlock( block ) && block.getAttribute( 'listType' ) == 'numbered' ); - const documentListEditingPlugin = this.editor.plugins.get( 'DocumentListEditing' ); - - blocks = expandListBlocksToCompleteList( blocks, documentListEditingPlugin.getSameListDefiningAttributes() ); + blocks = expandListBlocksToCompleteList( blocks ); model.change( writer => { for ( const block of blocks ) { diff --git a/packages/ckeditor5-list/src/documentlistproperties/documentliststylecommand.js b/packages/ckeditor5-list/src/documentlistproperties/documentliststylecommand.js index e7dc9d803dc..9f624199d3d 100644 --- a/packages/ckeditor5-list/src/documentlistproperties/documentliststylecommand.js +++ b/packages/ckeditor5-list/src/documentlistproperties/documentliststylecommand.js @@ -72,9 +72,7 @@ export default class DocumentListStyleCommand extends Command { return; } - const documentListEditingPlugin = this.editor.plugins.get( 'DocumentListEditing' ); - - blocks = expandListBlocksToCompleteList( blocks, documentListEditingPlugin.getSameListDefiningAttributes() ); + blocks = expandListBlocksToCompleteList( blocks ); for ( const block of blocks ) { writer.setAttribute( 'listStyle', options.type || this._defaultType, block ); diff --git a/packages/ckeditor5-list/tests/documentlist/_utils/utils.js b/packages/ckeditor5-list/tests/documentlist/_utils/utils.js index af9348b1911..edbfecab109 100644 --- a/packages/ckeditor5-list/tests/documentlist/_utils/utils.js +++ b/packages/ckeditor5-list/tests/documentlist/_utils/utils.js @@ -331,7 +331,7 @@ export function stringifyList( fragmentOrElement ) { if ( node.hasAttribute( 'listItemId' ) ) { const marker = node.getAttribute( 'listType' ) == 'numbered' ? '#' : '*'; const indentSpaces = ( node.getAttribute( 'listIndent' ) + 1 ) * 2; - const isFollowing = !!ListWalker.first( node, { sameIndent: true, sameListAttributes: 'listItemId' } ); + const isFollowing = !!ListWalker.first( node, { sameIndent: true, sameAttributes: 'listItemId' } ); pad = isFollowing ? ' '.repeat( indentSpaces ) : marker.padStart( indentSpaces - 1 ) + ' '; } diff --git a/packages/ckeditor5-list/tests/documentlist/documentlistcommand.js b/packages/ckeditor5-list/tests/documentlist/documentlistcommand.js index beb485e9226..7812255c1c4 100644 --- a/packages/ckeditor5-list/tests/documentlist/documentlistcommand.js +++ b/packages/ckeditor5-list/tests/documentlist/documentlistcommand.js @@ -7,7 +7,6 @@ import DocumentListCommand from '../../src/documentlist/documentlistcommand'; import stubUid from './_utils/uid'; import { modelList } from './_utils/utils'; -import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; import Editor from '@ckeditor/ckeditor5-core/src/editor/editor'; import Model from '@ckeditor/ckeditor5-engine/src/model/model'; @@ -19,20 +18,8 @@ describe( 'DocumentListCommand', () => { testUtils.createSinonSandbox(); - class DocumentListEditingMock extends Plugin { - static get pluginName() { - return 'DocumentListEditing'; - } - - getSameListDefiningAttributes() { - return [ 'listType' ]; - } - } - beforeEach( async () => { - editor = new Editor( { - plugins: [ DocumentListEditingMock ] - } ); + editor = new Editor(); await editor.initPlugins(); diff --git a/packages/ckeditor5-list/tests/documentlist/documentlistediting.js b/packages/ckeditor5-list/tests/documentlist/documentlistediting.js index a2293a79b52..9f770111f8e 100644 --- a/packages/ckeditor5-list/tests/documentlist/documentlistediting.js +++ b/packages/ckeditor5-list/tests/documentlist/documentlistediting.js @@ -105,28 +105,6 @@ describe( 'DocumentListEditing', () => { expect( model.schema.checkAttribute( [ '$root', 'tableCell' ], 'listType' ) ).to.be.false; } ); - describe( 'same list-defining attributes', () => { - let plugin; - - beforeEach( () => { - plugin = editor.plugins.get( DocumentListEditing ); - } ); - - it( '`listType` should define list', () => { - expect( plugin.getSameListDefiningAttributes() ).to.include( 'listType' ); - } ); - - it( 'should allow registering custom list-defining attributes', () => { - plugin.registerSameListDefiningAttributes( 'attr1' ); - plugin.registerSameListDefiningAttributes( 'attr2' ); - plugin.registerSameListDefiningAttributes( 'attr3' ); - - expect( plugin.getSameListDefiningAttributes() ).to.include( 'attr1' ); - expect( plugin.getSameListDefiningAttributes() ).to.include( 'attr2' ); - expect( plugin.getSameListDefiningAttributes() ).to.include( 'attr3' ); - } ); - } ); - describe( 'commands', () => { it( 'should register indent list command', () => { const command = editor.commands.get( 'indentList' ); diff --git a/packages/ckeditor5-list/tests/documentlist/utils/listwalker.js b/packages/ckeditor5-list/tests/documentlist/utils/listwalker.js index 5c5d1d88467..29542bf5d55 100644 --- a/packages/ckeditor5-list/tests/documentlist/utils/listwalker.js +++ b/packages/ckeditor5-list/tests/documentlist/utils/listwalker.js @@ -154,7 +154,7 @@ describe( 'DocumentList - utils - ListWalker', () => { direction: 'forward', sameIndent: true, includeSelf: true, - sameListAttributes: 'listItemId' + sameAttributes: 'listItemId' } ); const blocks = Array.from( walker ); @@ -175,7 +175,7 @@ describe( 'DocumentList - utils - ListWalker', () => { direction: 'forward', sameIndent: true, includeSelf: true, - sameListAttributes: [ 'listType' ] + sameAttributes: [ 'listType' ] } ); const blocks = Array.from( walker ); @@ -196,7 +196,7 @@ describe( 'DocumentList - utils - ListWalker', () => { direction: 'forward', sameIndent: true, includeSelf: true, - sameListAttributes: [ 'listStyle' ] + sameAttributes: [ 'listStyle' ] } ); const blocks = Array.from( walker ); @@ -218,7 +218,7 @@ describe( 'DocumentList - utils - ListWalker', () => { direction: 'forward', sameIndent: true, includeSelf: true, - sameListAttributes: [ 'listStyle', 'listReversed' ] + sameAttributes: [ 'listStyle', 'listReversed' ] } ); const blocks = Array.from( walker ); @@ -311,7 +311,7 @@ describe( 'DocumentList - utils - ListWalker', () => { sameIndent: true, higherIndent: true, includeSelf: true, - sameListAttributes: 'listItemId' + sameAttributes: 'listItemId' } ); const blocks = Array.from( walker ); @@ -338,7 +338,7 @@ describe( 'DocumentList - utils - ListWalker', () => { sameIndent: true, higherIndent: true, includeSelf: true, - sameListAttributes: 'listItemId' + sameAttributes: 'listItemId' } ); const blocks = Array.from( walker ); @@ -365,7 +365,7 @@ describe( 'DocumentList - utils - ListWalker', () => { sameIndent: true, higherIndent: true, includeSelf: true, - sameListAttributes: 'listItemId' + sameAttributes: 'listItemId' } ); const blocks = Array.from( walker ); diff --git a/packages/ckeditor5-list/tests/documentlist/utils/model.js b/packages/ckeditor5-list/tests/documentlist/utils/model.js index 41bbd87bca8..8fdd40f7102 100644 --- a/packages/ckeditor5-list/tests/documentlist/utils/model.js +++ b/packages/ckeditor5-list/tests/documentlist/utils/model.js @@ -469,26 +469,7 @@ describe( 'DocumentList - utils - model', () => { const fragment = parseModel( input, schema ); const listItem = fragment.getChild( 2 ); - expect( getListItems( listItem, [ 'listType' ] ) ).to.deep.equal( [ - fragment.getChild( 1 ), - fragment.getChild( 2 ), - fragment.getChild( 3 ) - ] ); - } ); - - it( 'shout return all list items of the same properties', () => { - const input = modelList( [ - '# 0 {reversed:true}{start:1}', - '# 1 {reversed:false}{start:5}', - '# 2', - '# 3', - '# 4 {reversed:false}{start:3}' - ] ); - - const fragment = parseModel( input, schema ); - const listItem = fragment.getChild( 2 ); - - expect( getListItems( listItem, [ 'listType', 'listReversed', 'listStart' ] ) ).to.deep.equal( [ + expect( getListItems( listItem ) ).to.deep.equal( [ fragment.getChild( 1 ), fragment.getChild( 2 ), fragment.getChild( 3 ) diff --git a/packages/ckeditor5-list/tests/documentlistproperties/documentlistreversedcommand.js b/packages/ckeditor5-list/tests/documentlistproperties/documentlistreversedcommand.js index 47d0090a113..df3d6097355 100644 --- a/packages/ckeditor5-list/tests/documentlistproperties/documentlistreversedcommand.js +++ b/packages/ckeditor5-list/tests/documentlistproperties/documentlistreversedcommand.js @@ -5,7 +5,6 @@ import Editor from '@ckeditor/ckeditor5-core/src/editor/editor'; import Model from '@ckeditor/ckeditor5-engine/src/model/model'; -import { Plugin } from '@ckeditor/ckeditor5-core'; import { setData, getData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; import DocumentListReversedCommand from '../../src/documentlistproperties/documentlistreversedcommand'; @@ -14,20 +13,8 @@ import { modelList } from '../documentlist/_utils/utils'; describe( 'DocumentListReversedCommand', () => { let editor, model, listReversedCommand; - class DocumentListEditingMock extends Plugin { - static get pluginName() { - return 'DocumentListEditing'; - } - - getSameListDefiningAttributes() { - return [ 'listType', 'listReversed' ]; - } - } - beforeEach( async () => { - editor = new Editor( { - plugins: [ DocumentListEditingMock ] - } ); + editor = new Editor(); await editor.initPlugins(); diff --git a/packages/ckeditor5-list/tests/documentlistproperties/documentliststartcommand.js b/packages/ckeditor5-list/tests/documentlistproperties/documentliststartcommand.js index b07edd9ed63..dcf7d381f12 100644 --- a/packages/ckeditor5-list/tests/documentlistproperties/documentliststartcommand.js +++ b/packages/ckeditor5-list/tests/documentlistproperties/documentliststartcommand.js @@ -5,7 +5,6 @@ import Editor from '@ckeditor/ckeditor5-core/src/editor/editor'; import Model from '@ckeditor/ckeditor5-engine/src/model/model'; -import { Plugin } from '@ckeditor/ckeditor5-core'; import { setData, getData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; import DocumentListStartCommand from '../../src/documentlistproperties/documentliststartcommand'; @@ -14,20 +13,8 @@ import { modelList } from '../documentlist/_utils/utils'; describe( 'DocumentListStartCommand', () => { let editor, model, listStartCommand; - class DocumentListEditingMock extends Plugin { - static get pluginName() { - return 'DocumentListEditing'; - } - - getSameListDefiningAttributes() { - return [ 'listType', 'listStart' ]; - } - } - beforeEach( async () => { - editor = new Editor( { - plugins: [ DocumentListEditingMock ] - } ); + editor = new Editor(); await editor.initPlugins(); @@ -293,24 +280,6 @@ describe( 'DocumentListStartCommand', () => { ` ) ); } ); - it( 'should stop searching for the list items when spotted listItem with different listStart attribute', () => { - setData( model, modelList( ` - Foo. - # 1.[] {start:2} - # 2. - # 3. {start:3} - ` ) ); - - listStartCommand.execute( { startIndex: 5 } ); - - expect( getData( model ) ).to.equalMarkup( modelList( ` - Foo. - # 1.[] {start:5} - # 2. - # 3. {start:3} - ` ) ); - } ); - it( 'should set the `listStart` attribute for selected items (non-collapsed selection)', () => { setData( model, modelList( ` # 1. {start:7} diff --git a/packages/ckeditor5-list/tests/documentlistproperties/documentliststylecommand.js b/packages/ckeditor5-list/tests/documentlistproperties/documentliststylecommand.js index de4f85bc624..7d7f10f11fa 100644 --- a/packages/ckeditor5-list/tests/documentlistproperties/documentliststylecommand.js +++ b/packages/ckeditor5-list/tests/documentlistproperties/documentliststylecommand.js @@ -6,7 +6,6 @@ import Editor from '@ckeditor/ckeditor5-core/src/editor/editor'; import Model from '@ckeditor/ckeditor5-engine/src/model/model'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; -import { Plugin } from '@ckeditor/ckeditor5-core'; import { setData, getData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; import DocumentListCommand from '../../src/documentlist/documentlistcommand'; @@ -19,20 +18,8 @@ describe( 'DocumentListStyleCommand', () => { testUtils.createSinonSandbox(); - class DocumentListEditingMock extends Plugin { - static get pluginName() { - return 'DocumentListEditing'; - } - - getSameListDefiningAttributes() { - return [ 'listType', 'listStyle' ]; - } - } - beforeEach( async () => { - editor = new Editor( { - plugins: [ DocumentListEditingMock ] - } ); + editor = new Editor(); await editor.initPlugins(); @@ -278,24 +265,6 @@ describe( 'DocumentListStyleCommand', () => { ` ) ); } ); - it( 'should stop searching for the list items when spotted listItem with different `listStyle` attribute', () => { - setData( model, modelList( ` - Foo. - * 1.[] {style:default} - * 2. - * 3. {style:disc} - ` ) ); - - listStyleCommand.execute( { type: 'circle' } ); - - expect( getData( model ) ).to.equalMarkup( modelList( ` - Foo. - * 1.[] {style:circle} - * 2. - * 3. {style:disc} - ` ) ); - } ); - it( 'should set the `listStyle` attribute for selected items (non-collapsed selection)', () => { setData( model, modelList( ` * 1. {style:disc} From 5ad2f0fe982f00b3d856922432dc16040ebd521f Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Thu, 3 Mar 2022 15:25:46 +0100 Subject: [PATCH 13/25] Reverted handling of a list attributes (using attribute name prefix convention). --- .../src/integrations/documentlist.js | 34 ++++++++-------- .../src/documentlist/converters.js | 39 ++++++++++--------- .../src/documentlist/documentlistediting.js | 38 +++++------------- 3 files changed, 47 insertions(+), 64 deletions(-) diff --git a/packages/ckeditor5-html-support/src/integrations/documentlist.js b/packages/ckeditor5-html-support/src/integrations/documentlist.js index 844923e8ba2..0466a0d3df3 100644 --- a/packages/ckeditor5-html-support/src/integrations/documentlist.js +++ b/packages/ckeditor5-html-support/src/integrations/documentlist.js @@ -48,30 +48,32 @@ export default class DocumentListElementSupport extends Plugin { evt.stop(); // Do not register same converters twice. - if ( schema.checkAttribute( '$block', 'htmlListAttributes' ) ) { + if ( schema.checkAttribute( '$block', 'listHtmlListAttributes' ) ) { return; } - schema.extend( '$block', { allowAttributes: [ 'htmlListAttributes', 'htmlLiAttributes' ] } ); - schema.extend( '$blockObject', { allowAttributes: [ 'htmlListAttributes', 'htmlLiAttributes' ] } ); - schema.extend( '$container', { allowAttributes: [ 'htmlListAttributes', 'htmlLiAttributes' ] } ); + // Note that document list integration is using attributes prefixed by "list" + // to automatically use mechanisms built into the document lists. + schema.extend( '$block', { allowAttributes: [ 'listHtmlListAttributes', 'listHtmlLiAttributes' ] } ); + schema.extend( '$blockObject', { allowAttributes: [ 'listHtmlListAttributes', 'listHtmlLiAttributes' ] } ); + schema.extend( '$container', { allowAttributes: [ 'listHtmlListAttributes', 'listHtmlLiAttributes' ] } ); conversion.for( 'upcast' ).add( dispatcher => { - dispatcher.on( 'element:ul', viewToModelListAttributeConverter( 'htmlListAttributes', dataFilter ), { priority: 'low' } ); - dispatcher.on( 'element:ol', viewToModelListAttributeConverter( 'htmlListAttributes', dataFilter ), { priority: 'low' } ); - dispatcher.on( 'element:li', viewToModelListAttributeConverter( 'htmlLiAttributes', dataFilter ), { priority: 'low' } ); + dispatcher.on( 'element:ul', upcastListAttributeConverter( 'listHtmlListAttributes', dataFilter ), { priority: 'low' } ); + dispatcher.on( 'element:ol', upcastListAttributeConverter( 'listHtmlListAttributes', dataFilter ), { priority: 'low' } ); + dispatcher.on( 'element:li', upcastListAttributeConverter( 'listHtmlLiAttributes', dataFilter ), { priority: 'low' } ); } ); // Register downcast strategy. documentListEditing.registerDowncastStrategy( { scope: 'item', - attributeName: 'htmlLiAttributes', + attributeName: 'listHtmlLiAttributes', setAttributeOnDowncast: setViewAttributes } ); documentListEditing.registerDowncastStrategy( { scope: 'list', - attributeName: 'htmlListAttributes', + attributeName: 'listHtmlListAttributes', setAttributeOnDowncast: setViewAttributes } ); @@ -112,19 +114,19 @@ export default class DocumentListElementSupport extends Plugin { } if ( previousNodeInList.getAttribute( 'listType' ) == node.getAttribute( 'listType' ) ) { - const value = previousNodeInList.getAttribute( 'htmlListAttributes' ); + const value = previousNodeInList.getAttribute( 'listHtmlListAttributes' ); - if ( node.getAttribute( 'htmlListAttributes' ) != value ) { - writer.setAttribute( 'htmlListAttributes', value, node ); + if ( node.getAttribute( 'listHtmlListAttributes' ) != value ) { + writer.setAttribute( 'listHtmlListAttributes', value, node ); evt.return = true; } } if ( previousNodeInList.getAttribute( 'listItemId' ) == node.getAttribute( 'listItemId' ) ) { - const value = previousNodeInList.getAttribute( 'htmlLiAttributes' ); + const value = previousNodeInList.getAttribute( 'listHtmlLiAttributes' ); - if ( node.getAttribute( 'htmlLiAttributes' ) != value ) { - writer.setAttribute( 'htmlLiAttributes', value, node ); + if ( node.getAttribute( 'listHtmlLiAttributes' ) != value ) { + writer.setAttribute( 'listHtmlLiAttributes', value, node ); evt.return = true; } } @@ -141,7 +143,7 @@ export default class DocumentListElementSupport extends Plugin { // @param {String} attributeName // @param {module:html-support/datafilter~DataFilter} dataFilter // @returns {Function} Returns a conversion callback. -function viewToModelListAttributeConverter( attributeName, dataFilter ) { +function upcastListAttributeConverter( attributeName, dataFilter ) { return ( evt, data, conversionApi ) => { const viewElement = data.viewItem; diff --git a/packages/ckeditor5-list/src/documentlist/converters.js b/packages/ckeditor5-list/src/documentlist/converters.js index c39651aa335..2b2534b1bce 100644 --- a/packages/ckeditor5-list/src/documentlist/converters.js +++ b/packages/ckeditor5-list/src/documentlist/converters.js @@ -106,11 +106,10 @@ export function listUpcastCleanList() { * @protected * @param {module:engine/model/model~Model} model The editor model. * @param {module:engine/controller/editingcontroller~EditingController} editing The editing controller. - * @param {Array.} attributeNames The list of all model list attributes (including registered strategies). * @param {module:list/documentlist/documentlistediting~DocumentListEditing} documentListEditing The document list editing plugin. * @return {Function} */ -export function reconvertItemsOnDataChange( model, editing, attributeNames, documentListEditing ) { +export function reconvertItemsOnDataChange( model, editing, documentListEditing ) { return () => { const changes = model.document.differ.getChanges(); const itemsToRefresh = []; @@ -136,7 +135,7 @@ export function reconvertItemsOnDataChange( model, editing, attributeNames, docu else if ( entry.type == 'attribute' ) { const item = entry.range.start.nodeAfter; - if ( attributeNames.includes( entry.attributeKey ) ) { + if ( entry.attributeKey.startsWith( 'list' ) ) { findAndAddListHeadToMap( entry.range.start, itemToListHead ); if ( entry.attributeNewValue === null ) { @@ -188,7 +187,7 @@ export function reconvertItemsOnDataChange( model, editing, attributeNames, docu // Update the stack for the current indent level. stack[ itemIndent ] = Object.fromEntries( Array.from( node.getAttributes() ) - .filter( ( [ key ] ) => attributeNames.includes( key ) ) + .filter( ( [ key ] ) => key.startsWith( 'list' ) ) ); // Find all blocks of the current node. @@ -222,7 +221,7 @@ export function reconvertItemsOnDataChange( model, editing, attributeNames, docu return false; } - const useBogus = shouldUseBogusParagraph( item, attributeNames, blocks ); + const useBogus = shouldUseBogusParagraph( item, blocks ); if ( useBogus && viewElement.is( 'element', 'p' ) ) { return true; @@ -283,19 +282,22 @@ export function reconvertItemsOnDataChange( model, editing, attributeNames, docu * Returns the list item downcast converter. * * @protected - * @param {Array.} attributes A list of attribute names that should be converted if are set. - * @param {TODO} strategies TODO - * @param {module:engine/model/model~Model} model The model. + * @param {module:list/documentlist/documentlistediting~DocumentListEditing} documentListEditing The document list editing plugin. * @returns {Function} */ -export function listItemDowncastConverter( attributes, strategies, model ) { - const consumer = createAttributesConsumer( attributes ); +export function listItemDowncastConverter( documentListEditing ) { + const model = documentListEditing.editor.model; + const consumer = createAttributesConsumer(); return ( evt, data, conversionApi ) => { const { writer, mapper, consumable } = conversionApi; const listItem = data.item; + if ( !data.attributeKey.startsWith( 'list' ) ) { + return; + } + // Test if attributes on the converted items are not consumed. if ( !consumer( listItem, consumable ) ) { return; @@ -309,7 +311,7 @@ export function listItemDowncastConverter( attributes, strategies, model ) { unwrapListItemBlock( viewElement, writer ); // Then wrap them with the new list wrappers. - wrapListItemBlock( listItem, writer.createRangeOn( viewElement ), strategies, writer ); + wrapListItemBlock( listItem, writer.createRangeOn( viewElement ), documentListEditing._downcastStrategies, writer ); }; } @@ -317,15 +319,14 @@ export function listItemDowncastConverter( attributes, strategies, model ) { * Returns the bogus paragraph view element creator. A bogus paragraph is used if a list item contains only a single block or nested list. * * @protected - * @param {Array.} attributeNames The list of all model list attributes (including registered strategies). * @param {Object} [options] * @param {Boolean} [options.dataPipeline=false] * @returns {Function} */ -export function bogusParagraphCreator( attributeNames, { dataPipeline } = {} ) { +export function bogusParagraphCreator( { dataPipeline } = {} ) { return ( modelElement, { writer } ) => { // Convert only if a bogus paragraph should be used. - if ( !shouldUseBogusParagraph( modelElement, attributeNames ) ) { + if ( !shouldUseBogusParagraph( modelElement ) ) { return; } @@ -410,13 +411,13 @@ function wrapListItemBlock( listItem, viewRange, strategies, writer ) { } // Returns the function that is responsible for consuming attributes that are set on the model node. -function createAttributesConsumer( attributes ) { +function createAttributesConsumer() { return ( node, consumable ) => { const events = []; // Collect all set attributes that are triggering conversion. - for ( const attributeName of attributes ) { - if ( node.hasAttribute( attributeName ) ) { + for ( const attributeName of node.getAttributeKeys() ) { + if ( attributeName.startsWith( 'list' ) ) { events.push( `attribute:${ attributeName }` ); } } @@ -432,7 +433,7 @@ function createAttributesConsumer( attributes ) { } // Whether the given item should be rendered as a bogus paragraph. -function shouldUseBogusParagraph( item, attributeNames, blocks = getAllListItemBlocks( item ) ) { +function shouldUseBogusParagraph( item, blocks = getAllListItemBlocks( item ) ) { if ( !isListItemBlock( item ) ) { return false; } @@ -444,7 +445,7 @@ function shouldUseBogusParagraph( item, attributeNames, blocks = getAllListItemB } // Don't use bogus paragraph if there are attributes from other features. - if ( !attributeNames.includes( attributeKey ) ) { + if ( !attributeKey.startsWith( 'list' ) ) { return false; } } diff --git a/packages/ckeditor5-list/src/documentlist/documentlistediting.js b/packages/ckeditor5-list/src/documentlist/documentlistediting.js index f75e4b4b9f6..d91d89c70a0 100644 --- a/packages/ckeditor5-list/src/documentlist/documentlistediting.js +++ b/packages/ckeditor5-list/src/documentlist/documentlistediting.js @@ -78,7 +78,7 @@ export default class DocumentListEditing extends Plugin { /** * TODO * - * @private + * @protected */ this._downcastStrategies = []; @@ -152,18 +152,6 @@ export default class DocumentListEditing extends Plugin { this._downcastStrategies.push( strategy ); } - /** - * TODO - * - * @private - */ - _getListAttributeNames() { - return [ - ...LIST_BASE_ATTRIBUTES, - ...this._downcastStrategies.map( strategy => strategy.attributeName ) - ]; - } - /** * Attaches the listener to the {@link module:engine/view/document~Document#event:delete} event and handles backspace/delete * keys in and around document lists. @@ -348,7 +336,6 @@ export default class DocumentListEditing extends Plugin { _setupConversion() { const editor = this.editor; const model = editor.model; - const attributeNames = this._getListAttributeNames(); editor.conversion.for( 'upcast' ) .elementToElement( { view: 'li', model: 'paragraph' } ) @@ -361,28 +348,23 @@ export default class DocumentListEditing extends Plugin { editor.conversion.for( 'editingDowncast' ) .elementToElement( { model: 'paragraph', - view: bogusParagraphCreator( attributeNames ), + view: bogusParagraphCreator(), converterPriority: 'high' } ); editor.conversion.for( 'dataDowncast' ) .elementToElement( { model: 'paragraph', - view: bogusParagraphCreator( attributeNames, { dataPipeline: true } ), + view: bogusParagraphCreator( { dataPipeline: true } ), converterPriority: 'high' } ); editor.conversion.for( 'downcast' ) .add( dispatcher => { - for ( const attributeName of attributeNames ) { - dispatcher.on( - `attribute:${ attributeName }`, - listItemDowncastConverter( attributeNames, this._downcastStrategies, model ) - ); - } + dispatcher.on( 'attribute', listItemDowncastConverter( this ) ); } ); - this.listenTo( model.document, 'change:data', reconvertItemsOnDataChange( model, editor.editing, attributeNames, this ) ); + this.listenTo( model.document, 'change:data', reconvertItemsOnDataChange( model, editor.editing, this ) ); // For LI verify if an ID of the attribute element is correct. this.on( 'checkAttributes:item', ( evt, { viewElement, modelAttributes } ) => { @@ -411,11 +393,10 @@ export default class DocumentListEditing extends Plugin { */ _setupModelPostFixing() { const model = this.editor.model; - const attributeNames = this._getListAttributeNames(); // Register list fixing. // First the low level handler. - model.document.registerPostFixer( writer => modelChangePostFixer( model, writer, attributeNames, this ) ); + model.document.registerPostFixer( writer => modelChangePostFixer( model, writer, this ) ); // Then the callbacks for the specific lists. // The indentation fixing must be the first one... @@ -451,10 +432,9 @@ export default class DocumentListEditing extends Plugin { // // @param {module:engine/model/model~Model} model The data model. // @param {module:engine/model/writer~Writer} writer The writer to do changes with. -// @param {Array.} attributeNames The list of all model list attributes (including registered strategies). // @param {module:list/documentlist/documentlistediting~DocumentListEditing} documentListEditing The document list editing plugin. // @returns {Boolean} `true` if any change has been applied, `false` otherwise. -function modelChangePostFixer( model, writer, attributeNames, documentListEditing ) { +function modelChangePostFixer( model, writer, documentListEditing ) { const changes = model.document.differ.getChanges(); const itemToListHead = new Map(); @@ -467,7 +447,7 @@ function modelChangePostFixer( model, writer, attributeNames, documentListEditin // Remove attributes in case of renamed element. if ( !model.schema.checkAttribute( item, 'listItemId' ) ) { for ( const attributeName of Array.from( item.getAttributeKeys() ) ) { - if ( attributeNames.includes( attributeName ) ) { + if ( attributeName.startsWith( 'list' ) ) { writer.removeAttribute( attributeName, item ); applied = true; @@ -494,7 +474,7 @@ function modelChangePostFixer( model, writer, attributeNames, documentListEditin findAndAddListHeadToMap( entry.position, itemToListHead ); } // Changed list item indent or type. - else if ( entry.type == 'attribute' && attributeNames.includes( entry.attributeKey ) ) { + else if ( entry.type == 'attribute' && entry.attributeKey.startsWith( 'list' ) ) { findAndAddListHeadToMap( entry.range.start, itemToListHead ); if ( entry.attributeNewValue === null ) { From 829b34a889bcd4aa36cbedca12b7ef17e77d1beb Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Thu, 3 Mar 2022 15:29:46 +0100 Subject: [PATCH 14/25] Reverted changes in downcast writer. --- .../src/view/downcastwriter.js | 57 ++++++------------- 1 file changed, 17 insertions(+), 40 deletions(-) diff --git a/packages/ckeditor5-engine/src/view/downcastwriter.js b/packages/ckeditor5-engine/src/view/downcastwriter.js index 170c867fe7c..92d992eebad 100644 --- a/packages/ckeditor5-engine/src/view/downcastwriter.js +++ b/packages/ckeditor5-engine/src/view/downcastwriter.js @@ -1626,18 +1626,10 @@ export default class DowncastWriter { * @returns {Boolean} Returns `true` if elements are merged. */ _wrapAttributeElement( wrapper, toWrap ) { - if ( wrapper.id !== toWrap.id ) { + if ( !canBeJoined( wrapper, toWrap ) ) { return false; } - // Those elements have the same ID so just wrap those without further checking. - if ( wrapper.id !== null ) { - // Move all attributes/classes/styles from wrapper to wrapped AttributeElement. - this._mergeElementAttributes( wrapper, toWrap ); - - return true; - } - // Can't merge if name or priority differs. if ( wrapper.name !== toWrap.name || wrapper.priority !== toWrap.priority ) { return false; @@ -1664,17 +1656,6 @@ export default class DowncastWriter { } // Move all attributes/classes/styles from wrapper to wrapped AttributeElement. - this._mergeElementAttributes( wrapper, toWrap ); - - return true; - } - - /** - * TODO - * - * @private - */ - _mergeElementAttributes( wrapper, toWrap ) { for ( const key of wrapper.getAttributeKeys() ) { // Classes and styles should be checked separately. if ( key === 'class' || key === 'style' ) { @@ -1698,6 +1679,8 @@ export default class DowncastWriter { this.addClass( key, toWrap ); } } + + return true; } /** @@ -1711,18 +1694,10 @@ export default class DowncastWriter { * @returns {Boolean} Returns `true` if elements are unwrapped. **/ _unwrapAttributeElement( wrapper, toUnwrap ) { - if ( wrapper.id !== toUnwrap.id ) { + if ( !canBeJoined( wrapper, toUnwrap ) ) { return false; } - // Those elements have the same ID so just unwrap those without further checking. - if ( wrapper.id !== null ) { - // Remove all wrapper's attributes from unwrapped element. - this._unmergeElementAttributes( wrapper, toUnwrap ); - - return true; - } - // Can't unwrap if name or priority differs. if ( wrapper.name !== toUnwrap.name || wrapper.priority !== toUnwrap.priority ) { return false; @@ -1755,17 +1730,6 @@ export default class DowncastWriter { } // Remove all wrapper's attributes from unwrapped element. - this._unmergeElementAttributes( wrapper, toUnwrap ); - - return true; - } - - /** - * TODO - * - * @private - */ - _unmergeElementAttributes( wrapper, toUnwrap ) { for ( const key of wrapper.getAttributeKeys() ) { // Classes and styles should be checked separately. if ( key === 'class' || key === 'style' ) { @@ -1780,6 +1744,8 @@ export default class DowncastWriter { // Remove all wrapper's styles from unwrapped element. this.removeStyle( Array.from( wrapper.getStyleNames() ), toUnwrap ); + + return true; } /** @@ -2214,3 +2180,14 @@ function validateRangeContainer( range, errorContext ) { throw new CKEditorError( 'view-writer-invalid-range-container', errorContext ); } } + +// Checks if two attribute elements can be joined together. Elements can be joined together if, and only if +// they do not have ids specified. +// +// @private +// @param {module:engine/view/element~Element} a +// @param {module:engine/view/element~Element} b +// @returns {Boolean} +function canBeJoined( a, b ) { + return a.id === null && b.id === null; +} From 2face4ce5f7e46e42bf9269d5f1a5412bfa6ffee Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Thu, 3 Mar 2022 15:57:06 +0100 Subject: [PATCH 15/25] Code cleaning. --- .../src/integrations/documentlist.js | 10 ++++++++-- .../documentlistpropertiesediting.js | 13 +++++++++---- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/packages/ckeditor5-html-support/src/integrations/documentlist.js b/packages/ckeditor5-html-support/src/integrations/documentlist.js index 0466a0d3df3..517c8b44610 100644 --- a/packages/ckeditor5-html-support/src/integrations/documentlist.js +++ b/packages/ckeditor5-html-support/src/integrations/documentlist.js @@ -68,13 +68,19 @@ export default class DocumentListElementSupport extends Plugin { documentListEditing.registerDowncastStrategy( { scope: 'item', attributeName: 'listHtmlLiAttributes', - setAttributeOnDowncast: setViewAttributes + + setAttributeOnDowncast( writer, attributeValue, viewElement ) { + setViewAttributes( writer, attributeValue, viewElement ); + } } ); documentListEditing.registerDowncastStrategy( { scope: 'list', attributeName: 'listHtmlListAttributes', - setAttributeOnDowncast: setViewAttributes + + setAttributeOnDowncast( writer, viewAttributes, viewElement ) { + setViewAttributes( writer, viewAttributes, viewElement ); + } } ); // Make sure that all items in a single list (items at the same level & listType) have the same properties. diff --git a/packages/ckeditor5-list/src/documentlistproperties/documentlistpropertiesediting.js b/packages/ckeditor5-list/src/documentlistproperties/documentlistpropertiesediting.js index de8dc2f3d14..a7aa756effa 100644 --- a/packages/ckeditor5-list/src/documentlistproperties/documentlistpropertiesediting.js +++ b/packages/ckeditor5-list/src/documentlistproperties/documentlistpropertiesediting.js @@ -69,12 +69,20 @@ export default class DocumentListPropertiesEditing extends Plugin { for ( const strategy of strategies ) { strategy.addCommand( editor ); + model.schema.extend( '$container', { allowAttributes: strategy.attributeName } ); model.schema.extend( '$block', { allowAttributes: strategy.attributeName } ); model.schema.extend( '$blockObject', { allowAttributes: strategy.attributeName } ); // Register downcast strategy. - documentListEditing.registerDowncastStrategy( strategy ); + documentListEditing.registerDowncastStrategy( { + scope: 'list', + attributeName: strategy.attributeName, + + setAttributeOnDowncast( writer, attributeValue, viewElement ) { + strategy.setAttributeOnDowncast( writer, attributeValue, viewElement ); + } + } ); } // Set up conversion. @@ -203,7 +211,6 @@ function createAttributeStrategies( enabledProperties ) { if ( enabledProperties.styles ) { strategies.push( { - scope: 'list', attributeName: 'listStyle', defaultValue: DEFAULT_LIST_TYPE, viewConsumables: { styles: 'list-style-type' }, @@ -246,7 +253,6 @@ function createAttributeStrategies( enabledProperties ) { if ( enabledProperties.reversed ) { strategies.push( { - scope: 'list', attributeName: 'listReversed', defaultValue: false, viewConsumables: { attributes: 'reversed' }, @@ -279,7 +285,6 @@ function createAttributeStrategies( enabledProperties ) { if ( enabledProperties.startIndex ) { strategies.push( { - scope: 'list', attributeName: 'listStart', defaultValue: 1, viewConsumables: { attributes: 'start' }, From 4d1eda8f5ff94d1d19b5c988ac69988aacf67e50 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Mon, 7 Mar 2022 11:46:44 +0100 Subject: [PATCH 16/25] Revert "Reverted handling of a list attributes (using attribute name prefix convention)." This reverts commit 5ad2f0fe --- .../src/integrations/documentlist.js | 34 ++++++++-------- .../src/documentlist/converters.js | 39 +++++++++---------- .../src/documentlist/documentlistediting.js | 38 +++++++++++++----- 3 files changed, 64 insertions(+), 47 deletions(-) diff --git a/packages/ckeditor5-html-support/src/integrations/documentlist.js b/packages/ckeditor5-html-support/src/integrations/documentlist.js index 517c8b44610..e474104fa37 100644 --- a/packages/ckeditor5-html-support/src/integrations/documentlist.js +++ b/packages/ckeditor5-html-support/src/integrations/documentlist.js @@ -48,26 +48,24 @@ export default class DocumentListElementSupport extends Plugin { evt.stop(); // Do not register same converters twice. - if ( schema.checkAttribute( '$block', 'listHtmlListAttributes' ) ) { + if ( schema.checkAttribute( '$block', 'htmlListAttributes' ) ) { return; } - // Note that document list integration is using attributes prefixed by "list" - // to automatically use mechanisms built into the document lists. - schema.extend( '$block', { allowAttributes: [ 'listHtmlListAttributes', 'listHtmlLiAttributes' ] } ); - schema.extend( '$blockObject', { allowAttributes: [ 'listHtmlListAttributes', 'listHtmlLiAttributes' ] } ); - schema.extend( '$container', { allowAttributes: [ 'listHtmlListAttributes', 'listHtmlLiAttributes' ] } ); + schema.extend( '$block', { allowAttributes: [ 'htmlListAttributes', 'htmlLiAttributes' ] } ); + schema.extend( '$blockObject', { allowAttributes: [ 'htmlListAttributes', 'htmlLiAttributes' ] } ); + schema.extend( '$container', { allowAttributes: [ 'htmlListAttributes', 'htmlLiAttributes' ] } ); conversion.for( 'upcast' ).add( dispatcher => { - dispatcher.on( 'element:ul', upcastListAttributeConverter( 'listHtmlListAttributes', dataFilter ), { priority: 'low' } ); - dispatcher.on( 'element:ol', upcastListAttributeConverter( 'listHtmlListAttributes', dataFilter ), { priority: 'low' } ); - dispatcher.on( 'element:li', upcastListAttributeConverter( 'listHtmlLiAttributes', dataFilter ), { priority: 'low' } ); + dispatcher.on( 'element:ul', viewToModelListAttributeConverter( 'htmlListAttributes', dataFilter ), { priority: 'low' } ); + dispatcher.on( 'element:ol', viewToModelListAttributeConverter( 'htmlListAttributes', dataFilter ), { priority: 'low' } ); + dispatcher.on( 'element:li', viewToModelListAttributeConverter( 'htmlLiAttributes', dataFilter ), { priority: 'low' } ); } ); // Register downcast strategy. documentListEditing.registerDowncastStrategy( { scope: 'item', - attributeName: 'listHtmlLiAttributes', + attributeName: 'htmlLiAttributes', setAttributeOnDowncast( writer, attributeValue, viewElement ) { setViewAttributes( writer, attributeValue, viewElement ); @@ -76,7 +74,7 @@ export default class DocumentListElementSupport extends Plugin { documentListEditing.registerDowncastStrategy( { scope: 'list', - attributeName: 'listHtmlListAttributes', + attributeName: 'htmlListAttributes', setAttributeOnDowncast( writer, viewAttributes, viewElement ) { setViewAttributes( writer, viewAttributes, viewElement ); @@ -120,19 +118,19 @@ export default class DocumentListElementSupport extends Plugin { } if ( previousNodeInList.getAttribute( 'listType' ) == node.getAttribute( 'listType' ) ) { - const value = previousNodeInList.getAttribute( 'listHtmlListAttributes' ); + const value = previousNodeInList.getAttribute( 'htmlListAttributes' ); - if ( node.getAttribute( 'listHtmlListAttributes' ) != value ) { - writer.setAttribute( 'listHtmlListAttributes', value, node ); + if ( node.getAttribute( 'htmlListAttributes' ) != value ) { + writer.setAttribute( 'htmlListAttributes', value, node ); evt.return = true; } } if ( previousNodeInList.getAttribute( 'listItemId' ) == node.getAttribute( 'listItemId' ) ) { - const value = previousNodeInList.getAttribute( 'listHtmlLiAttributes' ); + const value = previousNodeInList.getAttribute( 'htmlLiAttributes' ); - if ( node.getAttribute( 'listHtmlLiAttributes' ) != value ) { - writer.setAttribute( 'listHtmlLiAttributes', value, node ); + if ( node.getAttribute( 'htmlLiAttributes' ) != value ) { + writer.setAttribute( 'htmlLiAttributes', value, node ); evt.return = true; } } @@ -149,7 +147,7 @@ export default class DocumentListElementSupport extends Plugin { // @param {String} attributeName // @param {module:html-support/datafilter~DataFilter} dataFilter // @returns {Function} Returns a conversion callback. -function upcastListAttributeConverter( attributeName, dataFilter ) { +function viewToModelListAttributeConverter( attributeName, dataFilter ) { return ( evt, data, conversionApi ) => { const viewElement = data.viewItem; diff --git a/packages/ckeditor5-list/src/documentlist/converters.js b/packages/ckeditor5-list/src/documentlist/converters.js index 2b2534b1bce..c39651aa335 100644 --- a/packages/ckeditor5-list/src/documentlist/converters.js +++ b/packages/ckeditor5-list/src/documentlist/converters.js @@ -106,10 +106,11 @@ export function listUpcastCleanList() { * @protected * @param {module:engine/model/model~Model} model The editor model. * @param {module:engine/controller/editingcontroller~EditingController} editing The editing controller. + * @param {Array.} attributeNames The list of all model list attributes (including registered strategies). * @param {module:list/documentlist/documentlistediting~DocumentListEditing} documentListEditing The document list editing plugin. * @return {Function} */ -export function reconvertItemsOnDataChange( model, editing, documentListEditing ) { +export function reconvertItemsOnDataChange( model, editing, attributeNames, documentListEditing ) { return () => { const changes = model.document.differ.getChanges(); const itemsToRefresh = []; @@ -135,7 +136,7 @@ export function reconvertItemsOnDataChange( model, editing, documentListEditing else if ( entry.type == 'attribute' ) { const item = entry.range.start.nodeAfter; - if ( entry.attributeKey.startsWith( 'list' ) ) { + if ( attributeNames.includes( entry.attributeKey ) ) { findAndAddListHeadToMap( entry.range.start, itemToListHead ); if ( entry.attributeNewValue === null ) { @@ -187,7 +188,7 @@ export function reconvertItemsOnDataChange( model, editing, documentListEditing // Update the stack for the current indent level. stack[ itemIndent ] = Object.fromEntries( Array.from( node.getAttributes() ) - .filter( ( [ key ] ) => key.startsWith( 'list' ) ) + .filter( ( [ key ] ) => attributeNames.includes( key ) ) ); // Find all blocks of the current node. @@ -221,7 +222,7 @@ export function reconvertItemsOnDataChange( model, editing, documentListEditing return false; } - const useBogus = shouldUseBogusParagraph( item, blocks ); + const useBogus = shouldUseBogusParagraph( item, attributeNames, blocks ); if ( useBogus && viewElement.is( 'element', 'p' ) ) { return true; @@ -282,22 +283,19 @@ export function reconvertItemsOnDataChange( model, editing, documentListEditing * Returns the list item downcast converter. * * @protected - * @param {module:list/documentlist/documentlistediting~DocumentListEditing} documentListEditing The document list editing plugin. + * @param {Array.} attributes A list of attribute names that should be converted if are set. + * @param {TODO} strategies TODO + * @param {module:engine/model/model~Model} model The model. * @returns {Function} */ -export function listItemDowncastConverter( documentListEditing ) { - const model = documentListEditing.editor.model; - const consumer = createAttributesConsumer(); +export function listItemDowncastConverter( attributes, strategies, model ) { + const consumer = createAttributesConsumer( attributes ); return ( evt, data, conversionApi ) => { const { writer, mapper, consumable } = conversionApi; const listItem = data.item; - if ( !data.attributeKey.startsWith( 'list' ) ) { - return; - } - // Test if attributes on the converted items are not consumed. if ( !consumer( listItem, consumable ) ) { return; @@ -311,7 +309,7 @@ export function listItemDowncastConverter( documentListEditing ) { unwrapListItemBlock( viewElement, writer ); // Then wrap them with the new list wrappers. - wrapListItemBlock( listItem, writer.createRangeOn( viewElement ), documentListEditing._downcastStrategies, writer ); + wrapListItemBlock( listItem, writer.createRangeOn( viewElement ), strategies, writer ); }; } @@ -319,14 +317,15 @@ export function listItemDowncastConverter( documentListEditing ) { * Returns the bogus paragraph view element creator. A bogus paragraph is used if a list item contains only a single block or nested list. * * @protected + * @param {Array.} attributeNames The list of all model list attributes (including registered strategies). * @param {Object} [options] * @param {Boolean} [options.dataPipeline=false] * @returns {Function} */ -export function bogusParagraphCreator( { dataPipeline } = {} ) { +export function bogusParagraphCreator( attributeNames, { dataPipeline } = {} ) { return ( modelElement, { writer } ) => { // Convert only if a bogus paragraph should be used. - if ( !shouldUseBogusParagraph( modelElement ) ) { + if ( !shouldUseBogusParagraph( modelElement, attributeNames ) ) { return; } @@ -411,13 +410,13 @@ function wrapListItemBlock( listItem, viewRange, strategies, writer ) { } // Returns the function that is responsible for consuming attributes that are set on the model node. -function createAttributesConsumer() { +function createAttributesConsumer( attributes ) { return ( node, consumable ) => { const events = []; // Collect all set attributes that are triggering conversion. - for ( const attributeName of node.getAttributeKeys() ) { - if ( attributeName.startsWith( 'list' ) ) { + for ( const attributeName of attributes ) { + if ( node.hasAttribute( attributeName ) ) { events.push( `attribute:${ attributeName }` ); } } @@ -433,7 +432,7 @@ function createAttributesConsumer() { } // Whether the given item should be rendered as a bogus paragraph. -function shouldUseBogusParagraph( item, blocks = getAllListItemBlocks( item ) ) { +function shouldUseBogusParagraph( item, attributeNames, blocks = getAllListItemBlocks( item ) ) { if ( !isListItemBlock( item ) ) { return false; } @@ -445,7 +444,7 @@ function shouldUseBogusParagraph( item, blocks = getAllListItemBlocks( item ) ) } // Don't use bogus paragraph if there are attributes from other features. - if ( !attributeKey.startsWith( 'list' ) ) { + if ( !attributeNames.includes( attributeKey ) ) { return false; } } diff --git a/packages/ckeditor5-list/src/documentlist/documentlistediting.js b/packages/ckeditor5-list/src/documentlist/documentlistediting.js index d91d89c70a0..f75e4b4b9f6 100644 --- a/packages/ckeditor5-list/src/documentlist/documentlistediting.js +++ b/packages/ckeditor5-list/src/documentlist/documentlistediting.js @@ -78,7 +78,7 @@ export default class DocumentListEditing extends Plugin { /** * TODO * - * @protected + * @private */ this._downcastStrategies = []; @@ -152,6 +152,18 @@ export default class DocumentListEditing extends Plugin { this._downcastStrategies.push( strategy ); } + /** + * TODO + * + * @private + */ + _getListAttributeNames() { + return [ + ...LIST_BASE_ATTRIBUTES, + ...this._downcastStrategies.map( strategy => strategy.attributeName ) + ]; + } + /** * Attaches the listener to the {@link module:engine/view/document~Document#event:delete} event and handles backspace/delete * keys in and around document lists. @@ -336,6 +348,7 @@ export default class DocumentListEditing extends Plugin { _setupConversion() { const editor = this.editor; const model = editor.model; + const attributeNames = this._getListAttributeNames(); editor.conversion.for( 'upcast' ) .elementToElement( { view: 'li', model: 'paragraph' } ) @@ -348,23 +361,28 @@ export default class DocumentListEditing extends Plugin { editor.conversion.for( 'editingDowncast' ) .elementToElement( { model: 'paragraph', - view: bogusParagraphCreator(), + view: bogusParagraphCreator( attributeNames ), converterPriority: 'high' } ); editor.conversion.for( 'dataDowncast' ) .elementToElement( { model: 'paragraph', - view: bogusParagraphCreator( { dataPipeline: true } ), + view: bogusParagraphCreator( attributeNames, { dataPipeline: true } ), converterPriority: 'high' } ); editor.conversion.for( 'downcast' ) .add( dispatcher => { - dispatcher.on( 'attribute', listItemDowncastConverter( this ) ); + for ( const attributeName of attributeNames ) { + dispatcher.on( + `attribute:${ attributeName }`, + listItemDowncastConverter( attributeNames, this._downcastStrategies, model ) + ); + } } ); - this.listenTo( model.document, 'change:data', reconvertItemsOnDataChange( model, editor.editing, this ) ); + this.listenTo( model.document, 'change:data', reconvertItemsOnDataChange( model, editor.editing, attributeNames, this ) ); // For LI verify if an ID of the attribute element is correct. this.on( 'checkAttributes:item', ( evt, { viewElement, modelAttributes } ) => { @@ -393,10 +411,11 @@ export default class DocumentListEditing extends Plugin { */ _setupModelPostFixing() { const model = this.editor.model; + const attributeNames = this._getListAttributeNames(); // Register list fixing. // First the low level handler. - model.document.registerPostFixer( writer => modelChangePostFixer( model, writer, this ) ); + model.document.registerPostFixer( writer => modelChangePostFixer( model, writer, attributeNames, this ) ); // Then the callbacks for the specific lists. // The indentation fixing must be the first one... @@ -432,9 +451,10 @@ export default class DocumentListEditing extends Plugin { // // @param {module:engine/model/model~Model} model The data model. // @param {module:engine/model/writer~Writer} writer The writer to do changes with. +// @param {Array.} attributeNames The list of all model list attributes (including registered strategies). // @param {module:list/documentlist/documentlistediting~DocumentListEditing} documentListEditing The document list editing plugin. // @returns {Boolean} `true` if any change has been applied, `false` otherwise. -function modelChangePostFixer( model, writer, documentListEditing ) { +function modelChangePostFixer( model, writer, attributeNames, documentListEditing ) { const changes = model.document.differ.getChanges(); const itemToListHead = new Map(); @@ -447,7 +467,7 @@ function modelChangePostFixer( model, writer, documentListEditing ) { // Remove attributes in case of renamed element. if ( !model.schema.checkAttribute( item, 'listItemId' ) ) { for ( const attributeName of Array.from( item.getAttributeKeys() ) ) { - if ( attributeName.startsWith( 'list' ) ) { + if ( attributeNames.includes( attributeName ) ) { writer.removeAttribute( attributeName, item ); applied = true; @@ -474,7 +494,7 @@ function modelChangePostFixer( model, writer, documentListEditing ) { findAndAddListHeadToMap( entry.position, itemToListHead ); } // Changed list item indent or type. - else if ( entry.type == 'attribute' && entry.attributeKey.startsWith( 'list' ) ) { + else if ( entry.type == 'attribute' && attributeNames.includes( entry.attributeKey ) ) { findAndAddListHeadToMap( entry.range.start, itemToListHead ); if ( entry.attributeNewValue === null ) { From a174f29066bc7ae375329f8461c263595a5c85f2 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Mon, 7 Mar 2022 12:09:42 +0100 Subject: [PATCH 17/25] Using registered strategies instead of attribute name prefix. --- .../src/integrations/documentlist.js | 39 ++++++++++--------- .../src/documentlist/converters.js | 14 ++++--- .../src/documentlist/documentlistediting.js | 7 +--- 3 files changed, 30 insertions(+), 30 deletions(-) diff --git a/packages/ckeditor5-html-support/src/integrations/documentlist.js b/packages/ckeditor5-html-support/src/integrations/documentlist.js index e474104fa37..3fcae5d983e 100644 --- a/packages/ckeditor5-html-support/src/integrations/documentlist.js +++ b/packages/ckeditor5-html-support/src/integrations/documentlist.js @@ -40,6 +40,26 @@ export default class DocumentListElementSupport extends Plugin { const dataFilter = editor.plugins.get( DataFilter ); const documentListEditing = editor.plugins.get( 'DocumentListEditing' ); + // Register downcast strategy. + // Note that this must be done before document list editing registers conversion in afterInit. + documentListEditing.registerDowncastStrategy( { + scope: 'item', + attributeName: 'htmlLiAttributes', + + setAttributeOnDowncast( writer, attributeValue, viewElement ) { + setViewAttributes( writer, attributeValue, viewElement ); + } + } ); + + documentListEditing.registerDowncastStrategy( { + scope: 'list', + attributeName: 'htmlListAttributes', + + setAttributeOnDowncast( writer, viewAttributes, viewElement ) { + setViewAttributes( writer, viewAttributes, viewElement ); + } + } ); + dataFilter.on( 'register', ( evt, definition ) => { if ( ![ 'ul', 'ol', 'li' ].includes( definition.view ) ) { return; @@ -62,25 +82,6 @@ export default class DocumentListElementSupport extends Plugin { dispatcher.on( 'element:li', viewToModelListAttributeConverter( 'htmlLiAttributes', dataFilter ), { priority: 'low' } ); } ); - // Register downcast strategy. - documentListEditing.registerDowncastStrategy( { - scope: 'item', - attributeName: 'htmlLiAttributes', - - setAttributeOnDowncast( writer, attributeValue, viewElement ) { - setViewAttributes( writer, attributeValue, viewElement ); - } - } ); - - documentListEditing.registerDowncastStrategy( { - scope: 'list', - attributeName: 'htmlListAttributes', - - setAttributeOnDowncast( writer, viewAttributes, viewElement ) { - setViewAttributes( writer, viewAttributes, viewElement ); - } - } ); - // Make sure that all items in a single list (items at the same level & listType) have the same properties. // Note: This is almost exact copy from DocumentListPropertiesEditing. documentListEditing.on( 'postFixer', ( evt, { listNodes, writer } ) => { diff --git a/packages/ckeditor5-list/src/documentlist/converters.js b/packages/ckeditor5-list/src/documentlist/converters.js index c39651aa335..5e31e7019be 100644 --- a/packages/ckeditor5-list/src/documentlist/converters.js +++ b/packages/ckeditor5-list/src/documentlist/converters.js @@ -283,19 +283,23 @@ export function reconvertItemsOnDataChange( model, editing, attributeNames, docu * Returns the list item downcast converter. * * @protected - * @param {Array.} attributes A list of attribute names that should be converted if are set. + * @param {Array.} attributeNames A list of attribute names that should be converted if are set. * @param {TODO} strategies TODO * @param {module:engine/model/model~Model} model The model. * @returns {Function} */ -export function listItemDowncastConverter( attributes, strategies, model ) { - const consumer = createAttributesConsumer( attributes ); +export function listItemDowncastConverter( attributeNames, strategies, model ) { + const consumer = createAttributesConsumer( attributeNames ); return ( evt, data, conversionApi ) => { const { writer, mapper, consumable } = conversionApi; const listItem = data.item; + if ( !attributeNames.includes( data.attributeKey ) ) { + return; + } + // Test if attributes on the converted items are not consumed. if ( !consumer( listItem, consumable ) ) { return; @@ -410,12 +414,12 @@ function wrapListItemBlock( listItem, viewRange, strategies, writer ) { } // Returns the function that is responsible for consuming attributes that are set on the model node. -function createAttributesConsumer( attributes ) { +function createAttributesConsumer( attributeNames ) { return ( node, consumable ) => { const events = []; // Collect all set attributes that are triggering conversion. - for ( const attributeName of attributes ) { + for ( const attributeName of attributeNames ) { if ( node.hasAttribute( attributeName ) ) { events.push( `attribute:${ attributeName }` ); } diff --git a/packages/ckeditor5-list/src/documentlist/documentlistediting.js b/packages/ckeditor5-list/src/documentlist/documentlistediting.js index f75e4b4b9f6..90cebe09671 100644 --- a/packages/ckeditor5-list/src/documentlist/documentlistediting.js +++ b/packages/ckeditor5-list/src/documentlist/documentlistediting.js @@ -374,12 +374,7 @@ export default class DocumentListEditing extends Plugin { editor.conversion.for( 'downcast' ) .add( dispatcher => { - for ( const attributeName of attributeNames ) { - dispatcher.on( - `attribute:${ attributeName }`, - listItemDowncastConverter( attributeNames, this._downcastStrategies, model ) - ); - } + dispatcher.on( 'attribute', listItemDowncastConverter( attributeNames, this._downcastStrategies, model ) ); } ); this.listenTo( model.document, 'change:data', reconvertItemsOnDataChange( model, editor.editing, attributeNames, this ) ); From e1973ffcad52865ead2f1048ce96173f7f126903 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Mon, 7 Mar 2022 15:20:01 +0100 Subject: [PATCH 18/25] Code cleaning. --- .../src/documentlist/converters.js | 2 +- .../src/documentlist/documentlistediting.js | 14 +++++++--- .../src/documentlist/utils/listwalker.js | 28 ++++++++++++++++++- 3 files changed, 38 insertions(+), 6 deletions(-) diff --git a/packages/ckeditor5-list/src/documentlist/converters.js b/packages/ckeditor5-list/src/documentlist/converters.js index 50af557e442..c6a717052de 100644 --- a/packages/ckeditor5-list/src/documentlist/converters.js +++ b/packages/ckeditor5-list/src/documentlist/converters.js @@ -297,7 +297,7 @@ export function reconvertItemsOnDataChange( model, editing, attributeNames, docu * * @protected * @param {Array.} attributeNames A list of attribute names that should be converted if are set. - * @param {TODO} strategies TODO + * @param {Array.} strategies The strategies. * @param {module:engine/model/model~Model} model The model. * @returns {Function} */ diff --git a/packages/ckeditor5-list/src/documentlist/documentlistediting.js b/packages/ckeditor5-list/src/documentlist/documentlistediting.js index 7a4843ffa06..8dd0a5843b3 100644 --- a/packages/ckeditor5-list/src/documentlist/documentlistediting.js +++ b/packages/ckeditor5-list/src/documentlist/documentlistediting.js @@ -40,7 +40,10 @@ import { getViewElementIdForListType, getViewElementNameForListType } from './utils/view'; -import ListWalker, { iterateSiblingListBlocks } from './utils/listwalker'; +import ListWalker, { + iterateSiblingListBlocks, + ListBlocksIterable +} from './utils/listwalker'; import '../../theme/documentlist.css'; @@ -518,9 +521,12 @@ function modelChangePostFixer( model, writer, attributeNames, documentListEditin * @param {Object} modelAttributes * @returns {Boolean} If a post-fixer made a change of the model tree, it should return `true`. */ - const listNodes = { [ Symbol.iterator ]: () => iterateSiblingListBlocks( listHead, 'forward' ) }; - - applied = documentListEditing.fire( 'postFixer', { listHead, writer, seenIds, listNodes } ) || applied; + applied = documentListEditing.fire( 'postFixer', { + listNodes: new ListBlocksIterable( listHead ), + listHead, + writer, + seenIds + } ) || applied; } return applied; diff --git a/packages/ckeditor5-list/src/documentlist/utils/listwalker.js b/packages/ckeditor5-list/src/documentlist/utils/listwalker.js index 76233851e44..0beb11eab5e 100644 --- a/packages/ckeditor5-list/src/documentlist/utils/listwalker.js +++ b/packages/ckeditor5-list/src/documentlist/utils/listwalker.js @@ -210,7 +210,7 @@ export default class ListWalker { * @protected * @param {module:engine/model/node~Node} node The model node. * @param {'backward'|'forward'} [direction='forward'] Iteration direction. - * @returns {Iterable.} The object with `node` and `previous` + * @returns {Iterator.} The object with `node` and `previous` * {@link module:engine/model/element~Element blocks}. */ export function* iterateSiblingListBlocks( node, direction = 'forward' ) { @@ -225,9 +225,35 @@ export function* iterateSiblingListBlocks( node, direction = 'forward' ) { } } +/** + * The iterable protocol over the list elements. + * + * @protected + */ +export class ListBlocksIterable { + /** + * @param {module:engine/model/element~Element} listHead The head element of a list. + */ + constructor( listHead ) { + this._listHead = listHead; + } + + /** + * List blocks iterator. + * + * Iterates over all blocks of a list. + * + * @returns {Iterator.} + */ + [ Symbol.iterator ]() { + return iterateSiblingListBlocks( this._listHead, 'forward' ); + } +} + /** * Object returned by `iterateSiblingListBlocks()` when traversing a list. * + * @protected * @typedef {Object} module:list/documentlist/utils/listwalker~ListIteratorValue * @property {module:engine/model/node~Node} node The current list node. * @property {module:engine/model/node~Node} previous The previous list node. From 406f00bd97ed649b5381c5b4987f771686502f35 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Mon, 7 Mar 2022 15:52:10 +0100 Subject: [PATCH 19/25] Updated docs. --- .../src/documentlist/documentlistediting.js | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/packages/ckeditor5-list/src/documentlist/documentlistediting.js b/packages/ckeditor5-list/src/documentlist/documentlistediting.js index 8dd0a5843b3..670235b1f92 100644 --- a/packages/ckeditor5-list/src/documentlist/documentlistediting.js +++ b/packages/ckeditor5-list/src/documentlist/documentlistediting.js @@ -79,9 +79,10 @@ export default class DocumentListEditing extends Plugin { */ init() { /** - * TODO + * The list of registered downcast strategies. * * @private + * @type {Array.} */ this._downcastStrategies = []; @@ -149,14 +150,19 @@ export default class DocumentListEditing extends Plugin { } /** - * TODO + * Registers a downcast strategyl + * + * **Note**: Strategies must be registered in the `Plugin#init()` phase so that it can be applied + * in the `DocumentListEditing#afterInit()`. + * + * @param {module:list/documentlist/documentlistediting~DowncastStrategy} strategy The downcast strategy to register. */ registerDowncastStrategy( strategy ) { this._downcastStrategies.push( strategy ); } /** - * TODO + * Returns list of model attribute names that should affect downcast conversion. * * @private */ @@ -428,6 +434,13 @@ export default class DocumentListEditing extends Plugin { } } +/** + * @typedef {Object} module:list/documentlist/documentlistediting~DowncastStrategy + * @property {'list'|'item'} scope The scope of the downcast (whether it applies to LI or OL/UL). + * @property {String} attributeName The model attribute name. + * @property {Function} setAttributeOnDowncast Sets the property on the view element. + */ + // Post-fixer that reacts to changes on document and fixes incorrect model states (invalid `listItemId` and `listIndent` values). // // In the example below, there is a correct list structure. From 58fe16d83e0f6afe775eaafb36539a47c13ed5ae Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Mon, 7 Mar 2022 16:47:11 +0100 Subject: [PATCH 20/25] Manual tests cleaning. --- .../tests/manual/documentlist.html | 16 +- .../tests/manual/documentlist.js | 7 + .../manual/documentlist-properties-all.html | 47 ++++ .../manual/documentlist-properties-all.js | 123 +++++++++ .../manual/documentlist-properties-all.md | 8 + .../tests/manual/documentlist-properties.html | 235 ++++++------------ .../tests/manual/documentlist.js | 6 +- .../tests/manual/documentlist.md | 9 +- 8 files changed, 278 insertions(+), 173 deletions(-) create mode 100644 packages/ckeditor5-list/tests/manual/documentlist-properties-all.html create mode 100644 packages/ckeditor5-list/tests/manual/documentlist-properties-all.js create mode 100644 packages/ckeditor5-list/tests/manual/documentlist-properties-all.md diff --git a/packages/ckeditor5-html-support/tests/manual/documentlist.html b/packages/ckeditor5-html-support/tests/manual/documentlist.html index 9b99dc92928..5dc7186e511 100644 --- a/packages/ckeditor5-html-support/tests/manual/documentlist.html +++ b/packages/ckeditor5-html-support/tests/manual/documentlist.html @@ -1,7 +1,7 @@ - - - +
        + + +
          @@ -17,16 +17,20 @@
        diff --git a/packages/ckeditor5-html-support/tests/manual/documentlist.js b/packages/ckeditor5-html-support/tests/manual/documentlist.js index 87957aae174..b79d78b36c4 100644 --- a/packages/ckeditor5-html-support/tests/manual/documentlist.js +++ b/packages/ckeditor5-html-support/tests/manual/documentlist.js @@ -13,6 +13,7 @@ import Italic from '@ckeditor/ckeditor5-basic-styles/src/italic'; import Strikethrough from '@ckeditor/ckeditor5-basic-styles/src/strikethrough'; import DocumentListProperties from '@ckeditor/ckeditor5-list/src/documentlistproperties'; import SourceEditing from '@ckeditor/ckeditor5-source-editing/src/sourceediting'; +import Indent from '@ckeditor/ckeditor5-indent/src/indent'; import GeneralHtmlSupport from '../../src/generalhtmlsupport'; @@ -25,6 +26,7 @@ ClassicEditor Italic, Paragraph, Strikethrough, + Indent, SourceEditing, GeneralHtmlSupport ], @@ -32,6 +34,7 @@ ClassicEditor 'sourceEditing', '|', 'numberedList', 'bulletedList', '|', 'outdent', 'indent', '|', + 'outdent', 'indent', '|', 'bold', 'italic', 'strikethrough' ], list: { @@ -58,3 +61,7 @@ ClassicEditor .catch( err => { console.error( err.stack ); } ); + +document.getElementById( 'chbx-show-borders' ).addEventListener( 'change', () => { + document.body.classList.toggle( 'show-borders' ); +} ); diff --git a/packages/ckeditor5-list/tests/manual/documentlist-properties-all.html b/packages/ckeditor5-list/tests/manual/documentlist-properties-all.html new file mode 100644 index 00000000000..67eb57eaea7 --- /dev/null +++ b/packages/ckeditor5-list/tests/manual/documentlist-properties-all.html @@ -0,0 +1,47 @@ +
        + + +
        + +
        +
          +
        1. A numbered list with lower-roman numbering that starts at 3.
          2. +
        2. Item with multiple blocks...
          ...and this is the 3rd block of it.
          3. +

        3. This is a list item with two paragraphs.

          And here is the last one.

          4. +
        +
          +
        • +

          This is a multi block item of a square bulleted list.

          +
          +

          And there is a block widget above.

          +
          • +
        • +

          Other item with multi blocks...

          +

          ...and nested list:

          +
            +

          1. It's numbered, lower-alpha that starts at 3...

            2. +

          2. ...and it is reversed.

            3. +
          +

          Here is also a block after a nested list.

          +
          • +
        +
        + + diff --git a/packages/ckeditor5-list/tests/manual/documentlist-properties-all.js b/packages/ckeditor5-list/tests/manual/documentlist-properties-all.js new file mode 100644 index 00000000000..786d6c4a734 --- /dev/null +++ b/packages/ckeditor5-list/tests/manual/documentlist-properties-all.js @@ -0,0 +1,123 @@ +/** + * @license Copyright (c) 2003-2022, CKSource Holding sp. z o.o. All rights reserved. + * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license + */ + +/* globals console, window, document */ + +import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor'; +import Alignment from '@ckeditor/ckeditor5-alignment/src/alignment'; +import AutoImage from '@ckeditor/ckeditor5-image/src/autoimage'; +import CodeBlock from '@ckeditor/ckeditor5-code-block/src/codeblock'; +import EasyImage from '@ckeditor/ckeditor5-easy-image/src/easyimage'; +import HorizontalLine from '@ckeditor/ckeditor5-horizontal-line/src/horizontalline'; +import HtmlEmbed from '@ckeditor/ckeditor5-html-embed/src/htmlembed'; +import HtmlComment from '@ckeditor/ckeditor5-html-support/src/htmlcomment'; +import ImageResize from '@ckeditor/ckeditor5-image/src/imageresize'; +import LinkImage from '@ckeditor/ckeditor5-link/src/linkimage'; +import PageBreak from '@ckeditor/ckeditor5-page-break/src/pagebreak'; +import SourceEditing from '@ckeditor/ckeditor5-source-editing/src/sourceediting'; +import TableCaption from '@ckeditor/ckeditor5-table/src/tablecaption'; +import CloudServices from '@ckeditor/ckeditor5-cloud-services/src/cloudservices'; +import ImageUpload from '@ckeditor/ckeditor5-image/src/imageupload'; +import Essentials from '@ckeditor/ckeditor5-essentials/src/essentials'; +import BlockQuote from '@ckeditor/ckeditor5-block-quote/src/blockquote'; +import Bold from '@ckeditor/ckeditor5-basic-styles/src/bold'; +import Heading from '@ckeditor/ckeditor5-heading/src/heading'; +import Image from '@ckeditor/ckeditor5-image/src/image'; +import ImageCaption from '@ckeditor/ckeditor5-image/src/imagecaption'; +import ImageStyle from '@ckeditor/ckeditor5-image/src/imagestyle'; +import ImageToolbar from '@ckeditor/ckeditor5-image/src/imagetoolbar'; +import Indent from '@ckeditor/ckeditor5-indent/src/indent'; +import Italic from '@ckeditor/ckeditor5-basic-styles/src/italic'; +import Link from '@ckeditor/ckeditor5-link/src/link'; +import MediaEmbed from '@ckeditor/ckeditor5-media-embed/src/mediaembed'; +import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; +import Table from '@ckeditor/ckeditor5-table/src/table'; +import TableToolbar from '@ckeditor/ckeditor5-table/src/tabletoolbar'; + +import { CS_CONFIG } from '@ckeditor/ckeditor5-cloud-services/tests/_utils/cloud-services-config'; + +import DocumentList from '../../src/documentlist'; +import DocumentListProperties from '../../src/documentlistproperties'; + +ClassicEditor + .create( document.querySelector( '#editor' ), { + ...( { + plugins: [ + Essentials, BlockQuote, Bold, Heading, Image, ImageCaption, ImageStyle, ImageToolbar, Indent, Italic, Link, + MediaEmbed, Paragraph, Table, TableToolbar, CodeBlock, TableCaption, EasyImage, ImageResize, LinkImage, + AutoImage, HtmlEmbed, HtmlComment, Alignment, PageBreak, HorizontalLine, ImageUpload, + CloudServices, SourceEditing, DocumentList, DocumentListProperties + ], + toolbar: [ + 'sourceEditing', '|', + 'numberedList', 'bulletedList', '|', + 'outdent', 'indent', '|', + 'heading', '|', + 'bold', 'italic', 'link', '|', + 'blockQuote', 'uploadImage', 'insertTable', 'mediaEmbed', 'codeBlock', '|', + 'htmlEmbed', '|', + 'alignment', '|', + 'pageBreak', 'horizontalLine', '|', + 'undo', 'redo' + ], + cloudServices: CS_CONFIG, + table: { + contentToolbar: [ + 'tableColumn', 'tableRow', 'mergeTableCells', 'toggleTableCaption' + ] + }, + image: { + styles: [ + 'alignCenter', + 'alignLeft', + 'alignRight' + ], + resizeOptions: [ + { + name: 'resizeImage:original', + label: 'Original size', + value: null + }, + { + name: 'resizeImage:50', + label: '50%', + value: '50' + }, + { + name: 'resizeImage:75', + label: '75%', + value: '75' + } + ], + toolbar: [ + 'imageTextAlternative', 'toggleImageCaption', '|', + 'imageStyle:inline', 'imageStyle:wrapText', 'imageStyle:breakText', 'imageStyle:side', '|', + 'resizeImage' + ] + }, + placeholder: 'Type the content here!', + htmlEmbed: { + showPreviews: true, + sanitizeHtml: html => ( { html, hasChange: false } ) + } + } ), + list: { + properties: { + styles: true, + startIndex: true, + reversed: true + } + } + } ) + .then( editor => { + window.editor = editor; + } ) + .catch( err => { + console.error( err.stack ); + } ); + +document.getElementById( 'chbx-show-borders' ).addEventListener( 'change', () => { + document.body.classList.toggle( 'show-borders' ); +} ); diff --git a/packages/ckeditor5-list/tests/manual/documentlist-properties-all.md b/packages/ckeditor5-list/tests/manual/documentlist-properties-all.md new file mode 100644 index 00000000000..2152f0ca9b7 --- /dev/null +++ b/packages/ckeditor5-list/tests/manual/documentlist-properties-all.md @@ -0,0 +1,8 @@ +# List properties feature + +This is a single editor instance with all `DocumentListProperties` enabled; +* list style; +* list start (for numbered lists); +* list reversed (for numbered lists). + +Border colors: Blue for `ul` and `ol`, red for `li`. diff --git a/packages/ckeditor5-list/tests/manual/documentlist-properties.html b/packages/ckeditor5-list/tests/manual/documentlist-properties.html index 1c4ac27e57c..fcdefd07c35 100644 --- a/packages/ckeditor5-list/tests/manual/documentlist-properties.html +++ b/packages/ckeditor5-list/tests/manual/documentlist-properties.html @@ -2,76 +2,49 @@

        With styles and other properties

        Styles + Start index + Reversed

        -
          -
        1. a
          2. -
        2. b
          x
          3. -
        3. c
          4. +

          Ordered list

          +
            +
          1. First item
            2. +
          2. Second item
            3. +
          3. Third item
          -
            -
          • -

            a

            -
            -

            b

            -
            • -
          • -

            c

            -

            d

            -
              -

            1. e

              2. -

            2. f

              3. -
            -

            g

            -
            • +

            Unordered list

            +
              +
            • First item
              • +
            • Second item
              • +
            • Third item

        Styles + Start index

        -
          -
        1. a
          2. -
        2. b
          x
          3. -
        3. c
          4. +

          Ordered list

          +
            +
          1. First item
            2. +
          2. Second item
            3. +
          3. Third item
          -
            -
          • -

            a

            -
            -

            b

            -
            • -
          • -

            c

            -

            d

            -
              -

            1. e

              2. -

            2. f

              3. -
            -

            g

            -
            • +

            Unordered list

            +
              +
            • First item
              • +
            • Second item
              • +
            • Third item

        Styles + Reversed

        -
          -
        1. a
          2. -
        2. b
          x
          3. -
        3. c
          4. +

          Ordered list

          +
            +
          1. First item
            2. +
          2. Second item
            3. +
          3. Third item
          -
            -
          • -

            a

            -
            -

            b

            -
            • -
          • -

            c

            -

            d

            -
              -

            1. e

              2. -

            2. f

              3. -
            -

            g

            -
            • +

            Unordered list

            +
              +
            • First item
              • +
            • Second item
              • +
            • Third item
        @@ -79,142 +52,82 @@

        Without styles, just extra properties

        Start index + Reversed

        -
          -
        1. a
          2. -
        2. b
          x
          3. -
        3. c
          4. +

          Ordered list

          +
            +
          1. First item
            2. +
          2. Second item
            3. +
          3. Third item
          +

          Unordered list

            -
          • -

            a

            -
            -

            b

            -
            • -
          • -

            c

            -

            d

            -
              -

            1. e

              2. -

            2. f

              3. -
            -

            g

            -
            • +
          • First item
            • +
          • Second item
            • +
          • Third item

        Start index

        -
          -
        1. a
          2. -
        2. b
          x
          3. -
        3. c
          4. +

          Ordered list

          +
            +
          1. First item
            2. +
          2. Second item
            3. +
          3. Third item
          +

          Unordered list

            -
          • -

            a

            -
            -

            b

            -
            • -
          • -

            c

            -

            d

            -
              -

            1. e

              2. -

            2. f

              3. -
            -

            g

            -
            • +
          • First item
            • +
          • Second item
            • +
          • Third item

        Reversed

        +

        Ordered list

          -
        1. a
          2. -
        2. b
          x
          3. -
        3. c
          4. +
        4. First item
          5. +
        5. Second item
          6. +
        6. Third item
        +

        Unordered list

          -
        • -

          a

          -
          -

          b

          -
          • -
        • -

          c

          -

          d

          -
            -

          1. e

            2. -

          2. f

            3. -
          -

          g

          -
          • +
        • First item
          • +
        • Second item
          • +
        • Third item

        Just styles

        -
          -
        1. a
          2. -
        2. b
          x
          3. -
        3. c
          4. +

          Ordered list

          +
            +
          1. First item
            2. +
          2. Second item
            3. +
          3. Third item
          -
            -
          • -

            a

            -
            -

            b

            -
            • -
          • -

            c

            -

            d

            -
              -

            1. e

              2. -

            2. f

              3. -
            -

            g

            -
            • +

            Unordered list

            +
              +
            • First item
              • +
            • Second item
              • +
            • Third item

        No properties enabled

        +

        Ordered list

          -
        1. a
          2. -
        2. b
          x
          3. -
        3. c
          4. +
        4. First item
          5. +
        5. Second item
          6. +
        6. Third item
        +

        Unordered list

          -
        • -

          a

          -
          -

          b

          -
          • -
        • -

          c

          -

          d

          -
            -

          1. e

            2. -

          2. f

            3. -
          -

          g

          -
          • +
        • First item
          • +
        • Second item
          • +
        • Third item
        - - diff --git a/packages/ckeditor5-list/tests/manual/documentlist.js b/packages/ckeditor5-list/tests/manual/documentlist.js index 901da48a889..4df9cec5aa9 100644 --- a/packages/ckeditor5-list/tests/manual/documentlist.js +++ b/packages/ckeditor5-list/tests/manual/documentlist.js @@ -109,8 +109,6 @@ ClassicEditor console.error( err.stack ); } ); -const onShowBordersChange = () => { +document.getElementById( 'chbx-show-borders' ).addEventListener( 'change', () => { document.body.classList.toggle( 'show-borders' ); -}; - -document.getElementById( 'chbx-show-borders' ).addEventListener( 'change', onShowBordersChange ); +} ); diff --git a/packages/ckeditor5-list/tests/manual/documentlist.md b/packages/ckeditor5-list/tests/manual/documentlist.md index 209d8a97043..a93659d24e7 100644 --- a/packages/ckeditor5-list/tests/manual/documentlist.md +++ b/packages/ckeditor5-list/tests/manual/documentlist.md @@ -1,5 +1,10 @@ ## Document List -TODO +The basic document list feature. -Border colors: Blue for `ul` and `ol`, red for `li`. \ No newline at end of file +Supported features: +* list type (bulleted, numbered); +* list indentation; +* merging/splitting list items. + +Border colors: Blue for `ul` and `ol`, red for `li`. From 5a56385de264fd6366c6f0592e5cf3ca8ddfab5c Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Tue, 8 Mar 2022 12:37:02 +0100 Subject: [PATCH 21/25] Added basic tests for 100CC. --- .../tests/documentlist/documentlistediting.js | 75 ++++++++++++++++++- 1 file changed, 74 insertions(+), 1 deletion(-) diff --git a/packages/ckeditor5-list/tests/documentlist/documentlistediting.js b/packages/ckeditor5-list/tests/documentlist/documentlistediting.js index 9f770111f8e..797358327a4 100644 --- a/packages/ckeditor5-list/tests/documentlist/documentlistediting.js +++ b/packages/ckeditor5-list/tests/documentlist/documentlistediting.js @@ -14,17 +14,19 @@ import IndentEditing from '@ckeditor/ckeditor5-indent/src/indentediting'; import TableEditing from '@ckeditor/ckeditor5-table/src/tableediting'; import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; +import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor'; import { getData as getModelData, parse as parseModel, setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; +import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; import ListEditing from '../../src/list/listediting'; import DocumentListIndentCommand from '../../src/documentlist/documentlistindentcommand'; import DocumentListSplitCommand from '../../src/documentlist/documentlistsplitcommand'; import stubUid from './_utils/uid'; -import { prepareTest } from './_utils/utils'; +import { modelList, prepareTest } from './_utils/utils'; describe( 'DocumentListEditing', () => { let editor, model, modelDoc, modelRoot, view; @@ -718,3 +720,74 @@ describe( 'DocumentListEditing', () => { } ); } ); } ); + +describe( 'DocumentListEditing - registerDowncastStrategy()', () => { + let editor, model, view; + + afterEach( async () => { + await editor.destroy(); + } ); + + it( 'should allow registering strategy for list elements', async () => { + await createEditor( class CustomPlugin extends Plugin { + init() { + this.editor.plugins.get( 'DocumentListEditing' ).registerDowncastStrategy( { + scope: 'list', + attributeName: 'someFoo', + + setAttributeOnDowncast( writer, attributeValue, viewElement ) { + writer.setAttribute( 'data-foo', attributeValue, viewElement ); + } + } ); + } + } ); + + setModelData( model, modelList( ` + * foo + * bar + ` ) ); + + expect( getViewData( view, { withoutSelection: true } ) ).to.equalMarkup( + '
          ' + + '
        • foo
          • ' + + '
        • bar
          • ' + + '
        ' + ); + } ); + + it( 'should allow registering strategy for list items elements', async () => { + await createEditor( class CustomPlugin extends Plugin { + init() { + this.editor.plugins.get( 'DocumentListEditing' ).registerDowncastStrategy( { + scope: 'item', + attributeName: 'someFoo', + + setAttributeOnDowncast( writer, attributeValue, viewElement ) { + writer.setAttribute( 'data-foo', attributeValue, viewElement ); + } + } ); + } + } ); + + setModelData( model, modelList( ` + * foo + * bar + ` ) ); + + expect( getViewData( view, { withoutSelection: true } ) ).to.equalMarkup( + '
          ' + + '
        • foo
          • ' + + '
        • bar
          • ' + + '
        ' + ); + } ); + + async function createEditor( extraPlugin ) { + editor = await VirtualTestEditor.create( { + plugins: [ Paragraph, DocumentListEditing, UndoEditing, extraPlugin ] + } ); + + model = editor.model; + view = editor.editing.view; + } +} ); From 5dd4a9cbceb89f70180a7bfd032b0288061b1006 Mon Sep 17 00:00:00 2001 From: Arkadiusz Filipczak Date: Wed, 16 Mar 2022 14:53:27 +0100 Subject: [PATCH 22/25] GHS: DocumentListSupport - tests --- .../src/integrations/documentlist.js | 87 +-- .../tests/integrations/documentlist.js | 497 ++++++++++++++++++ 2 files changed, 541 insertions(+), 43 deletions(-) create mode 100644 packages/ckeditor5-html-support/tests/integrations/documentlist.js diff --git a/packages/ckeditor5-html-support/src/integrations/documentlist.js b/packages/ckeditor5-html-support/src/integrations/documentlist.js index 3fcae5d983e..05dea1cd337 100644 --- a/packages/ckeditor5-html-support/src/integrations/documentlist.js +++ b/packages/ckeditor5-html-support/src/integrations/documentlist.js @@ -7,6 +7,7 @@ * @module html-support/integrations/documentlist */ +import { isEqual } from 'lodash-es'; import { Plugin } from 'ckeditor5/src/core'; import { setViewAttributes } from '../conversionutils.js'; @@ -81,62 +82,62 @@ export default class DocumentListElementSupport extends Plugin { dispatcher.on( 'element:ol', viewToModelListAttributeConverter( 'htmlListAttributes', dataFilter ), { priority: 'low' } ); dispatcher.on( 'element:li', viewToModelListAttributeConverter( 'htmlLiAttributes', dataFilter ), { priority: 'low' } ); } ); + } ); - // Make sure that all items in a single list (items at the same level & listType) have the same properties. - // Note: This is almost exact copy from DocumentListPropertiesEditing. - documentListEditing.on( 'postFixer', ( evt, { listNodes, writer } ) => { - const previousNodesByIndent = []; // Last seen nodes of lower indented lists. + // Make sure that all items in a single list (items at the same level & listType) have the same properties. + // Note: This is almost exact copy from DocumentListPropertiesEditing. + documentListEditing.on( 'postFixer', ( evt, { listNodes, writer } ) => { + const previousNodesByIndent = []; // Last seen nodes of lower indented lists. - for ( const { node, previous } of listNodes ) { - // For the first list block there is nothing to compare with. - if ( !previous ) { - continue; - } + for ( const { node, previous } of listNodes ) { + // For the first list block there is nothing to compare with. + if ( !previous ) { + continue; + } - const nodeIndent = node.getAttribute( 'listIndent' ); - const previousNodeIndent = previous.getAttribute( 'listIndent' ); + const nodeIndent = node.getAttribute( 'listIndent' ); + const previousNodeIndent = previous.getAttribute( 'listIndent' ); - let previousNodeInList = null; // It's like `previous` but has the same indent as current node. + let previousNodeInList = null; // It's like `previous` but has the same indent as current node. - // Let's find previous node for the same indent. - // We're going to need that when we get back to previous indent. - if ( nodeIndent > previousNodeIndent ) { - previousNodesByIndent[ previousNodeIndent ] = previous; - } - // Restore the one for given indent. - else if ( nodeIndent < previousNodeIndent ) { - previousNodeInList = previousNodesByIndent[ nodeIndent ]; - previousNodesByIndent.length = nodeIndent; - } - // Same indent. - else { - previousNodeInList = previous; - } + // Let's find previous node for the same indent. + // We're going to need that when we get back to previous indent. + if ( nodeIndent > previousNodeIndent ) { + previousNodesByIndent[ previousNodeIndent ] = previous; + } + // Restore the one for given indent. + else if ( nodeIndent < previousNodeIndent ) { + previousNodeInList = previousNodesByIndent[ nodeIndent ]; + previousNodesByIndent.length = nodeIndent; + } + // Same indent. + else { + previousNodeInList = previous; + } - // This is a first item of a nested list. - if ( !previousNodeInList ) { - continue; - } + // This is a first item of a nested list. + if ( !previousNodeInList ) { + continue; + } - if ( previousNodeInList.getAttribute( 'listType' ) == node.getAttribute( 'listType' ) ) { - const value = previousNodeInList.getAttribute( 'htmlListAttributes' ); + if ( previousNodeInList.getAttribute( 'listType' ) == node.getAttribute( 'listType' ) ) { + const value = previousNodeInList.getAttribute( 'htmlListAttributes' ); - if ( node.getAttribute( 'htmlListAttributes' ) != value ) { - writer.setAttribute( 'htmlListAttributes', value, node ); - evt.return = true; - } + if ( !isEqual( node.getAttribute( 'htmlListAttributes' ), value ) ) { + writer.setAttribute( 'htmlListAttributes', value, node ); + evt.return = true; } + } - if ( previousNodeInList.getAttribute( 'listItemId' ) == node.getAttribute( 'listItemId' ) ) { - const value = previousNodeInList.getAttribute( 'htmlLiAttributes' ); + if ( previousNodeInList.getAttribute( 'listItemId' ) == node.getAttribute( 'listItemId' ) ) { + const value = previousNodeInList.getAttribute( 'htmlLiAttributes' ); - if ( node.getAttribute( 'htmlLiAttributes' ) != value ) { - writer.setAttribute( 'htmlLiAttributes', value, node ); - evt.return = true; - } + if ( !isEqual( node.getAttribute( 'htmlLiAttributes' ), value ) ) { + writer.setAttribute( 'htmlLiAttributes', value, node ); + evt.return = true; } } - } ); + } } ); } } diff --git a/packages/ckeditor5-html-support/tests/integrations/documentlist.js b/packages/ckeditor5-html-support/tests/integrations/documentlist.js new file mode 100644 index 00000000000..8460d200675 --- /dev/null +++ b/packages/ckeditor5-html-support/tests/integrations/documentlist.js @@ -0,0 +1,497 @@ +/** + * @license Copyright (c) 2003-2022, CKSource Holding sp. z o.o. All rights reserved. + * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license + */ + +import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor'; +import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; +import GeneralHtmlSupport from '../../src/generalhtmlsupport'; +import DocumentListEditing from '@ckeditor/ckeditor5-list/src/documentlist/documentlistediting'; + +import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; +import stubUid from '@ckeditor/ckeditor5-list/tests/documentlist/_utils/uid'; + +import { getModelDataWithAttributes } from '../_utils/utils'; +import { getData as getModelData, setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; + +/* global document */ + +describe( 'DocumentListElementSupport', () => { + let editor, model, editorElement, dataFilter; + + testUtils.createSinonSandbox(); + + beforeEach( async () => { + editorElement = document.createElement( 'div' ); + document.body.appendChild( editorElement ); + + editor = await ClassicTestEditor + .create( editorElement, { + plugins: [ DocumentListEditing, Paragraph, GeneralHtmlSupport ] + } ); + model = editor.model; + dataFilter = editor.plugins.get( 'DataFilter' ); + + stubUid(); + } ); + + afterEach( () => { + editorElement.remove(); + + return editor.destroy(); + } ); + + describe( 'upcast', () => { + it( 'should allow attributes', () => { + dataFilter.allowElement( /^(ul|ol)$/ ); + dataFilter.allowAttributes( { name: /^(ul|ol)$/, attributes: { 'data-foo': true } } ); + dataFilter.allowAttributes( { name: 'li', attributes: { 'data-bar': true } } ); + + editor.setData( '
        • Foo
        • Bar
        ' ); + + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: + '' + + 'Foo' + + '' + + '' + + 'Bar' + + '', + attributes: { + 1: { + attributes: { 'data-bar': 'A' } + }, + 2: { + attributes: { 'data-foo': 'foo' } + }, + 3: { + attributes: { 'data-bar': 'B' } + }, + 4: { + attributes: { 'data-foo': 'foo' } + } + } + } ); + } ); + + it( 'should allow attributes (classes)', () => { + dataFilter.allowElement( /^(ul|ol)$/ ); + dataFilter.allowAttributes( { name: /^(ul|ol)$/, classes: 'foo' } ); + dataFilter.allowAttributes( { name: 'li', classes: /^(bar|baz)$/ } ); + + editor.setData( '
        1. Foo
        2. Bar
        ' ); + + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: + '' + + 'Foo' + + '' + + '' + + 'Bar' + + '', + attributes: { + 1: { + classes: [ 'bar' ] + }, + 2: { + classes: [ 'foo' ] + }, + 3: { + classes: [ 'baz' ] + }, + 4: { + classes: [ 'foo' ] + } + } + } ); + } ); + + it( 'should allow attributes (styles)', () => { + dataFilter.allowElement( /^(ul|ol)$/ ); + dataFilter.allowAttributes( { name: /^(ul|ol)$/, styles: { background: 'blue' } } ); + dataFilter.allowAttributes( { name: 'li', styles: { color: /^(red|green)$/ } } ); + + editor.setData( '
        1. Foo
        2. Bar
        ' ); + + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: + '' + + 'Foo' + + '' + + '' + + 'Bar' + + '', + attributes: { + 1: { + styles: { color: 'red' } + }, + 2: { + styles: { background: 'blue' } + }, + 3: { + styles: { color: 'green' } + }, + 4: { + styles: { background: 'blue' } + } + } + } ); + } ); + + it( 'should allow attributes (complex))', () => { + dataFilter.allowElement( /^(ul|ol)$/ ); + dataFilter.allowAttributes( { name: /^(ul|ol)$/, attributes: { 'data-foo': true } } ); + dataFilter.allowAttributes( { name: 'li', attributes: { 'data-bar': true } } ); + + editor.setData( + '
          ' + + '
        • ' + + '

          Foo

          ' + + '
            ' + + '
          1. Bar
            2. ' + + '
          ' + + '

          Baz

          ' + + '
          • ' + + '
        ' ); + + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: + '' + + 'Foo' + + '' + + '' + + 'Bar' + + '' + + '' + + 'Baz' + + '', + attributes: { + 1: { + attributes: { 'data-bar': 'A' } + }, + 2: { + attributes: { 'data-foo': 'foo' } + }, + 3: { + attributes: { 'data-bar': 'B' } + }, + 4: { + attributes: { 'data-foo': 'bar' } + }, + 5: { + attributes: { 'data-bar': 'A' } + }, + 6: { + attributes: { 'data-foo': 'foo' } + } + } + } ); + } ); + + it( 'should allow attributes (non-list item content)', () => { + dataFilter.allowElement( /^(ul|ol|div)$/ ); + dataFilter.allowAttributes( { name: /^(ul|ol)$/, attributes: { 'data-foo': true } } ); + dataFilter.allowAttributes( { name: /^(li|div)$/, attributes: { 'data-bar': true } } ); + + model.schema.register( 'div', { inheritAllFrom: '$block' } ); + editor.conversion.elementToElement( { view: 'div', model: 'div' } ); + + editor.model.schema.addAttributeCheck( ( context, attributeName ) => { + if ( context.endsWith( 'div' ) && attributeName == 'listItemId' ) { + return false; + } + } ); + + editor.setData( '
        • Foo
          Bar
        ' ); + + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: + '' + + 'Foo' + + '' + + '
        Bar
        ', + attributes: { + 1: { + attributes: { 'data-bar': 'A' } + }, + 2: { + attributes: { 'data-foo': 'foo' } + }, + 3: { + attributes: { 'data-bar': 'B' } + } + } + } ); + } ); + + it( 'should disallow attributes', () => { + dataFilter.allowElement( /^(ul|ol)$/ ); + dataFilter.allowAttributes( { name: /^(ul|ol)$/, attributes: { 'data-foo': true } } ); + dataFilter.allowAttributes( { name: 'li', attributes: { 'data-bar': true } } ); + + dataFilter.disallowAttributes( { name: /^(ul|ol)$/, attributes: { 'data-foo': true } } ); + dataFilter.disallowAttributes( { name: 'li', attributes: { 'data-bar': true } } ); + + editor.setData( '
        • Foo
        • Bar
        ' ); + + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: + '' + + 'Foo' + + '' + + '' + + 'Bar' + + '', + attributes: { + 1: {}, + 2: {}, + 3: {}, + 4: {} + } + } ); + } ); + + it( 'should disallow attributes (classes)', () => { + dataFilter.allowElement( /^(ul|ol)$/ ); + dataFilter.allowAttributes( { name: /^(ul|ol)$/, classes: 'foo' } ); + dataFilter.allowAttributes( { name: 'li', classes: /^(bar|baz)$/ } ); + + dataFilter.disallowAttributes( { name: /^(ul|ol)$/, classes: 'foo' } ); + dataFilter.disallowAttributes( { name: 'li', classes: /^(bar|baz)$/ } ); + + editor.setData( '
        1. Foo
        2. Bar
        ' ); + + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: + '' + + 'Foo' + + '' + + '' + + 'Bar' + + '', + attributes: { + 1: {}, + 2: {}, + 3: {}, + 4: {} + } + } ); + } ); + + it( 'should disallow attributes (styles)', () => { + dataFilter.allowElement( /^(ul|ol)$/ ); + dataFilter.allowAttributes( { name: /^(ul|ol)$/, styles: { background: 'blue' } } ); + dataFilter.allowAttributes( { name: 'li', styles: { color: /^(red|green)$/ } } ); + + dataFilter.disallowAttributes( { name: /^(ul|ol)$/, styles: { background: 'blue' } } ); + dataFilter.disallowAttributes( { name: 'li', styles: { color: /^(red|green)$/ } } ); + + editor.setData( '
        1. Foo
        2. Bar
        ' ); + + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: + '' + + 'Foo' + + '' + + '' + + 'Bar' + + '', + attributes: { + 1: {}, + 2: {}, + 3: {}, + 4: {} + } + } ); + } ); + } ); + + describe( 'post-fixer', () => { + describe( 'htmlListAttributes', () => { + it( 'should ensure that all items in a single list have the same `htmlListAttributes`', () => { + setModelData( model, + paragraph( '1.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + paragraph( '2.', '02', 0, 'numbered', { 'data-foo': 'A' } ) + + paragraph( '3.', '03', 0, 'numbered', { 'data-foo': 'B' } ) + + paragraph( '4.', '04', 0, 'numbered', { 'data-foo': 'A' } ) + + paragraph( '4.1.', '05', 1, 'bulleted', { 'data-foo': 'X' } ) + + paragraph( '4.2.', '06', 1, 'bulleted', { 'data-foo': 'Y' } ) + + paragraph( '4.3.', '07', 1, 'bulleted', { 'data-foo': 'X' } ) + + paragraph( '5.', '08', 0, 'numbered', { 'data-foo': 'C' } ) + + paragraph( 'A.', '09', 0, 'bulleted', { 'data-foo': 'B' } ) + + paragraph( 'B.', '10', 0, 'bulleted', { 'data-foo': 'C' } ) + + paragraph( 'C.', '11', 0, 'bulleted', { 'data-foo': 'B' } ) + ); + + expect( getModelData( model, { withoutSelection: true } ) ).to.equal( unquote( + paragraph( '1.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + paragraph( '2.', '02', 0, 'numbered', { 'data-foo': 'A' } ) + + paragraph( '3.', '03', 0, 'numbered', { 'data-foo': 'A' } ) + + paragraph( '4.', '04', 0, 'numbered', { 'data-foo': 'A' } ) + + paragraph( '4.1.', '05', 1, 'bulleted', { 'data-foo': 'X' } ) + + paragraph( '4.2.', '06', 1, 'bulleted', { 'data-foo': 'X' } ) + + paragraph( '4.3.', '07', 1, 'bulleted', { 'data-foo': 'X' } ) + + paragraph( '5.', '08', 0, 'numbered', { 'data-foo': 'A' } ) + + paragraph( 'A.', '09', 0, 'bulleted', { 'data-foo': 'B' } ) + + paragraph( 'B.', '10', 0, 'bulleted', { 'data-foo': 'B' } ) + + paragraph( 'C.', '11', 0, 'bulleted', { 'data-foo': 'B' } ) + ) ); + } ); + + it( 'should ensure that all list items have the same `htmlListAttributes` after removing a block between them', () => { + setModelData( model, + paragraph( '1.', '01', 0, 'bulleted', { 'data-foo': 'A' } ) + + paragraph( '2.', '02', 0, 'bulleted', { 'data-foo': 'A' } ) + + 'Foo' + + paragraph( '3.', '03', 0, 'bulleted', { 'data-foo': 'B' } ) + + paragraph( '4.', '04', 0, 'bulleted', { 'data-foo': 'B' } ) + ); + + model.change( writer => { + writer.remove( model.document.getRoot().getChild( 2 ) ); + } ); + + expect( getModelData( model, { withoutSelection: true } ) ).to.equal( unquote( + paragraph( '1.', '01', 0, 'bulleted', { 'data-foo': 'A' } ) + + paragraph( '2.', '02', 0, 'bulleted', { 'data-foo': 'A' } ) + + paragraph( '3.', '03', 0, 'bulleted', { 'data-foo': 'A' } ) + + paragraph( '4.', '04', 0, 'bulleted', { 'data-foo': 'A' } ) + ) ); + } ); + + it( 'should restore `htmlListAttributes` attribute after it\'s changed in one of the following items', () => { + setModelData( model, + paragraph( '1.', '01', 0, 'bulleted', { 'data-foo': 'A' } ) + + paragraph( '2.', '02', 0, 'bulleted', { 'data-foo': 'A' } ) + + paragraph( '3.', '03', 0, 'bulleted', { 'data-foo': 'A' } ) + ); + + model.change( writer => { + writer.setAttribute( + 'htmlListAttributes', + { attributes: { 'data-foo': 'B' } }, + model.document.getRoot().getChild( 2 ) + ); + } ); + + expect( getModelData( model, { withoutSelection: true } ) ).to.equal( unquote( + paragraph( '1.', '01', 0, 'bulleted', { 'data-foo': 'A' } ) + + paragraph( '2.', '02', 0, 'bulleted', { 'data-foo': 'A' } ) + + paragraph( '3.', '03', 0, 'bulleted', { 'data-foo': 'A' } ) + ) ); + } ); + + it( 'should change `htmlListAttributes` attribute for all the following items after the first one is changed', () => { + setModelData( model, + paragraph( '1.', '01', 0, 'bulleted', { 'data-foo': 'A' } ) + + paragraph( '2.', '02', 0, 'bulleted', { 'data-foo': 'A' } ) + + paragraph( '3.', '03', 0, 'bulleted', { 'data-foo': 'A' } ) + ); + + model.change( writer => { + writer.setAttribute( + 'htmlListAttributes', + { attributes: { 'data-foo': 'B' } }, + model.document.getRoot().getChild( 0 ) + ); + } ); + + expect( getModelData( model, { withoutSelection: true } ) ).to.equal( unquote( + paragraph( '1.', '01', 0, 'bulleted', { 'data-foo': 'B' } ) + + paragraph( '2.', '02', 0, 'bulleted', { 'data-foo': 'B' } ) + + paragraph( '3.', '03', 0, 'bulleted', { 'data-foo': 'B' } ) + ) ); + } ); + + function paragraph( text, id, indent, type, listAttributes ) { + const attrs = JSON.stringify( { attributes: listAttributes } ).replaceAll( '"', '"' ); + + return ( + `` + + text + + '' + ); + } + } ); + + describe( 'htmlLiAttributes', () => { + it( 'should ensure that all blocks of single list item have the same `htmlLiAttributes`', () => { + setModelData( model, + paragraph( 'A1.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + paragraph( 'A2.', '01', 0, 'numbered', { 'data-foo': 'B' } ) + + paragraph( 'A3.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + paragraph( 'B1.', '02', 0, 'numbered', { 'data-foo': 'X' } ) + + paragraph( 'B2.', '02', 0, 'numbered', { 'data-foo': 'Y' } ) + + paragraph( 'B3.', '02', 0, 'numbered', { 'data-foo': 'Z' } ) + ); + + expect( getModelData( model, { withoutSelection: true } ) ).to.equal( unquote( + paragraph( 'A1.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + paragraph( 'A2.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + paragraph( 'A3.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + paragraph( 'B1.', '02', 0, 'numbered', { 'data-foo': 'X' } ) + + paragraph( 'B2.', '02', 0, 'numbered', { 'data-foo': 'X' } ) + + paragraph( 'B3.', '02', 0, 'numbered', { 'data-foo': 'X' } ) + ) ); + } ); + + it( 'should restore `htmlLiAttributes` attribute after it\'s changed in one of the following items', () => { + setModelData( model, + paragraph( '1.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + paragraph( '2.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + paragraph( '3.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + ); + + model.change( writer => { + writer.setAttribute( + 'htmlLiAttributes', + { attributes: { 'data-foo': 'B' } }, + model.document.getRoot().getChild( 2 ) + ); + } ); + + expect( getModelData( model, { withoutSelection: true } ) ).to.equal( unquote( + paragraph( '1.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + paragraph( '2.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + paragraph( '3.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + ) ); + } ); + + it( 'should change `htmlLiAttributes` attribute for all the following items after the first one is changed', () => { + setModelData( model, + paragraph( '1.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + paragraph( '2.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + paragraph( '3.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + ); + + model.change( writer => { + writer.setAttribute( + 'htmlLiAttributes', + { attributes: { 'data-foo': 'B' } }, + model.document.getRoot().getChild( 0 ) + ); + } ); + + expect( getModelData( model, { withoutSelection: true } ) ).to.equal( unquote( + paragraph( '1.', '01', 0, 'numbered', { 'data-foo': 'B' } ) + + paragraph( '2.', '01', 0, 'numbered', { 'data-foo': 'B' } ) + + paragraph( '3.', '01', 0, 'numbered', { 'data-foo': 'B' } ) + ) ); + } ); + + function paragraph( text, id, indent, type, liAttributes ) { + const attrs = JSON.stringify( { attributes: liAttributes } ).replaceAll( '"', '"' ); + + return ( + `` + + text + + '' + ); + } + } ); + + function unquote( text ) { + return text.replaceAll( '"', '"' ); + } + } ); +} ); From 299bb38193360b79afb5f3b6c6b9a17f57aeec9d Mon Sep 17 00:00:00 2001 From: Arkadiusz Filipczak Date: Thu, 17 Mar 2022 14:29:32 +0100 Subject: [PATCH 23/25] GHS: DocumentListSupport - indenting list --- .../src/integrations/documentlist.js | 12 ++ .../tests/integrations/documentlist.js | 156 +++++++++++++----- 2 files changed, 130 insertions(+), 38 deletions(-) diff --git a/packages/ckeditor5-html-support/src/integrations/documentlist.js b/packages/ckeditor5-html-support/src/integrations/documentlist.js index 05dea1cd337..f7bd71045c8 100644 --- a/packages/ckeditor5-html-support/src/integrations/documentlist.js +++ b/packages/ckeditor5-html-support/src/integrations/documentlist.js @@ -84,6 +84,18 @@ export default class DocumentListElementSupport extends Plugin { } ); } ); + // Reset list attributes after indenting list items. + this.listenTo( editor.commands.get( 'indentList' ), 'afterExecute', ( evt, changedBlocks ) => { + editor.model.change( writer => { + for ( const node of changedBlocks ) { + // Just reset the attribute. + // If there is a previous indented list that this node should be merged into, + // the postfixer will unify all the attributes of both sub-lists. + writer.setAttribute( 'htmlListAttributes', {}, node ); + } + } ); + } ); + // Make sure that all items in a single list (items at the same level & listType) have the same properties. // Note: This is almost exact copy from DocumentListPropertiesEditing. documentListEditing.on( 'postFixer', ( evt, { listNodes, writer } ) => { diff --git a/packages/ckeditor5-html-support/tests/integrations/documentlist.js b/packages/ckeditor5-html-support/tests/integrations/documentlist.js index 8460d200675..05196bafb1a 100644 --- a/packages/ckeditor5-html-support/tests/integrations/documentlist.js +++ b/packages/ckeditor5-html-support/tests/integrations/documentlist.js @@ -402,44 +402,34 @@ describe( 'DocumentListElementSupport', () => { paragraph( '3.', '03', 0, 'bulleted', { 'data-foo': 'B' } ) ) ); } ); - - function paragraph( text, id, indent, type, listAttributes ) { - const attrs = JSON.stringify( { attributes: listAttributes } ).replaceAll( '"', '"' ); - - return ( - `` + - text + - '' - ); - } } ); describe( 'htmlLiAttributes', () => { it( 'should ensure that all blocks of single list item have the same `htmlLiAttributes`', () => { setModelData( model, - paragraph( 'A1.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + - paragraph( 'A2.', '01', 0, 'numbered', { 'data-foo': 'B' } ) + - paragraph( 'A3.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + - paragraph( 'B1.', '02', 0, 'numbered', { 'data-foo': 'X' } ) + - paragraph( 'B2.', '02', 0, 'numbered', { 'data-foo': 'Y' } ) + - paragraph( 'B3.', '02', 0, 'numbered', { 'data-foo': 'Z' } ) + liParagraph( 'A1.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + liParagraph( 'A2.', '01', 0, 'numbered', { 'data-foo': 'B' } ) + + liParagraph( 'A3.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + liParagraph( 'B1.', '02', 0, 'numbered', { 'data-foo': 'X' } ) + + liParagraph( 'B2.', '02', 0, 'numbered', { 'data-foo': 'Y' } ) + + liParagraph( 'B3.', '02', 0, 'numbered', { 'data-foo': 'Z' } ) ); expect( getModelData( model, { withoutSelection: true } ) ).to.equal( unquote( - paragraph( 'A1.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + - paragraph( 'A2.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + - paragraph( 'A3.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + - paragraph( 'B1.', '02', 0, 'numbered', { 'data-foo': 'X' } ) + - paragraph( 'B2.', '02', 0, 'numbered', { 'data-foo': 'X' } ) + - paragraph( 'B3.', '02', 0, 'numbered', { 'data-foo': 'X' } ) + liParagraph( 'A1.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + liParagraph( 'A2.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + liParagraph( 'A3.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + liParagraph( 'B1.', '02', 0, 'numbered', { 'data-foo': 'X' } ) + + liParagraph( 'B2.', '02', 0, 'numbered', { 'data-foo': 'X' } ) + + liParagraph( 'B3.', '02', 0, 'numbered', { 'data-foo': 'X' } ) ) ); } ); it( 'should restore `htmlLiAttributes` attribute after it\'s changed in one of the following items', () => { setModelData( model, - paragraph( '1.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + - paragraph( '2.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + - paragraph( '3.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + liParagraph( '1.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + liParagraph( '2.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + liParagraph( '3.', '01', 0, 'numbered', { 'data-foo': 'A' } ) ); model.change( writer => { @@ -451,17 +441,17 @@ describe( 'DocumentListElementSupport', () => { } ); expect( getModelData( model, { withoutSelection: true } ) ).to.equal( unquote( - paragraph( '1.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + - paragraph( '2.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + - paragraph( '3.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + liParagraph( '1.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + liParagraph( '2.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + liParagraph( '3.', '01', 0, 'numbered', { 'data-foo': 'A' } ) ) ); } ); it( 'should change `htmlLiAttributes` attribute for all the following items after the first one is changed', () => { setModelData( model, - paragraph( '1.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + - paragraph( '2.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + - paragraph( '3.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + liParagraph( '1.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + liParagraph( '2.', '01', 0, 'numbered', { 'data-foo': 'A' } ) + + liParagraph( '3.', '01', 0, 'numbered', { 'data-foo': 'A' } ) ); model.change( writer => { @@ -473,13 +463,13 @@ describe( 'DocumentListElementSupport', () => { } ); expect( getModelData( model, { withoutSelection: true } ) ).to.equal( unquote( - paragraph( '1.', '01', 0, 'numbered', { 'data-foo': 'B' } ) + - paragraph( '2.', '01', 0, 'numbered', { 'data-foo': 'B' } ) + - paragraph( '3.', '01', 0, 'numbered', { 'data-foo': 'B' } ) + liParagraph( '1.', '01', 0, 'numbered', { 'data-foo': 'B' } ) + + liParagraph( '2.', '01', 0, 'numbered', { 'data-foo': 'B' } ) + + liParagraph( '3.', '01', 0, 'numbered', { 'data-foo': 'B' } ) ) ); } ); - function paragraph( text, id, indent, type, liAttributes ) { + function liParagraph( text, id, indent, type, liAttributes ) { const attrs = JSON.stringify( { attributes: liAttributes } ).replaceAll( '"', '"' ); return ( @@ -489,9 +479,99 @@ describe( 'DocumentListElementSupport', () => { ); } } ); + } ); - function unquote( text ) { - return text.replaceAll( '"', '"' ); - } + describe( 'indenting lists', () => { + it( 'should reset `htmlListAttributes` attribute after indenting a single item', () => { + setModelData( model, + paragraph( '1.', '01', 0, 'numbered', { 'data-foo': 'foo' } ) + + paragraph( '1a.', '02', 1, 'bulleted', { 'data-foo': 'bar' } ) + + paragraph( '2.', '03', 0, 'numbered', { 'data-foo': 'foo' } ) + + paragraph( '3.[]', '04', 0, 'numbered', { 'data-foo': 'foo' } ) + + paragraph( '4.', '05', 0, 'numbered', { 'data-foo': 'foo' } ) + ); + + editor.execute( 'indentList' ); + + expect( getModelData( model ) ).to.equal( unquote( + paragraph( '1.', '01', 0, 'numbered', { 'data-foo': 'foo' } ) + + paragraph( '1a.', '02', 1, 'bulleted', { 'data-foo': 'bar' } ) + + paragraph( '2.', '03', 0, 'numbered', { 'data-foo': 'foo' } ) + + paragraph( '3.[]', '04', 1, 'numbered', undefined ) + + paragraph( '4.', '05', 0, 'numbered', { 'data-foo': 'foo' } ) + ) ); + } ); + + it( 'should reset `htmlListAttributes` attribute after indenting a few items', () => { + setModelData( model, + paragraph( '1.', '01', 0, 'bulleted', { 'data-foo': 'foo' } ) + + paragraph( '[2.', '02', 0, 'bulleted', { 'data-foo': 'foo' } ) + + paragraph( '3.]', '03', 0, 'bulleted', { 'data-foo': 'foo' } ) + ); + + editor.execute( 'indentList' ); + + expect( getModelData( model ) ).to.equal( unquote( + paragraph( '1.', '01', 0, 'bulleted', { 'data-foo': 'foo' } ) + + paragraph( '[2.', '02', 1, 'bulleted', undefined ) + + paragraph( '3.]', '03', 1, 'bulleted', undefined ) + ) ); + } ); + + it( 'should copy `htmlListAttributes` attribute after indenting a single item into previously nested list', () => { + setModelData( model, + paragraph( '1.', '01', 0, 'bulleted', { 'data-foo': 'foo' } ) + + paragraph( '1a.', '02', 1, 'bulleted', { 'data-foo': 'bar' } ) + + paragraph( '1b.', '03', 1, 'bulleted', { 'data-foo': 'bar' } ) + + paragraph( '2.[]', '04', 0, 'bulleted', { 'data-foo': 'foo' } ) + + paragraph( '3.', '05', 0, 'bulleted', { 'data-foo': 'foo' } ) + ); + + editor.execute( 'indentList' ); + + expect( getModelData( model ) ).to.equal( unquote( + paragraph( '1.', '01', 0, 'bulleted', { 'data-foo': 'foo' } ) + + paragraph( '1a.', '02', 1, 'bulleted', { 'data-foo': 'bar' } ) + + paragraph( '1b.', '03', 1, 'bulleted', { 'data-foo': 'bar' } ) + + paragraph( '2.[]', '04', 1, 'bulleted', { 'data-foo': 'bar' } ) + + paragraph( '3.', '05', 0, 'bulleted', { 'data-foo': 'foo' } ) + ) ); + } ); + + it( 'should copy `htmlListAttributes` attribute after indenting a few items into previously nested list', () => { + setModelData( model, + paragraph( '1.', '01', 0, 'bulleted', { 'data-foo': 'foo' } ) + + paragraph( '1a.', '02', 1, 'bulleted', { 'data-foo': 'bar' } ) + + paragraph( '1b.', '03', 1, 'bulleted', { 'data-foo': 'bar' } ) + + paragraph( '[2.', '04', 0, 'bulleted', { 'data-foo': 'foo' } ) + + paragraph( '3.]', '05', 0, 'bulleted', { 'data-foo': 'foo' } ) + + paragraph( '4.', '06', 0, 'bulleted', { 'data-foo': 'foo' } ) + ); + + editor.execute( 'indentList' ); + + expect( getModelData( model ) ).to.equal( unquote( + paragraph( '1.', '01', 0, 'bulleted', { 'data-foo': 'foo' } ) + + paragraph( '1a.', '02', 1, 'bulleted', { 'data-foo': 'bar' } ) + + paragraph( '1b.', '03', 1, 'bulleted', { 'data-foo': 'bar' } ) + + paragraph( '[2.', '04', 1, 'bulleted', { 'data-foo': 'bar' } ) + + paragraph( '3.]', '05', 1, 'bulleted', { 'data-foo': 'bar' } ) + + paragraph( '4.', '06', 0, 'bulleted', { 'data-foo': 'foo' } ) + ) ); + } ); } ); + + function paragraph( text, id, indent, type, listAttributes ) { + const attrs = JSON.stringify( { attributes: listAttributes } ).replaceAll( '"', '"' ); + + return ( + `` + + text + + '' + ); + } + + function unquote( text ) { + return text.replaceAll( '"', '"' ); + } } ); From ec7b4e6ce7c21e65ef92c1c5c3294324c0bab258 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski <1232187+niegowski@users.noreply.github.com> Date: Tue, 22 Mar 2022 11:52:52 +0100 Subject: [PATCH 24/25] Apply review comment. --- packages/ckeditor5-list/src/documentlist/documentlistediting.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ckeditor5-list/src/documentlist/documentlistediting.js b/packages/ckeditor5-list/src/documentlist/documentlistediting.js index 670235b1f92..e6173c47457 100644 --- a/packages/ckeditor5-list/src/documentlist/documentlistediting.js +++ b/packages/ckeditor5-list/src/documentlist/documentlistediting.js @@ -150,7 +150,7 @@ export default class DocumentListEditing extends Plugin { } /** - * Registers a downcast strategyl + * Registers a downcast strategy. * * **Note**: Strategies must be registered in the `Plugin#init()` phase so that it can be applied * in the `DocumentListEditing#afterInit()`. From c45f85d5b46dda10864e851793167c4eb7617686 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski <1232187+niegowski@users.noreply.github.com> Date: Tue, 22 Mar 2022 12:02:20 +0100 Subject: [PATCH 25/25] Apply review comment. --- .../ckeditor5-html-support/tests/integrations/documentlist.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/ckeditor5-html-support/tests/integrations/documentlist.js b/packages/ckeditor5-html-support/tests/integrations/documentlist.js index 05196bafb1a..9ba1cd04af3 100644 --- a/packages/ckeditor5-html-support/tests/integrations/documentlist.js +++ b/packages/ckeditor5-html-support/tests/integrations/documentlist.js @@ -3,9 +3,10 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ +import GeneralHtmlSupport from '../../src/generalhtmlsupport'; + import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor'; import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; -import GeneralHtmlSupport from '../../src/generalhtmlsupport'; import DocumentListEditing from '@ckeditor/ckeditor5-list/src/documentlist/documentlistediting'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';