Bladeren bron

Changed: command should listen only to model.Document#event:changesDone. Fixes #49.
Changed: improved list item type fixing when indenting and outdenting items.
Changed: fixed indenting of first list item of a list. Fixes #52.

Szymon Cofalik 8 jaren geleden
bovenliggende
commit
27b1a83b61

+ 26 - 30
packages/ckeditor5-list/src/indentcommand.js

@@ -35,11 +35,6 @@ export default class IndentCommand extends Command {
 		 */
 		this._indentBy = indentDirection == 'forward' ? 1 : -1;
 
-		// Refresh command state after selection is changed or changes has been done to the document.
-		this.listenTo( editor.document.selection, 'change:range', () => {
-			this.refreshState();
-		} );
-
 		this.listenTo( editor.document, 'changesDone', () => {
 			this.refreshState();
 		} );
@@ -87,19 +82,17 @@ export default class IndentCommand extends Command {
 				}
 				// If indent is >= 0, change the attribute value.
 				else {
-					// If indent is > 0 check whether list item's type should not be fixed.
-					if ( indent > 0 ) {
-						// First, find previous sibling with same indent.
-						let prev = item.previousSibling;
-
-						while ( prev.getAttribute( 'indent' ) > indent ) {
-							prev = prev.previousSibling;
-						}
-
-						// Then check if that sibling has same type. If not, change type of this item.
-						if ( prev.getAttribute( 'type' ) != item.getAttribute( 'type' ) ) {
-							batch.setAttribute( item, 'type', prev.getAttribute( 'type' ) );
-						}
+					// Check whether list item's type should not be fixed.
+					// First, find previous sibling with same indent - if it exists.
+					let prev = item.previousSibling;
+
+					while ( prev && prev.is( 'listItem' ) && prev.getAttribute( 'indent' ) > indent ) {
+						prev = prev.previousSibling;
+					}
+
+					// Then check if that sibling has same type. If not, change type of this item.
+					if ( prev && prev.getAttribute( 'type' ) != item.getAttribute( 'type' ) ) {
+						batch.setAttribute( item, 'type', prev.getAttribute( 'type' ) );
 					}
 
 					batch.setAttribute( item, 'indent', indent );
@@ -120,21 +113,24 @@ export default class IndentCommand extends Command {
 			return false;
 		}
 
-		const prev = listItem.previousSibling;
-		const oldIndent = listItem.getAttribute( 'indent' );
-		const newIndent = oldIndent + this._indentBy;
-
 		if ( this._indentBy > 0 ) {
-			// If we are indenting, there are some conditions to meet.
-			// Cannot indent first list item.
-			if ( !prev || prev.name != 'listItem' ) {
-				return false;
-			}
+			// Cannot indent first item in it's list. Check if before `listItem` is a list item that is in same list.
+			// To be in the same list, the item has to have same attributes.
+			const indent = listItem.getAttribute( 'indent' );
+			const type = listItem.getAttribute( 'type' );
+
+			let prev = listItem.previousSibling;
+
+			while ( prev && prev.is( 'listItem' ) ) {
+				if ( prev.getAttribute( 'indent' ) == indent && prev.getAttribute( 'type' ) == type ) {
+					return true;
+				}
 
-			// Indent can be at most greater by one than indent of previous item.
-			if ( prev.getAttribute( 'indent' ) + 1 < newIndent ) {
-				return false;
+				prev = prev.previousSibling;
 			}
+
+			// Could not find similar list item, this means that `listItem` is first in its list.
+			return false;
 		}
 
 		// If we are outdenting it is enough to be in list item. Every list item can always be outdented.

+ 5 - 8
packages/ckeditor5-list/src/listcommand.js

@@ -42,14 +42,11 @@ export default class ListCommand extends Command {
 		 */
 		this.set( 'value', false );
 
-		const changeCallback = () => {
+		// Listen on selection and document changes and set the current command's value.
+		this.listenTo( editor.document, 'changesDone', () => {
 			this.refreshValue();
 			this.refreshState();
-		};
-
-		// Listen on selection and document changes and set the current command's value.
-		this.listenTo( editor.document.selection, 'change:range', changeCallback );
-		this.listenTo( editor.document, 'changesDone', changeCallback );
+		} );
 	}
 
 	/**
@@ -59,7 +56,7 @@ export default class ListCommand extends Command {
 		// Check whether closest `listItem` ancestor of the position has a correct type.
 		const listItem = first( this.editor.document.selection.getSelectedBlocks() );
 
-		this.value = listItem !== null && listItem.getAttribute( 'type' ) == this.type;
+		this.value = listItem && listItem.is( 'listItem' ) && listItem.getAttribute( 'type' ) == this.type;
 	}
 
 	/**
@@ -232,7 +229,7 @@ export default class ListCommand extends Command {
 	 * @inheritDoc
 	 */
 	_checkEnabled() {
-		// If command is enabled it means that we are in list item, so the command should be enabled.
+		// If command value is true it means that we are in list item, so the command should be enabled.
 		if ( this.value ) {
 			return true;
 		}

+ 48 - 48
packages/ckeditor5-list/tests/indentcommand.js

@@ -52,19 +52,25 @@ describe( 'IndentCommand', () => {
 
 		describe( 'isEnabled', () => {
 			it( 'should be true if selection starts in list item', () => {
-				doc.selection.collapse( root.getChild( 5 ) );
+				doc.enqueueChanges( () => {
+					doc.selection.collapse( root.getChild( 5 ) );
+				} );
 
 				expect( command.isEnabled ).to.be.true;
 			} );
 
 			it( 'should be false if selection starts in first list item', () => {
-				doc.selection.collapse( root.getChild( 0 ) );
+				doc.enqueueChanges( () => {
+					doc.selection.collapse( root.getChild( 0 ) );
+				} );
 
 				expect( command.isEnabled ).to.be.false;
 			} );
 
 			it( 'should be false if selection starts in a list item that has bigger indent than it\'s previous sibling', () => {
-				doc.selection.collapse( root.getChild( 2 ) );
+				doc.enqueueChanges( () => {
+					doc.selection.collapse( root.getChild( 2 ) );
+				} );
 
 				expect( command.isEnabled ).to.be.false;
 			} );
@@ -73,6 +79,7 @@ describe( 'IndentCommand', () => {
 			// and before we fixed it the command was enabled in such a case.
 			it( 'should be false if selection starts in a paragraph with indent attribute', () => {
 				doc.schema.allow( { name: 'paragraph', attributes: [ 'indent' ], inside: '$root' } );
+
 				setData( doc, '<listItem indent="0">a</listItem><paragraph indent="0">b[]</paragraph>' );
 
 				expect( command.isEnabled ).to.be.false;
@@ -81,7 +88,9 @@ describe( 'IndentCommand', () => {
 
 		describe( '_doExecute', () => {
 			it( 'should increment indent attribute by 1', () => {
-				doc.selection.collapse( root.getChild( 5 ) );
+				doc.enqueueChanges( () => {
+					doc.selection.collapse( root.getChild( 5 ) );
+				} );
 
 				command._doExecute();
 
@@ -97,7 +106,9 @@ describe( 'IndentCommand', () => {
 			} );
 
 			it( 'should increment indent of all sub-items of indented item', () => {
-				doc.selection.collapse( root.getChild( 1 ) );
+				doc.enqueueChanges( () => {
+					doc.selection.collapse( root.getChild( 1 ) );
+				} );
 
 				command._doExecute();
 
@@ -113,10 +124,12 @@ describe( 'IndentCommand', () => {
 			} );
 
 			it( 'should increment indent of all selected item when multiple items are selected', () => {
-				doc.selection.setRanges( [ new Range(
-					new Position( root.getChild( 1 ), [ 0 ] ),
-					new Position( root.getChild( 3 ), [ 0 ] )
-				) ] );
+				doc.enqueueChanges( () => {
+					doc.selection.setRanges( [ new Range(
+						new Position( root.getChild( 1 ), [ 0 ] ),
+						new Position( root.getChild( 3 ), [ 0 ] )
+					) ] );
+				} );
 
 				command._doExecute();
 
@@ -137,13 +150,9 @@ describe( 'IndentCommand', () => {
 					'<listItem indent="0" type="bulleted">a</listItem>' +
 					'<listItem indent="1" type="bulleted">b</listItem>' +
 					'<listItem indent="2" type="numbered">c</listItem>' +
-					'<listItem indent="1" type="bulleted">d</listItem>'
+					'<listItem indent="1" type="bulleted">[]d</listItem>'
 				);
 
-				doc.selection.setRanges( [ new Range(
-					new Position( root.getChild( 3 ), [ 0 ] )
-				) ] );
-
 				command._doExecute();
 
 				expect( getData( doc, { withoutSelection: true } ) ).to.equal(
@@ -169,21 +178,27 @@ describe( 'IndentCommand', () => {
 
 		describe( 'isEnabled', () => {
 			it( 'should be true if selection starts in list item', () => {
-				doc.selection.collapse( root.getChild( 5 ) );
+				doc.enqueueChanges( () => {
+					doc.selection.collapse( root.getChild( 5 ) );
+				} );
 
 				expect( command.isEnabled ).to.be.true;
 			} );
 
 			it( 'should be true if selection starts in first list item', () => {
 				// This is in contrary to forward indent command.
-				doc.selection.collapse( root.getChild( 0 ) );
+				doc.enqueueChanges( () => {
+					doc.selection.collapse( root.getChild( 0 ) );
+				} );
 
 				expect( command.isEnabled ).to.be.true;
 			} );
 
 			it( 'should be true if selection starts in a list item that has bigger indent than it\'s previous sibling', () => {
 				// This is in contrary to forward indent command.
-				doc.selection.collapse( root.getChild( 2 ) );
+				doc.enqueueChanges( () => {
+					doc.selection.collapse( root.getChild( 2 ) );
+				} );
 
 				expect( command.isEnabled ).to.be.true;
 			} );
@@ -191,7 +206,9 @@ describe( 'IndentCommand', () => {
 
 		describe( '_doExecute', () => {
 			it( 'should decrement indent attribute by 1 (if it is bigger than 0)', () => {
-				doc.selection.collapse( root.getChild( 5 ) );
+				doc.enqueueChanges( () => {
+					doc.selection.collapse( root.getChild( 5 ) );
+				} );
 
 				command._doExecute();
 
@@ -207,7 +224,9 @@ describe( 'IndentCommand', () => {
 			} );
 
 			it( 'should rename listItem to paragraph (if indent is equal to 0)', () => {
-				doc.selection.collapse( root.getChild( 0 ) );
+				doc.enqueueChanges( () => {
+					doc.selection.collapse( root.getChild( 0 ) );
+				} );
 
 				command._doExecute();
 
@@ -223,7 +242,9 @@ describe( 'IndentCommand', () => {
 			} );
 
 			it( 'should decrement indent of all sub-items of outdented item', () => {
-				doc.selection.collapse( root.getChild( 1 ) );
+				doc.enqueueChanges( () => {
+					doc.selection.collapse( root.getChild( 1 ) );
+				} );
 
 				command._doExecute();
 
@@ -239,10 +260,12 @@ describe( 'IndentCommand', () => {
 			} );
 
 			it( 'should outdent all selected item when multiple items are selected', () => {
-				doc.selection.setRanges( [ new Range(
-					new Position( root.getChild( 1 ), [ 0 ] ),
-					new Position( root.getChild( 3 ), [ 0 ] )
-				) ] );
+				doc.enqueueChanges( () => {
+					doc.selection.setRanges( [ new Range(
+						new Position( root.getChild( 1 ), [ 0 ] ),
+						new Position( root.getChild( 3 ), [ 0 ] )
+					) ] );
+				} );
 
 				command._doExecute();
 
@@ -262,13 +285,9 @@ describe( 'IndentCommand', () => {
 					doc,
 					'<listItem indent="0" type="bulleted">a</listItem>' +
 					'<listItem indent="1" type="bulleted">b</listItem>' +
-					'<listItem indent="2" type="numbered">c</listItem>'
+					'<listItem indent="2" type="numbered">[]c</listItem>'
 				);
 
-				doc.selection.setRanges( [ new Range(
-					new Position( root.getChild( 2 ), [ 0 ] )
-				) ] );
-
 				command._doExecute();
 
 				expect( getData( doc, { withoutSelection: true } ) ).to.equal(
@@ -277,25 +296,6 @@ describe( 'IndentCommand', () => {
 					'<listItem indent="1" type="bulleted">c</listItem>'
 				);
 			} );
-
-			it( 'should not fix list type if item is outdented to top level', () => {
-				setData(
-					doc,
-					'<listItem indent="0" type="bulleted">a</listItem>' +
-					'<listItem indent="1" type="numbered">b</listItem>'
-				);
-
-				doc.selection.setRanges( [ new Range(
-					new Position( root.getChild( 1 ), [ 0 ] )
-				) ] );
-
-				command._doExecute();
-
-				expect( getData( doc, { withoutSelection: true } ) ).to.equal(
-					'<listItem indent="0" type="bulleted">a</listItem>' +
-					'<listItem indent="0" type="numbered">b</listItem>'
-				);
-			} );
 		} );
 	} );
 } );

+ 26 - 27
packages/ckeditor5-list/tests/list.js

@@ -5,15 +5,17 @@
 
 /* globals document */
 
-import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor';
 import List from '../src/list';
-import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
 import ListEngine from '../src/listengine';
+
+import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor';
+import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
 import ButtonView from '@ckeditor/ckeditor5-ui/src/button/buttonview';
 import { getCode } from '@ckeditor/ckeditor5-utils/src/keyboard';
+import { setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
 
 describe( 'List', () => {
-	let editor, bulletedListButton, numberedListButton, schema;
+	let editor, doc, bulletedListButton, numberedListButton, schema;
 
 	beforeEach( () => {
 		const editorElement = document.createElement( 'div' );
@@ -25,6 +27,7 @@ describe( 'List', () => {
 			.then( newEditor => {
 				editor = newEditor;
 				schema = editor.document.schema;
+				doc = editor.document;
 
 				bulletedListButton = editor.ui.componentFactory.create( 'bulletedList' );
 				numberedListButton = editor.ui.componentFactory.create( 'numberedList' );
@@ -59,9 +62,7 @@ describe( 'List', () => {
 	} );
 
 	it( 'should bind bulleted list button model to bulledList command', () => {
-		editor.setData( '<ul><li>foo</li></ul>' );
-		// Collapsing selection in model, which has just flat listItems.
-		editor.document.selection.collapse( editor.document.getRoot().getChild( 0 ) );
+		setData( doc, '<listItem type="bulleted" indent="0">[]foo</listItem>' );
 
 		const command = editor.commands.get( 'bulletedList' );
 
@@ -76,9 +77,7 @@ describe( 'List', () => {
 	} );
 
 	it( 'should bind numbered list button model to numberedList command', () => {
-		editor.setData( '<ul><li>foo</li></ul>' );
-		// Collapsing selection in model, which has just flat listItems.
-		editor.document.selection.collapse( editor.document.getRoot().getChild( 0 ) );
+		setData( doc, '<listItem type="bulleted" indent="0">[]foo</listItem>' );
 
 		const command = editor.commands.get( 'numberedList' );
 
@@ -99,9 +98,7 @@ describe( 'List', () => {
 
 			sinon.spy( editor, 'execute' );
 
-			editor.setData( '<ul><li></li></ul>' );
-			// Collapsing selection in model, which has just flat listItems.
-			editor.document.selection.collapse( editor.document.getRoot().getChild( 0 ) );
+			setData( doc, '<listItem type="bulleted" indent="0">[]</listItem>' );
 
 			editor.editing.view.fire( 'enter', domEvtDataStub );
 
@@ -114,9 +111,7 @@ describe( 'List', () => {
 
 			sinon.spy( editor, 'execute' );
 
-			editor.setData( '<ul><li>foobar</li></ul>' );
-			// Collapsing selection in model, which has just flat listItems.
-			editor.document.selection.collapse( editor.document.getRoot().getChild( 0 ) );
+			setData( doc, '<listItem type="bulleted" indent="0">foo[]</listItem>' );
 
 			editor.editing.view.fire( 'enter', domEvtDataStub );
 
@@ -141,9 +136,11 @@ describe( 'List', () => {
 		} );
 
 		it( 'should execute indentList command on tab key', () => {
-			editor.setData( '<ul><li>foo</li><li>bar</li></ul>' );
-			// Collapsing selection in model, which has just flat listItems.
-			editor.document.selection.collapse( editor.document.getRoot().getChild( 1 ) );
+			setData(
+				doc,
+				'<listItem type="bulleted" indent="0">foo</listItem>' +
+				'<listItem type="bulleted" indent="0">[]bar</listItem>'
+			);
 
 			editor.editing.view.fire( 'keydown', domEvtDataStub );
 
@@ -154,9 +151,11 @@ describe( 'List', () => {
 		it( 'should execute outdentList command on Shift+Tab keystroke', () => {
 			domEvtDataStub.keystroke += getCode( 'Shift' );
 
-			editor.setData( '<ul><li>foo<ul><li>bar</li></ul></li></ul>' );
-			// Collapsing selection in model, which has just flat listItems.
-			editor.document.selection.collapse( editor.document.getRoot().getChild( 1 ) );
+			setData(
+				doc,
+				'<listItem type="bulleted" indent="0">foo</listItem>' +
+				'<listItem type="bulleted" indent="1">[]bar</listItem>'
+			);
 
 			editor.editing.view.fire( 'keydown', domEvtDataStub );
 
@@ -165,9 +164,7 @@ describe( 'List', () => {
 		} );
 
 		it( 'should not indent if command is disabled', () => {
-			editor.setData( '<ul><li>foo</li></ul>' );
-			// Collapsing selection in model, which has just flat listItems.
-			editor.document.selection.collapse( editor.document.getRoot().getChild( 0 ) );
+			setData( doc, '<listItem type="bulleted" indent="0">[]foo</listItem>' );
 
 			editor.editing.view.fire( 'keydown', domEvtDataStub );
 
@@ -177,9 +174,11 @@ describe( 'List', () => {
 		it( 'should not indent or outdent if alt+tab is pressed', () => {
 			domEvtDataStub.keystroke += getCode( 'alt' );
 
-			editor.setData( '<ul><li>foo</li></ul>' );
-			// Collapsing selection in model, which has just flat listItems.
-			editor.document.selection.collapse( editor.document.getRoot().getChild( 0 ) );
+			setData(
+				doc,
+				'<listItem type="bulleted" indent="0">foo</listItem>' +
+				'<listItem type="bulleted" indent="0">[]bar</listItem>'
+			);
 
 			editor.editing.view.fire( 'keydown', domEvtDataStub );
 

+ 37 - 33
packages/ckeditor5-list/tests/listcommand.js

@@ -62,35 +62,31 @@ describe( 'ListCommand', () => {
 
 		describe( 'value', () => {
 			it( 'should be false if first position in selection is not in a list item', () => {
-				doc.selection.collapse( doc.getRoot().getChild( 3 ) );
+				doc.enqueueChanges( () => {
+					doc.selection.collapse( doc.getRoot().getChild( 3 ) );
+				} );
+
 				expect( command.value ).to.be.false;
 			} );
 
 			it( 'should be false if first position in selection is in a list item of different type', () => {
-				doc.selection.collapse( doc.getRoot().getChild( 2 ) );
+				doc.enqueueChanges( () => {
+					doc.selection.collapse( doc.getRoot().getChild( 2 ) );
+				} );
+
 				expect( command.value ).to.be.false;
 			} );
 
 			it( 'should be true if first position in selection is in a list item of same type', () => {
-				doc.selection.collapse( doc.getRoot().getChild( 1 ) );
+				doc.enqueueChanges( () => {
+					doc.selection.collapse( doc.getRoot().getChild( 1 ) );
+				} );
+
 				expect( command.value ).to.be.true;
 			} );
 		} );
 
 		describe( 'isEnabled', () => {
-			it( 'should be true if command value is true', () => {
-				command.value = true;
-				command.refreshState();
-
-				expect( command.isEnabled ).to.be.true;
-
-				command.value = false;
-				doc.selection.collapse( doc.getRoot().getChild( 1 ) );
-
-				expect( command.value ).to.be.true;
-				expect( command.isEnabled ).to.be.true;
-			} );
-
 			it( 'should be true if entire selection is in a list', () => {
 				setData( doc, '<listItem type="bulleted" indent="0">[a]</listItem>' );
 				expect( command.isEnabled ).to.be.true;
@@ -252,10 +248,12 @@ describe( 'ListCommand', () => {
 				it( 'should rename closest block to listItem and set correct attributes', () => {
 					// From first paragraph to second paragraph.
 					// Command value=false, we are turning on list items.
-					doc.selection.setRanges( [ new Range(
-						Position.createAt( root.getChild( 2 ) ),
-						Position.createAt( root.getChild( 3 ) )
-					) ] );
+					doc.enqueueChanges( () => {
+						doc.selection.setRanges( [ new Range(
+							Position.createAt( root.getChild( 2 ) ),
+							Position.createAt( root.getChild( 3 ) )
+						) ] );
+					} );
 
 					command._doExecute();
 
@@ -275,10 +273,12 @@ describe( 'ListCommand', () => {
 				it( 'should rename closest listItem to paragraph', () => {
 					// From second bullet list item to first numbered list item.
 					// Command value=true, we are turning off list items.
-					doc.selection.setRanges( [ new Range(
-						Position.createAt( root.getChild( 1 ) ),
-						Position.createAt( root.getChild( 4 ) )
-					) ] );
+					doc.enqueueChanges( () => {
+						doc.selection.setRanges( [ new Range(
+							Position.createAt( root.getChild( 1 ) ),
+							Position.createAt( root.getChild( 4 ) )
+						) ] );
+					} );
 
 					// Convert paragraphs, leave numbered list items.
 					command._doExecute();
@@ -298,10 +298,12 @@ describe( 'ListCommand', () => {
 
 				it( 'should change closest listItem\'s type', () => {
 					// From first numbered lsit item to third bulleted list item.
-					doc.selection.setRanges( [ new Range(
-						Position.createAt( root.getChild( 4 ) ),
-						Position.createAt( root.getChild( 6 ) )
-					) ] );
+					doc.enqueueChanges( () => {
+						doc.selection.setRanges( [ new Range(
+							Position.createAt( root.getChild( 4 ) ),
+							Position.createAt( root.getChild( 6 ) )
+						) ] );
+					} );
 
 					// Convert paragraphs, leave numbered list items.
 					command._doExecute();
@@ -320,11 +322,13 @@ describe( 'ListCommand', () => {
 				} );
 
 				it( 'should handle outdenting sub-items when list item is turned off', () => {
-					// From first numbered lsit item to third bulleted list item.
-					doc.selection.setRanges( [ new Range(
-						Position.createAt( root.getChild( 1 ) ),
-						Position.createAt( root.getChild( 5 ) )
-					) ] );
+					// From first numbered list item to third bulleted list item.
+					doc.enqueueChanges( () => {
+						doc.selection.setRanges( [ new Range(
+							Position.createAt( root.getChild( 1 ) ),
+							Position.createAt( root.getChild( 5 ) )
+						) ] );
+					} );
 
 					// Convert paragraphs, leave numbered list items.
 					command._doExecute();