Skip to content

Commit

Permalink
Additional improvements.
Browse files Browse the repository at this point in the history
  • Loading branch information
@pomek
pomek committed Aug 15, 2020
1 parent e16cb29 commit 7adc393
Show file tree
Hide file tree
Showing 6 changed files with 132 additions and 47 deletions.
1 change: 1 addition & 0 deletions packages/ckeditor5-list/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
"@ckeditor/ckeditor5-highlight": "^21.0.0",
"@ckeditor/ckeditor5-indent": "^21.0.0",
"@ckeditor/ckeditor5-link": "^21.0.0",
"@ckeditor/ckeditor5-remove-format": "^21.0.0",
"@ckeditor/ckeditor5-table": "^21.0.0",
"@ckeditor/ckeditor5-typing": "^21.0.0",
"@ckeditor/ckeditor5-undo": "^21.0.0"
Expand Down
12 changes: 10 additions & 2 deletions packages/ckeditor5-list/src/listcommand.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export default class ListCommand extends Command {
const turnOff = this.value === true;
// If we are turning off items, we are going to rename them to paragraphs.

return model.change( writer => {
model.change( writer => {
// If part of a list got turned off, we need to handle (outdent) all of sub-items of the last turned-off item.
// To be sure that model is all the time in a good state, we first fix items below turned-off item.
if ( turnOff ) {
Expand Down Expand Up @@ -212,7 +212,15 @@ export default class ListCommand extends Command {
}
}

return blocks;
/**
* Event fired by the {@link #execute} method.
*
* It allows to execute an action after executing the {@link ~ListCommand#execute} method, e.g. adjusting
* attributes of changed blocks.
*
* @event executeCleanup
*/
this.fire( 'executeCleanup', blocks );
} );
}

Expand Down
125 changes: 81 additions & 44 deletions packages/ckeditor5-list/src/liststylesediting.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,20 @@ export default class ListStylesEditing extends Plugin {
allowAttributes: [ 'listStyle' ]
} );

// TODO: Is it correct?
editor.model.schema.setAttributeProperties( 'listStyle', {
isFormatting: true
} );

editor.commands.add( 'listStyles', new ListStylesCommand( editor, DEFAULT_LIST_TYPE ) );

// Fix list attributes when modifying their nesting levels (the `listIndent` attribute).
this.listenTo( editor.commands.get( 'indentList' ), 'executeCleanup', fixListAfterIndentListCommand( editor ) );
this.listenTo( editor.commands.get( 'outdentList' ), 'executeCleanup', fixListAfterOutdentListCommand( editor ) );

this.listenTo( editor.commands.get( 'bulletedList' ), 'executeCleanup', restoreDefaultListStyle( editor ) );
this.listenTo( editor.commands.get( 'numberedList' ), 'executeCleanup', restoreDefaultListStyle( editor ) );

// Register a post-fixer that ensures that the `listStyle` attribute is specified in each `listItem` element.
model.document.registerPostFixer( fixListStyleAttributeOnListItemElements( editor ) );

Expand Down Expand Up @@ -101,66 +109,77 @@ function upcastListItem() {
// @returns {Function}
function downcastListStyleAttribute() {
return dispatcher => {
// TODO: The idea seems to be OK. Unfortunately, it does not work.
// dispatcher.on( 'insert:listItem', ( evt, data, conversionApi ) => {
// const modelItem = data.item;
//
// // Do not update anything for "todo" `listItem`.
// if ( modelItem.getAttribute( 'listType' ) === 'todo' ) {
// return;
// }
//
// // Also, no changes are required if the style is already defined.
// if ( modelItem.hasAttribute( 'listStyle' ) ) {
// return;
// }
//
// const nextElement = getSiblingListItem( modelItem.nextSibling, {
// sameIndent: true,
// listIndent: modelItem.getAttribute( 'listIndent' ),
// direction: 'forward'
// } );
//
// const previousElement = getSiblingListItem( modelItem.previousSibling, {
// sameIndent: true,
// listIndent: modelItem.getAttribute( 'listIndent' ),
// direction: 'backward'
// } );
//
// if ( nextElement ) {
// // Insert at the beginning of the list.
// conversionApi.writer.setAttribute( 'listStyle', nextElement.getAttribute( 'listStyle' ), modelItem );
// } else if ( previousElement ) {
// // Insert at the end of the list.
// conversionApi.writer.setAttribute( 'listStyle', previousElement.getAttribute( 'listStyle' ), modelItem );
// }
//
// // We don't need to check whether `previousElement` or `nextElement` and `modelItem` belong to the same list.
// }, { priority: 'low' } );

dispatcher.on( 'attribute:listStyle:listItem', ( evt, data, conversionApi ) => {
// TODO: This converter is a little buggy. I know. The things could be done better.
const viewWriter = conversionApi.writer;
const currentItem = data.item;
const previousItem = currentItem.previousSibling;
const viewItem = conversionApi.mapper.toViewElement( currentItem );
const currentElement = data.item;
const listStyle = data.attributeNewValue;

// Parsing the first element in a list. Just set the attribute.
if ( !previousItem || !previousItem.is( 'element', 'listItem' ) ) {
return setListStyle( viewWriter, listStyle, viewItem.parent );
}
const previousElement = getSiblingListItem( currentElement.previousSibling, {
sameIndent: true,
listIndent: currentElement.getAttribute( 'listIndent' ),
direction: 'backward'
} );

// But if previous element is the list item, we must be sure that those two items belong to the same list.
// So, we should check whether the values of the `listType`, `listIndent` and `listStyle` attributes are equal.
//
// If the current parsed list item does not belong to the same list that the previous element,
// the `listStyle` attribute must be set once again since another list is being processed.
//
// Note: We ignore the check of the `listStyle` attribute since that case must be handled another way.
// If two items have the same values for `listType` and `listIndent` but not for `listStyle`,
// we must split the list container (`<ol>` or `<ul>`) since we're processing two different lists.
if ( !areRepresentingSameList( previousItem, currentItem ) ) {
return setListStyle( viewWriter, listStyle, viewItem.parent );
}
const viewItem = conversionApi.mapper.toViewElement( currentElement );

const previousListStyle = previousItem.getAttribute( 'listStyle' );
// Single item list.
if ( !previousElement ) {
setListStyle( viewWriter, listStyle, viewItem.parent );
} else if ( !areRepresentingSameList( previousElement, currentElement ) ) {
viewWriter.breakContainer( viewWriter.createPositionBefore( viewItem ) );
viewWriter.breakContainer( viewWriter.createPositionAfter( viewItem ) );

// Since we were ignoring the `listStyle` check, it must be checked before splitting the list container.
// No change is needed if previous element has the same value of the `listStyle` attribute.
if ( previousListStyle === listStyle ) {
return;
setListStyle( viewWriter, listStyle, viewItem.parent );
}

// But if those attributes are different, we must split the parent element
// and set the attribute for the new created container.
viewWriter.breakContainer( viewWriter.createPositionBefore( viewItem ) );
viewWriter.breakContainer( viewWriter.createPositionAfter( viewItem ) );

setListStyle( viewWriter, listStyle, viewItem.parent );
}, { priority: 'low' } );
};

// Checks whether specified list items belong to the same list.
//
// Comparing the `listStyle` attribute is by design since it requires additional actions.
//
// @param {module:engine/model/element~Element} listItem1 The first list item to check.
// @param {module:engine/model/element~Element} listItem2 The second list item to check.
// @returns {Boolean}
function areRepresentingSameList( listItem1, listItem2 ) {
if ( listItem1.getAttribute( 'listType' ) !== listItem2.getAttribute( 'listType' ) ) {
return false;
}

if ( listItem1.getAttribute( 'listIndent' ) !== listItem2.getAttribute( 'listIndent' ) ) {
return false;
}

return true;
return listItem1.getAttribute( 'listType' ) === listItem2.getAttribute( 'listType' ) &&
listItem1.getAttribute( 'listIndent' ) === listItem2.getAttribute( 'listIndent' ) &&
listItem1.getAttribute( 'listStyle' ) === listItem2.getAttribute( 'listStyle' );
}

// Updates or removes the `list-style-type` from the `element`.
Expand Down Expand Up @@ -367,7 +386,7 @@ function fixListStyleAttributeOnListItemElements( editor ) {
// ■ List item 1.
let existingListItem = insertedListItems[ insertedListItems.length - 1 ].nextSibling;

// If doesn't, maybe the `listItem` was inserted at the end of the list.
// If it doesn't, maybe the `listItem` was inserted at the end of the list.
//
// ■ List item 1.
// ■ Paragraph[] // <-- The inserted item.
Expand Down Expand Up @@ -472,3 +491,21 @@ function removeListStyleAttributeFromTodoList( editor ) {
return null;
}
}

// Restores the `listStyle` attribute after changing the list type.
//
// @private
// @param {module:core/editor/editor~Editor} editor
// @returns {Function}
function restoreDefaultListStyle( editor ) {
return ( evt, changedItems ) => {
changedItems = changedItems.filter( item => item.is( 'element', 'listItem' ) );

editor.model.change( writer => {
for ( const item of changedItems ) {
// Remove the attribute. Post-fixer will restore the proper value.
writer.removeAttribute( 'listStyle', item );
}
} );
};
}
29 changes: 29 additions & 0 deletions packages/ckeditor5-list/tests/listcommand.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,35 @@ describe( 'ListCommand', () => {
expect( getData( model ) ).to.equal( expectedData );
} );
} );

it( 'should fire "executeCleanup" event after finish all operations with all changed items', done => {
setData( model,
'<paragraph>Foo 1.</paragraph>' +
'<paragraph>[Foo 2.</paragraph>' +
'<paragraph>Foo 3.]</paragraph>' +
'<paragraph>Foo 4.</paragraph>'
);

command.execute();

expect( getData( model ) ).to.equal(
'<paragraph>Foo 1.</paragraph>' +
'<listItem listIndent="0" listType="bulleted">[Foo 2.</listItem>' +
'<listItem listIndent="0" listType="bulleted">Foo 3.]</listItem>' +
'<paragraph>Foo 4.</paragraph>'
);

command.on( 'executeCleanup', ( evt, data ) => {
expect( data ).to.deep.equal( [
root.getChild( 2 ),
root.getChild( 1 )
] );

done();
} );

command.execute();
} );
} );
} );
} );
6 changes: 6 additions & 0 deletions packages/ckeditor5-list/tests/liststylesediting.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,12 @@ describe( 'ListStylesEditing', () => {
it( 'should allow set `listStyle` on the `listItem`', () => {
expect( model.schema.checkAttribute( [ '$root', 'listItem' ], 'listStyle' ) ).to.be.true;
} );

it( 'should be marked with a formatting property', () => {
expect( model.schema.getAttributeProperties( 'listStyle' ) ).to.include( {
isFormatting: true
} );
} );
} );

describe( 'command', () => {
Expand Down
6 changes: 5 additions & 1 deletion packages/ckeditor5-list/tests/manual/list-styles.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import ListStyles from '../../src/liststyles';
import Indent from '@ckeditor/ckeditor5-indent/src/indent';
import IndentBlock from '@ckeditor/ckeditor5-indent/src/indentblock';
import TodoList from '../../src/todolist';
import RemoveFormat from '@ckeditor/ckeditor5-remove-format/src/removeformat';

ClassicEditor
.create( document.querySelector( '#editor' ), {
Expand All @@ -33,11 +34,14 @@ ClassicEditor
TablePropertiesEditing,
TableCellPropertiesEditing,
Indent,
IndentBlock
IndentBlock,
RemoveFormat
],
toolbar: [
'heading',
'|',
'removeFormat',
'|',
'bulletedList', 'numberedList', 'todoList',
'|',
'outdent',
Expand Down

0 comments on commit 7adc393

Please sign in to comment.