Skip to content

Commit

Permalink
Merge pull request #7902 from ckeditor/i/7879
Browse files Browse the repository at this point in the history 
Fix (list): When removing the content between two lists items, the lists will be merged into a single list. The second list should adjust its value for the `listStyle` attribute based on the most outer `listItem` in the first list. Closes #7879.
  • Loading branch information
@jodator
jodator authored Aug 27, 2020
2 parents f9b38be + 4f1f558 commit 7aa9528
Show file tree
Hide file tree
Showing 5 changed files with 773 additions and 85 deletions.
82 changes: 1 addition & 81 deletions packages/ckeditor5-list/src/liststylecommand.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
*/

import Command from '@ckeditor/ckeditor5-core/src/command';
import TreeWalker from '@ckeditor/ckeditor5-engine/src/model/treewalker';
import { getSiblingNodes } from './utils';

/**
* The list style command. It is used by the {@link module:list/liststyle~ListStyle list styles feature}.
Expand Down Expand Up @@ -115,83 +115,3 @@ export default class ListStyleCommand extends Command {
return numberedList.isEnabled || bulletedList.isEnabled;
}
}

// Returns an array with all `listItem` elements that represents the same list.
//
// It means that values for `listIndent`, `listType`, and `listStyle` for all items
// are equal.
//
// @param {module:engine/model/position~Position} position Starting position.
// @param {'forward'|'backward'} direction Walking direction.
// @returns {Array.<module:engine/model/element~Element>
function getSiblingNodes( position, direction ) {
const items = [];
const listItem = position.parent;
const walkerOptions = {
ignoreElementEnd: true,
startPosition: position,
shallow: true,
direction
};
const limitIndent = listItem.getAttribute( 'listIndent' );
const nodes = [ ...new TreeWalker( walkerOptions ) ]
.filter( value => value.item.is( 'element' ) )
.map( value => value.item );

for ( const element of nodes ) {
// If found something else than `listItem`, we're out of the list scope.
if ( !element.is( 'element', 'listItem' ) ) {
break;
}

// If current parsed item has lower indent that element that the element that was a starting point,
// it means we left a nested list. Abort searching items.
//
// ■ List item 1. [listIndent=0]
// ○ List item 2.[] [listIndent=1], limitIndent = 1,
// ○ List item 3. [listIndent=1]
// ■ List item 4. [listIndent=0]
//
// Abort searching when leave nested list.
if ( element.getAttribute( 'listIndent' ) < limitIndent ) {
break;
}

// ■ List item 1.[] [listIndent=0] limitIndent = 0,
// ○ List item 2. [listIndent=1]
// ○ List item 3. [listIndent=1]
// ■ List item 4. [listIndent=0]
//
// Ignore nested lists.
if ( element.getAttribute( 'listIndent' ) > limitIndent ) {
continue;
}

// ■ List item 1.[] [listType=bulleted]
// 1. List item 2. [listType=numbered]
// 2.List item 3. [listType=numbered]
//
// Abort searching when found a different kind of a list.
if ( element.getAttribute( 'listType' ) !== listItem.getAttribute( 'listType' ) ) {
break;
}

// ■ List item 1.[] [listType=bulleted]
// ■ List item 2. [listType=bulleted]
// ○ List item 3. [listType=bulleted]
// ○ List item 4. [listType=bulleted]
//
// Abort searching when found a different list style.
if ( element.getAttribute( 'listStyle' ) !== listItem.getAttribute( 'listStyle' ) ) {
break;
}

if ( direction === 'backward' ) {
items.unshift( element );
} else {
items.push( element );
}
}

return items;
}
106 changes: 105 additions & 1 deletion packages/ckeditor5-list/src/liststyleediting.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
import ListEditing from './listediting';
import ListStyleCommand from './liststylecommand';
import { getSiblingListItem } from './utils';
import { getSiblingListItem, getSiblingNodes } from './utils';

const DEFAULT_LIST_TYPE = 'default';

Expand Down Expand Up @@ -66,6 +66,9 @@ export default class ListStyleEditing extends Plugin {
// Set up conversion.
editor.conversion.for( 'upcast' ).add( upcastListItemStyle() );
editor.conversion.for( 'downcast' ).add( downcastListStyleAttribute() );

// Handle merging two separated lists into the single one.
this._mergeListStyleAttributeWhileMergingLists();
}

/**
Expand All @@ -80,6 +83,107 @@ export default class ListStyleEditing extends Plugin {
editor.model.document.registerPostFixer( removeListStyleAttributeFromTodoList( editor ) );
}
}

/**
* Starts listening to {@link module:engine/model/model~Model#deleteContent} checks whether two lists will be merged into a single one
* after deleting the content.
*
* The purpose of this action is to adjust the `listStyle` value for the list that was merged.
*
* Consider the following model's content:
*
* <listItem listIndent="0" listType="bulleted" listStyle="square">UL List item 1</listItem>
* <listItem listIndent="0" listType="bulleted" listStyle="square">UL List item 2</listItem>
* <paragraph>[A paragraph.]</paragraph>
* <listItem listIndent="0" listType="bulleted" listStyle="circle">UL List item 1</listItem>
* <listItem listIndent="0" listType="bulleted" listStyle="circle">UL List item 2</listItem>
*
* After removing the paragraph element, the second list will be merged into the first one.
* We want to inherit the `listStyle` attribute for the second list from the first one.
*
* <listItem listIndent="0" listType="bulleted" listStyle="square">UL List item 1</listItem>
* <listItem listIndent="0" listType="bulleted" listStyle="square">UL List item 2</listItem>
* <listItem listIndent="0" listType="bulleted" listStyle="square">UL List item 1</listItem>
* <listItem listIndent="0" listType="bulleted" listStyle="square">UL List item 2</listItem>
*
* See https://github.com/ckeditor/ckeditor5/issues/7879.
*
* @private
*/
_mergeListStyleAttributeWhileMergingLists() {
const editor = this.editor;
const model = editor.model;

// First the most-outer `listItem` in the first list reference.
// If found, lists should be merged and this `listItem` provides the `listStyle` attribute
// and it' also a starting point when searching for items in the second list.
let firstMostOuterItem;

// Check whether the removed content is between two lists.
this.listenTo( model, 'deleteContent', ( evt, [ selection ] ) => {
const firstPosition = selection.getFirstPosition();
const lastPosition = selection.getLastPosition();

// Typing or removing content in a single item. Aborting.
if ( firstPosition.parent === lastPosition.parent ) {
return;
}

// An element before the content that will be removed is not a list.
if ( !firstPosition.parent.is( 'element', 'listItem' ) ) {
return;
}

const nextSibling = lastPosition.parent.nextSibling;

// An element after the content that will be removed is not a list.
if ( !nextSibling || !nextSibling.is( 'element', 'listItem' ) ) {
return;
}

const mostOuterItemList = getSiblingListItem( firstPosition.parent, {
sameIndent: true,
listIndent: 0
} );

if ( mostOuterItemList.getAttribute( 'listType' ) === nextSibling.getAttribute( 'listType' ) ) {
firstMostOuterItem = mostOuterItemList;
}
}, { priority: 'high' } );

// If so, update the `listStyle` attribute for the second list.
this.listenTo( model, 'deleteContent', () => {
if ( !firstMostOuterItem ) {
return;
}

model.change( writer => {
// Find the first most-outer item list in the merged list.
// A case when the first list item in the second list was merged into the last item in the first list.
//
// <listItem listIndent="0" listType="bulleted" listStyle="square">UL List item 1</listItem>
// <listItem listIndent="0" listType="bulleted" listStyle="square">UL List item 2</listItem>
// <listItem listIndent="0" listType="bulleted" listStyle="circle">[]UL List item 1</listItem>
// <listItem listIndent="0" listType="bulleted" listStyle="circle">UL List item 2</listItem>
const secondListMostOuterItem = getSiblingListItem( firstMostOuterItem.nextSibling, {
sameIndent: true,
listIndent: 0,
direction: 'forward'
} );

const items = [
secondListMostOuterItem,
...getSiblingNodes( writer.createPositionAt( secondListMostOuterItem, 0 ), 'forward' )
];

for ( const listItem of items ) {
writer.setAttribute( 'listStyle', firstMostOuterItem.getAttribute( 'listStyle' ), listItem );
}
} );

firstMostOuterItem = null;
}, { priority: 'low' } );
}
}

// Returns a converter that consumes the `style` attribute and search for `list-style-type` definition.
Expand Down
89 changes: 88 additions & 1 deletion packages/ckeditor5-list/src/utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

import { getFillerOffset } from '@ckeditor/ckeditor5-engine/src/view/containerelement';
import ButtonView from '@ckeditor/ckeditor5-ui/src/button/buttonview';
import TreeWalker from '@ckeditor/ckeditor5-engine/src/model/treewalker';

/**
* Creates a list item {@link module:engine/view/containerelement~ContainerElement}.
Expand Down Expand Up @@ -207,6 +208,7 @@ export function positionAfterUiElements( viewPosition ) {
* @param {Boolean} [options.sameIndent=false] Whether the sought sibling should have the same indentation.
* @param {Boolean} [options.smallerIndent=false] Whether the sought sibling should have a smaller indentation.
* @param {Number} [options.listIndent] The reference indentation.
* @param {'forward'|'backward'} [options.direction='backward'] Walking direction.
* @returns {module:engine/model/item~Item|null}
*/
export function getSiblingListItem( modelItem, options ) {
Expand All @@ -223,7 +225,11 @@ export function getSiblingListItem( modelItem, options ) {
return item;
}

item = item.previousSibling;
if ( options.direction === 'forward' ) {
item = item.nextSibling;
} else {
item = item.previousSibling;
}
}

return null;
Expand Down Expand Up @@ -279,6 +285,87 @@ export function findNestedList( viewElement ) {
return null;
}

/**
* Returns an array with all `listItem` elements that represents the same list.
*
* It means that values for `listIndent`, `listType`, and `listStyle` for all items are equal.
*
* @param {module:engine/model/position~Position} position Starting position.
* @param {'forward'|'backward'} direction Walking direction.
* @returns {Array.<module:engine/model/element~Element>}
*/
export function getSiblingNodes( position, direction ) {
const items = [];
const listItem = position.parent;
const walkerOptions = {
ignoreElementEnd: true,
startPosition: position,
shallow: true,
direction
};
const limitIndent = listItem.getAttribute( 'listIndent' );
const nodes = [ ...new TreeWalker( walkerOptions ) ]
.filter( value => value.item.is( 'element' ) )
.map( value => value.item );

for ( const element of nodes ) {
// If found something else than `listItem`, we're out of the list scope.
if ( !element.is( 'element', 'listItem' ) ) {
break;
}

// If current parsed item has lower indent that element that the element that was a starting point,
// it means we left a nested list. Abort searching items.
//
// ■ List item 1. [listIndent=0]
// ○ List item 2.[] [listIndent=1], limitIndent = 1,
// ○ List item 3. [listIndent=1]
// ■ List item 4. [listIndent=0]
//
// Abort searching when leave nested list.
if ( element.getAttribute( 'listIndent' ) < limitIndent ) {
break;
}

// ■ List item 1.[] [listIndent=0] limitIndent = 0,
// ○ List item 2. [listIndent=1]
// ○ List item 3. [listIndent=1]
// ■ List item 4. [listIndent=0]
//
// Ignore nested lists.
if ( element.getAttribute( 'listIndent' ) > limitIndent ) {
continue;
}

// ■ List item 1.[] [listType=bulleted]
// 1. List item 2. [listType=numbered]
// 2.List item 3. [listType=numbered]
//
// Abort searching when found a different kind of a list.
if ( element.getAttribute( 'listType' ) !== listItem.getAttribute( 'listType' ) ) {
break;
}

// ■ List item 1.[] [listType=bulleted]
// ■ List item 2. [listType=bulleted]
// ○ List item 3. [listType=bulleted]
// ○ List item 4. [listType=bulleted]
//
// Abort searching when found a different list style.
if ( element.getAttribute( 'listStyle' ) !== listItem.getAttribute( 'listStyle' ) ) {
break;
}

if ( direction === 'backward' ) {
items.unshift( element );
} else {
items.push( element );
}
}

return items;
}

// Implementation of getFillerOffset for view list item element.
//
// @returns {Number|null} Block filler offset or `null` if block filler is not needed.
Expand Down
Loading

0 comments on commit 7aa9528

Please sign in to comment.