Merge pull request #981 from ckeditor/t/699

Fix: `LiveSelection` will correctly set its properties in case of a non-collapsed default range. This will fix loading data which starts with an object element. Closes #699.
ckeditor · Jun 27, 2017 · e6e92e9 · e6e92e9
2 parents 31d8ce5 + 5abc583
commit e6e92e9
Show file tree

Hide file tree

Showing 4 changed files with 232 additions and 41 deletions.
diff --git a/src/model/liveselection.js b/src/model/liveselection.js
@@ -88,7 +88,7 @@ export default class LiveSelection extends Selection {
 	get isCollapsed() {
 		const length = this._ranges.length;
 
-		return length === 0 ? true : super.isCollapsed;
+		return length === 0 ? this._document._getDefaultRange().isCollapsed : super.isCollapsed;
 	}
 
 	/**
@@ -102,7 +102,7 @@ export default class LiveSelection extends Selection {
 	 * @inheritDoc
 	 */
 	get focus() {
-		return super.focus || this._document._getDefaultRange().start;
+		return super.focus || this._document._getDefaultRange().end;
 	}
 
 	/**

diff --git a/tests/model/document/document.js b/tests/model/document/document.js
@@ -280,12 +280,18 @@ describe( 'Document', () => {
 
 		beforeEach( () => {
 			doc.schema.registerItem( 'paragraph', '$block' );
+
 			doc.schema.registerItem( 'emptyBlock' );
 			doc.schema.allow( { name: 'emptyBlock', inside: '$root' } );
-			doc.schema.registerItem( 'widget', '$block' );
+
+			doc.schema.registerItem( 'widget' );
 			doc.schema.allow( { name: 'widget', inside: '$root' } );
 			doc.schema.objects.add( 'widget' );
 
+			doc.schema.registerItem( 'blockWidget', '$block' );
+			doc.schema.allow( { name: 'blockWidget', inside: '$root' } );
+			doc.schema.objects.add( 'blockWidget' );
+
 			doc.createRoot();
 			selection = doc.selection;
 		} );
@@ -373,34 +379,6 @@ describe( 'Document', () => {
 			null
 		);
 
-		test(
-			'should select nearest object - both',
-			'<widget></widget>[]<widget></widget>',
-			'both',
-			'[<widget></widget>]<widget></widget>'
-		);
-
-		test(
-			'should select nearest object - forward',
-			'<paragraph></paragraph>[]<widget></widget>',
-			'forward',
-			'<paragraph></paragraph>[<widget></widget>]'
-		);
-
-		test(
-			'should select nearest object - forward',
-			'<paragraph></paragraph>[]<widget></widget>',
-			'forward',
-			'<paragraph></paragraph>[<widget></widget>]'
-		);
-
-		test(
-			'should select nearest object - backward',
-			'<widget></widget>[]<paragraph></paragraph>',
-			'backward',
-			'[<widget></widget>]<paragraph></paragraph>'
-		);
-
 		test(
 			'should move forward when placed at root start',
 			'[]<paragraph></paragraph><paragraph></paragraph>',
@@ -415,6 +393,101 @@ describe( 'Document', () => {
 			'<paragraph></paragraph><paragraph>[]</paragraph>'
 		);
 
+		describe( 'in case of objects which do not allow text inside', () => {
+			test(
+				'should select nearest object (o[]o) - both',
+				'<widget></widget>[]<widget></widget>',
+				'both',
+				'[<widget></widget>]<widget></widget>'
+			);
+
+			test(
+				'should select nearest object (o[]o) - forward',
+				'<widget></widget>[]<widget></widget>',
+				'forward',
+				'<widget></widget>[<widget></widget>]'
+			);
+
+			test(
+				'should select nearest object (o[]o) - backward',
+				'<widget></widget>[]<widget></widget>',
+				'both',
+				'[<widget></widget>]<widget></widget>'
+			);
+
+			test(
+				'should select nearest object (p[]o) - forward',
+				'<paragraph></paragraph>[]<widget></widget>',
+				'forward',
+				'<paragraph></paragraph>[<widget></widget>]'
+			);
+
+			test(
+				'should select nearest object (o[]p) - both',
+				'<widget></widget>[]<paragraph></paragraph>',
+				'both',
+				'[<widget></widget>]<paragraph></paragraph>'
+			);
+
+			test(
+				'should select nearest object (o[]p) - backward',
+				'<widget></widget>[]<paragraph></paragraph>',
+				'backward',
+				'[<widget></widget>]<paragraph></paragraph>'
+			);
+
+			test(
+				'should select nearest object ([]o) - both',
+				'[]<widget></widget><paragraph></paragraph>',
+				'both',
+				'[<widget></widget>]<paragraph></paragraph>'
+			);
+
+			test(
+				'should select nearest object ([]o) - forward',
+				'[]<widget></widget><paragraph></paragraph>',
+				'forward',
+				'[<widget></widget>]<paragraph></paragraph>'
+			);
+
+			test(
+				'should select nearest object (o[]) - both',
+				'<paragraph></paragraph><widget></widget>[]',
+				'both',
+				'<paragraph></paragraph>[<widget></widget>]'
+			);
+
+			test(
+				'should select nearest object (o[]) - backward',
+				'<paragraph></paragraph><widget></widget>[]',
+				'both',
+				'<paragraph></paragraph>[<widget></widget>]'
+			);
+		} );
+
+		describe( 'in case of objects which allow text inside', () => {
+			test(
+				'should select nearest object which allows text (o[]o) - both',
+				'<blockWidget></blockWidget>[]<blockWidget></blockWidget>',
+				'both',
+				'[<blockWidget></blockWidget>]<blockWidget></blockWidget>'
+			);
+
+			test(
+				'should select nearest object (o[]p) - both',
+				'<blockWidget></blockWidget>[]<paragraph></paragraph>',
+				'both',
+				'[<blockWidget></blockWidget>]<paragraph></paragraph>'
+			);
+
+			test(
+				'should select nearest object which allows text ([]o) - both',
+				'[]<blockWidget></blockWidget><paragraph></paragraph>',
+				'both',
+				'[<blockWidget></blockWidget>]<paragraph></paragraph>'
+			);
+		} );
+
 		function test( testName, data, direction, expected ) {
 			it( testName, () => {
 				setData( doc, data );

diff --git a/tests/model/liveselection.js b/tests/model/liveselection.js
@@ -43,6 +43,9 @@ describe( 'LiveSelection', () => {
 		selection = doc.selection;
 		doc.schema.registerItem( 'p', '$block' );
 
+		doc.schema.registerItem( 'widget' );
+		doc.schema.objects.add( 'widget' );
+
 		liveRange = new LiveRange( new Position( root, [ 0 ] ), new Position( root, [ 1 ] ) );
 		range = new Range( new Position( root, [ 2 ] ), new Position( root, [ 2, 2 ] ) );
 	} );
@@ -99,9 +102,66 @@ describe( 'LiveSelection', () => {
 	} );
 
 	describe( 'isCollapsed', () => {
-		it( 'should return true for default range', () => {
+		it( 'should be true for the default range (in text position)', () => {
 			expect( selection.isCollapsed ).to.be.true;
 		} );
+
+		it( 'should be false for the default range (object selection) ', () => {
+			root.insertChildren( 0, new Element( 'widget' ) );
+
+			expect( selection.isCollapsed ).to.be.false;
+		} );
+
+		it( 'should back off to the default algorithm if selection has ranges', () => {
+			selection.addRange( range );
+
+			expect( selection.isCollapsed ).to.be.false;
+		} );
+	} );
+
+	describe( 'anchor', () => {
+		it( 'should equal the default range\'s start (in text position)', () => {
+			const expectedPos = new Position( root, [ 0, 0 ] );
+
+			expect( selection.anchor.isEqual( expectedPos ) ).to.be.true;
+		} );
+
+		it( 'should equal the default range\'s start (object selection)', () => {
+			root.insertChildren( 0, new Element( 'widget' ) );
+
+			const expectedPos = new Position( root, [ 0 ] );
+
+			expect( selection.anchor.isEqual( expectedPos ) ).to.be.true;
+		} );
+
+		it( 'should back off to the default algorithm if selection has ranges', () => {
+			selection.addRange( range );
+
+			expect( selection.anchor.isEqual( range.start ) ).to.be.true;
+		} );
+	} );
+
+	describe( 'focus', () => {
+		it( 'should equal the default range\'s end (in text position)', () => {
+			const expectedPos = new Position( root, [ 0, 0 ] );
+
+			expect( selection.focus.isEqual( expectedPos ) ).to.be.true;
+		} );
+
+		it( 'should equal the default range\'s end (object selection)', () => {
+			root.insertChildren( 0, new Element( 'widget' ) );
+
+			const expectedPos = new Position( root, [ 1 ] );
+
+			expect( selection.focus.isEqual( expectedPos ) ).to.be.true;
+			expect( selection.focus.isEqual( selection.getFirstRange().end ) ).to.be.true;
+		} );
+
+		it( 'should back off to the default algorithm if selection has ranges', () => {
+			selection.addRange( range );
+
+			expect( selection.focus.isEqual( range.end ) ).to.be.true;
+		} );
 	} );
 
 	describe( 'rangeCount', () => {
@@ -118,7 +178,7 @@ describe( 'LiveSelection', () => {
 		} );
 	} );
 
-	describe( 'addRange', () => {
+	describe( 'addRange()', () => {
 		it( 'should convert added Range to LiveRange', () => {
 			selection.addRange( range );
 
@@ -149,7 +209,7 @@ describe( 'LiveSelection', () => {
 		} );
 	} );
 
-	describe( 'collapse', () => {
+	describe( 'collapse()', () => {
 		it( 'detaches all existing ranges', () => {
 			selection.addRange( range );
 			selection.addRange( liveRange );
@@ -161,7 +221,7 @@ describe( 'LiveSelection', () => {
 		} );
 	} );
 
-	describe( 'destroy', () => {
+	describe( 'destroy()', () => {
 		it( 'should unbind all events', () => {
 			selection.addRange( liveRange );
 			selection.addRange( range );
@@ -181,7 +241,7 @@ describe( 'LiveSelection', () => {
 		} );
 	} );
 
-	describe( 'setFocus', () => {
+	describe( 'setFocus()', () => {
 		it( 'modifies default range', () => {
 			const startPos = selection.getFirstPosition();
 			const endPos = Position.createAt( root, 'end' );
@@ -206,7 +266,7 @@ describe( 'LiveSelection', () => {
 		} );
 	} );
 
-	describe( 'removeAllRanges', () => {
+	describe( 'removeAllRanges()', () => {
 		let spy, ranges;
 
 		beforeEach( () => {
@@ -249,7 +309,7 @@ describe( 'LiveSelection', () => {
 		} );
 	} );
 
-	describe( 'setRanges', () => {
+	describe( 'setRanges()', () => {
 		it( 'should throw an error when range is invalid', () => {
 			expect( () => {
 				selection.setRanges( [ { invalid: 'range' } ] );
@@ -295,7 +355,7 @@ describe( 'LiveSelection', () => {
 		} );
 	} );
 
-	describe( 'getFirstRange', () => {
+	describe( 'getFirstRange()', () => {
 		it( 'should return default range if no ranges were added', () => {
 			const firstRange = selection.getFirstRange();
 
@@ -304,7 +364,7 @@ describe( 'LiveSelection', () => {
 		} );
 	} );
 
-	describe( 'getLastRange', () => {
+	describe( 'getLastRange()', () => {
 		it( 'should return default range if no ranges were added', () => {
 			const lastRange = selection.getLastRange();
 
@@ -313,7 +373,7 @@ describe( 'LiveSelection', () => {
 		} );
 	} );
 
-	describe( 'createFromSelection', () => {
+	describe( 'createFromSelection()', () => {
 		it( 'should return a LiveSelection instance', () => {
 			selection.addRange( range, true );
 

diff --git a/tests/tickets/699.js b/tests/tickets/699.js
@@ -0,0 +1,58 @@
+/**
+ * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+/* globals document */
+
+import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor';
+import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
+
+import buildViewConverter from '../../src/conversion/buildviewconverter';
+import buildModelConverter from '../../src/conversion/buildmodelconverter';
+
+import { getData as getModelData } from '../../src/dev-utils/model';
+import { getData as getViewData } from '../../src/dev-utils/view';
+
+describe( 'Bug ckeditor5-engine#699', () => {
+	let element;
+
+	beforeEach( () => {
+		element = document.createElement( 'div' );
+
+		document.body.appendChild( element );
+	} );
+
+	afterEach( () => {
+		element.remove();
+	} );
+
+	it( 'the engine sets the initial selection on the first widget', () => {
+		return ClassicTestEditor
+			.create( element, { plugins: [ Paragraph, WidgetPlugin ] } )
+			.then( editor => {
+				editor.setData( '<widget></widget><p>foo</p>' );
+
+				expect( getModelData( editor.document ) ).to.equal( '[<widget></widget>]<paragraph>foo</paragraph>' );
+				expect( getViewData( editor.editing.view ) ).to.equal( '[<widget></widget>]<p>foo</p>' );
+
+				return editor.destroy();
+			} );
+	} );
+} );
+
+function WidgetPlugin( editor ) {
+	const schema = editor.document.schema;
+
+	schema.registerItem( 'widget' );
+	schema.allow( { name: 'widget', inside: '$root' } );
+	schema.objects.add( 'widget' );
+
+	buildModelConverter().for( editor.data.modelToView, editor.editing.modelToView )
+		.fromElement( 'widget' )
+		.toElement( 'widget' );
+
+	buildViewConverter().for( editor.data.viewToModel )
+		.fromElement( 'widget' )
+		.toElement( 'widget' );
+}