Browse Source

Added: support for correct handling of combining marks and surrogate pairs.

Szymon Cofalik 9 years ago
parent
commit
3bbfb59055

+ 54 - 12
packages/ckeditor5-engine/src/model/composer/modifyselection.js

@@ -6,21 +6,44 @@
 import Position from '../position.js';
 import TreeWalker from '../treewalker.js';
 import Range from '../range.js';
+import { isInsideSurrogatePair, isInsideCombinedSymbol } from '../../../utils/unicode.js';
 
+/* jshint -W100 */
 /**
  * Modifies the selection. Currently the supported modifications are:
  *
- * * Extending. The selection focus is moved in the specified `options.direction` with a step specified in `options.unit`
- * (defaults to `'character'`, other values are not not yet supported).
- * Note: if you extend a forward selection in a backward direction you will in fact shrink it.
+ * * Extending. The selection focus is moved in the specified `options.direction` with a step specified in `options.unit`.
+ * Possible values for `unit` are:
+ *  * `'character'` (default) - moves selection by one user-perceived character. In most cases this means moving by one
+ *  character in {String} sense. However, unicode also defines "combing marks". These are special symbols, that combines
+ *  with a symbol before it ("base character") to create one user-perceived character. For example, `q̣̇` is a normal
+ *  letter `q` with two "combining marks": upper dot (`Ux0307`) and lower dot (`Ux0323`). For most actions, i.e. extending
+ *  selection by one position, it is correct to include both "base character" and all of it's "combining marks". That is
+ *  why `'character'` value is most natural and common method of modifying selection.
+ *  * `'codePoint'` - moves selection by one unicode code point. In contrary to, `'character'` unit, this will insert
+ *  selection between "base character" and "combining mark", because "combining marks" have their own unicode code points.
+ *  However, for technical reasons, unicode code points with values above `UxFFFF` are represented in native {String} by
+ *  two characters, called "surrogate pairs". Halves of "surrogate pairs" have a meaning only when placed next to each other.
+ *  For example `𨭎` is represented in {String} by `\uD862\uDF4E`. Both `\uD862` and `\uDF4E` do not have any meaning
+ *  outside the pair (are rendered as ? when alone). Position between them would be incorrect. In this case, selection
+ *  extension will include whole "surrogate pair".
+ *
+ * **Note:** if you extend a forward selection in a backward direction you will in fact shrink it.
+ *
+ * **Note:** you may use `CKEditor5 Graphemes` feature available at https://github.com/ckeditor/ckeditor5-graphemes
+ * to enhance `'character'` option to support so-called "graphemes". This feature is not available in
+ * `engine` out-of-the-box due to it's big size and niche usage.
  *
  * @method engine.model.composer.modifySelection
- * @param {engine.model.Selection} The selection to modify.
+ * @param {engine.model.Selection} selection The selection to modify.
  * @param {Object} [options]
  * @param {'forward'|'backward'} [options.direction='forward'] The direction in which the selection should be modified.
+ * @param {'character'|'codePoint'} [options.unit='character'] The unit by which selection should be modified.
  */
+/* jshint +W100 */
 export default function modifySelection( selection, options = {} ) {
 	const isForward = options.direction != 'backward';
+	options.unit = options.unit ? options.unit : 'character';
 
 	const focus = selection.focus;
 	const walker = new TreeWalker( {
@@ -38,21 +61,22 @@ export default function modifySelection( selection, options = {} ) {
 
 	let value = next.value;
 
-	// 2. Consume next character.
+	// 2. Focus is before/after text. Extending by text data.
 	if ( value.type == 'text' ) {
-		selection.setFocus( value.nextPosition );
+		selection.setFocus( getCorrectPosition( walker, options.unit ) );
 
 		return;
 	}
 
-	// 3. We're entering an element, so let's consume it fully.
+	// 3. Focus is before/after element. Extend by whole element.
 	if ( value.type == ( isForward ? 'elementStart' : 'elementEnd' ) ) {
 		selection.setFocus( value.item, isForward ? 'after' : 'before' );
 
 		return;
 	}
 
-	// 4. We're leaving an element. That's more tricky.
+	// 4. If previous scenarios are false, it means that focus is at the beginning/at the end of element and by
+	// extending we are "leaving" the element. Let's see what is further.
 	next = walker.next();
 
 	// 4.1. Nothing left, so let's stay where we were.
@@ -60,20 +84,38 @@ export default function modifySelection( selection, options = {} ) {
 		return;
 	}
 
-	// Replace TreeWalker step wrapper by clean step value.
 	value = next.value;
 
-	// 4.2. Character found after element end. Not really a valid case in our data model, but let's
-	// do something sensible and put the selection focus before that character.
+	// 4.2. Text data found after leaving an element end. Put selection before it. This way extension will include
+	// "opening" element tag.
 	if ( value.type == 'text' ) {
 		selection.setFocus( value.previousPosition );
 	}
-	// 4.3. OK, we're entering a new element. So let's place there the focus.
+	// 4.3. An element found after leaving previous element. Put focus inside that element, at it's beginning or end.
 	else {
 		selection.setFocus( value.item, isForward ? 0 : 'end' );
 	}
 }
 
+// Finds a correct position by walking in a text node and checking whether selection can be extended to given position
+// or should be extended further.
+function getCorrectPosition( walker, unit ) {
+	const textNode = walker.position.textNode;
+
+	if ( textNode ) {
+		const data = textNode.data;
+		let offset = walker.position.offset - textNode.startOffset;
+
+		while ( isInsideSurrogatePair( data, offset ) || ( unit == 'character' && isInsideCombinedSymbol( data, offset ) ) ) {
+			walker.next();
+
+			offset = walker.position.offset - textNode.startOffset;
+		}
+	}
+
+	return walker.position;
+}
+
 function getSearchRange( start, isForward ) {
 	const root = start.root;
 	const searchEnd = Position.createAt( root, isForward ? 'end' : 0 );

+ 44 - 0
packages/ckeditor5-engine/src/model/document.js

@@ -17,6 +17,7 @@ import clone from '../../utils/lib/lodash/clone.js';
 import EmitterMixin from '../../utils/emittermixin.js';
 import CKEditorError from '../../utils/ckeditorerror.js';
 import mix from '../../utils/mix.js';
+import { isInsideSurrogatePair, isInsideCombinedSymbol } from '../../utils/unicode.js';
 
 const graveyardName = '$graveyard';
 
@@ -111,6 +112,22 @@ export default class Document {
 			this.selection._updateAttributes();
 		} );
 
+		// Add events that will ensure selection correctness.
+		this.selection.on( 'change:range', () => {
+			for ( let range of this.selection.getRanges() ) {
+				if ( !this._validateSelectionRange( range ) ) {
+					/**
+					 * Range from document selection starts or ends at incorrect position.
+					 *
+					 * @error document-selection-wrong-position
+					 * @param {engine.model.Range} range
+					 */
+					throw new CKEditorError( 'document-selection-wrong-position: ' +
+						'Range from document selection starts or ends at incorrect position.', { range } );
+				}
+			}
+		} );
+
 		// Graveyard tree root. Document always have a graveyard root, which stores removed nodes.
 		this.createRoot( '$root', graveyardName );
 	}
@@ -307,6 +324,18 @@ export default class Document {
 	}
 
 	/**
+	 * Checks whether given {@link engine.model.Range range} is a valid range for
+	 * {@link engine.model.Document#selection document's selection}.
+	 *
+	 * @private
+	 * @param {engine.model.Range} range Range to check.
+	 * @returns {Boolean} `true` if `range` is valid, `false` otherwise.
+	 */
+	_validateSelectionRange( range ) {
+		return validateTextNodePosition( range.start ) && validateTextNodePosition( range.end );
+	}
+
+	/**
 	 * Fired when document changes by applying an operation.
 	 *
 	 * There are 5 types of change:
@@ -349,3 +378,18 @@ export default class Document {
 }
 
 mix( Document, EmitterMixin );
+
+// Checks whether given range boundary position is valid for document selection, meaning that is not between
+// unicode surrogate pairs or base character and combining marks.
+function validateTextNodePosition( rangeBoundary ) {
+	const textNode = rangeBoundary.textNode;
+
+	if ( textNode ) {
+		const data = textNode.data;
+		const offset = rangeBoundary.offset - textNode.startOffset;
+
+		return !isInsideSurrogatePair( data, offset ) && !isInsideCombinedSymbol( data, offset );
+	}
+
+	return true;
+}

+ 147 - 9
packages/ckeditor5-engine/tests/model/composer/modifyselection.js

@@ -6,8 +6,9 @@
 /* bender-tags: model, composer */
 
 import Document from '/ckeditor5/engine/model/document.js';
+import Selection from '/ckeditor5/engine/model/selection.js';
 import modifySelection from '/ckeditor5/engine/model/composer/modifyselection.js';
-import { setData, getData } from '/tests/engine/_utils/model.js';
+import { setData, stringify } from '/tests/engine/_utils/model.js';
 
 describe( 'Delete utils', () => {
 	let document;
@@ -123,6 +124,64 @@ describe( 'Delete utils', () => {
 					'<p>fo<selection backward><img></img></selection>o</p>',
 					{ direction: 'backward' }
 				);
+
+				//test(
+				//	'unicode support - forward',
+				//	'<p>நி<selection>லை</selection>க்கு</p>',
+				//	'<p>நி<selection>லைக்</selection>கு</p>'
+				//);
+				//
+				//test(
+				//	'unicode support - backward',
+				//	'<p>நி<selection backward>லை</selection>க்கு</p>',
+				//	'<p><selection backward>நிலை</selection>க்கு</p>',
+				//	{ direction: 'backward' }
+				//);
+
+				test(
+					'unicode support - combining mark forward',
+					'<p>foo<selection />b̂ar</p>',
+					'<p>foo<selection>b̂</selection>ar</p>'
+				);
+
+				test(
+					'unicode support - combining mark backward',
+					'<p>foob̂<selection />ar</p>',
+					'<p>foo<selection backward>b̂</selection>ar</p>',
+					{ direction: 'backward' }
+				);
+
+				test(
+					'unicode support - combining mark multiple',
+					'<p>fo<selection />o̻̐ͩbar</p>',
+					'<p>fo<selection>o̻̐ͩ</selection>bar</p>'
+				);
+
+				test(
+					'unicode support - combining mark multiple backward',
+					'<p>foo̻̐ͩ<selection />bar</p>',
+					'<p>fo<selection backward>o̻̐ͩ</selection>bar</p>',
+					{ direction: 'backward' }
+				);
+
+				test(
+					'unicode support - combining mark to the end',
+					'<p>fo<selection />o̻̐ͩ</p>',
+					'<p>fo<selection>o̻̐ͩ</selection></p>'
+				);
+
+				test(
+					'unicode support - surrogate pairs forward',
+					'<p><selection />\uD83D\uDCA9</p>',
+					'<p><selection>\uD83D\uDCA9</selection></p>'
+				);
+
+				test(
+					'unicode support - surrogate pairs backward',
+					'<p>\uD83D\uDCA9<selection /></p>',
+					'<p><selection backward>\uD83D\uDCA9</selection></p>',
+					{ direction: 'backward' }
+				);
 			} );
 
 			describe( 'beyond element', () => {
@@ -218,21 +277,100 @@ describe( 'Delete utils', () => {
 			} );
 		} );
 
-		test(
-			'updates selection attributes',
-			'<p><$text bold=true>foo</$text><selection>b</selection></p>',
-			'<p><$text bold=true>foo</$text><selection bold=true />b</p>',
-			{ direction: 'backward' }
-		);
+		describe( 'unit=codePoint', () => {
+			test(
+				'does nothing on empty content',
+				'<selection />',
+				'<selection />',
+				{ unit: 'codePoint' }
+			);
+
+			test(
+				'does nothing on empty content (with empty element)',
+				'<p><selection /></p>',
+				'<p><selection /></p>',
+				{ unit: 'codePoint' }
+			);
+
+			test(
+				'extends one user-perceived character forward - latin letters',
+				'<p>f<selection />oo</p>',
+				'<p>f<selection>o</selection>o</p>',
+				{ unit: 'codePoint' }
+			);
+
+			test(
+				'extends one user-perceived character backward - latin letters',
+				'<p>fo<selection />o</p>',
+				'<p>f<selection backward>o</selection>o</p>',
+				{ unit: 'codePoint', direction: 'backward' }
+			);
+
+			test(
+				'unicode support - combining mark forward',
+				'<p>foo<selection />b̂ar</p>',
+				'<p>foo<selection>b</selection>̂ar</p>',
+				{ unit: 'codePoint' }
+			);
+
+			test(
+				'unicode support - combining mark backward',
+				'<p>foob̂<selection />ar</p>',
+				'<p>foob<selection backward>̂</selection>ar</p>',
+				{ unit: 'codePoint', direction: 'backward' }
+			);
+
+			test(
+				'unicode support - combining mark multiple',
+				'<p>fo<selection />o̻̐ͩbar</p>',
+				'<p>fo<selection>o</selection>̻̐ͩbar</p>',
+				{ unit: 'codePoint' }
+			);
+
+			//test(
+			//	'extends one unicode code point forward',
+			//	'<p>நி<selection>லை</selection>க்கு</p>',
+			//	'<p>நி<selection>லைக</selection>்கு</p>',
+			//	{ unit: 'codePoint' }
+			//);
+			//
+			//test(
+			//	'shrinks one unicode code point backward (combining mark case) ',
+			//	'<p>நி<selection>லைக்</selection>கு</p>',
+			//	'<p>நி<selection>லைக</selection>்கு</p>',
+			//	{ unit: 'codePoint', direction: 'backward' }
+			//);
+
+			test(
+				'unicode support - surrogate pairs forward',
+				'<p><selection />\uD83D\uDCA9</p>',
+				'<p><selection>\uD83D\uDCA9</selection></p>',
+				{ unit: 'codePoint' }
+			);
+
+			test(
+				'unicode support surrogate pairs backward',
+				'<p>\uD83D\uDCA9<selection /></p>',
+				'<p><selection backward>\uD83D\uDCA9</selection></p>',
+				{ unit: 'codePoint', direction: 'backward' }
+			);
+		} );
 	} );
 
 	function test( title, input, output, options ) {
 		it( title, () => {
+			input = input.normalize();
+			output = output.normalize();
+
 			setData( document, input );
 
-			modifySelection( document.selection, options );
+			// Creating new instance of selection instead of operation on engine.model.Document#selection.
+			// Document's selection will throw errors in some test cases (which are correct cases, but only for
+			// non-document selections).
+			const testSelection = Selection.createFromSelection( document.selection );
+			modifySelection( testSelection, options );
 
-			expect( getData( document ) ).to.equal( output );
+			expect( stringify( document.getRoot(), testSelection ) ).to.equal( output );
 		} );
 	}
 } );

+ 54 - 9
packages/ckeditor5-engine/tests/model/document/document.js

@@ -11,6 +11,7 @@ import Composer from '/ckeditor5/engine/model/composer/composer.js';
 import RootElement from '/ckeditor5/engine/model/rootelement.js';
 import Batch from '/ckeditor5/engine/model/batch.js';
 import Delta from '/ckeditor5/engine/model/delta/delta.js';
+import Range from '/ckeditor5/engine/model/range.js';
 import CKEditorError from '/ckeditor5/utils/ckeditorerror.js';
 import count from '/ckeditor5/utils/count.js';
 import { jsonParseStringify } from '/tests/engine/model/_utils/utils.js';
@@ -223,20 +224,64 @@ describe( 'Document', () => {
 		} );
 	} );
 
-	it( 'should update selection attributes whenever selection gets updated', () => {
-		sinon.spy( doc.selection, '_updateAttributes' );
+	describe( 'selection', () => {
+		it( 'should get updated attributes whenever selection gets updated', () => {
+			sinon.spy( doc.selection, '_updateAttributes' );
 
-		doc.selection.fire( 'change:range' );
+			doc.selection.fire( 'change:range' );
 
-		expect( doc.selection._updateAttributes.called ).to.be.true;
-	} );
+			expect( doc.selection._updateAttributes.called ).to.be.true;
+		} );
+
+		it( 'should get updated attributes whenever changes to the document are applied', () => {
+			sinon.spy( doc.selection, '_updateAttributes' );
+
+			doc.fire( 'changesDone' );
 
-	it( 'should update selection attributes whenever changes to the document are applied', () => {
-		sinon.spy( doc.selection, '_updateAttributes' );
+			expect( doc.selection._updateAttributes.called ).to.be.true;
+		} );
+
+		it( 'should throw if one of ranges starts or ends inside surrogate pair', () => {
+			const root = doc.createRoot();
+			root.appendChildren( '\uD83D\uDCA9' );
 
-		doc.fire( 'changesDone' );
+			expect( () => {
+				doc.selection.setRanges( [ Range.createFromParentsAndOffsets( root, 0, root, 1 ) ] );
+			} ).to.throw( CKEditorError, /document-selection-wrong-position/ );
+
+			expect( () => {
+				doc.selection.setRanges( [ Range.createFromParentsAndOffsets( root, 1, root, 2 ) ] );
+			} ).to.throw( CKEditorError, /document-selection-wrong-position/ );
+		} );
 
-		expect( doc.selection._updateAttributes.called ).to.be.true;
+		it( 'should throw if one of ranges starts or ends between base character and combining mark', () => {
+			const root = doc.createRoot();
+			root.appendChildren( 'foo̻̐ͩbar' );
+
+			expect( () => {
+				doc.selection.setRanges( [ Range.createFromParentsAndOffsets( root, 3, root, 9 ) ] );
+			} ).to.throw( CKEditorError, /document-selection-wrong-position/ );
+
+			expect( () => {
+				doc.selection.setRanges( [ Range.createFromParentsAndOffsets( root, 4, root, 9 ) ] );
+			} ).to.throw( CKEditorError, /document-selection-wrong-position/ );
+
+			expect( () => {
+				doc.selection.setRanges( [ Range.createFromParentsAndOffsets( root, 5, root, 9 ) ] );
+			} ).to.throw( CKEditorError, /document-selection-wrong-position/ );
+
+			expect( () => {
+				doc.selection.setRanges( [ Range.createFromParentsAndOffsets( root, 1, root, 3 ) ] );
+			} ).to.throw( CKEditorError, /document-selection-wrong-position/ );
+
+			expect( () => {
+				doc.selection.setRanges( [ Range.createFromParentsAndOffsets( root, 1, root, 4 ) ] );
+			} ).to.throw( CKEditorError, /document-selection-wrong-position/ );
+
+			expect( () => {
+				doc.selection.setRanges( [ Range.createFromParentsAndOffsets( root, 1, root, 5 ) ] );
+			} ).to.throw( CKEditorError, /document-selection-wrong-position/ );
+		} );
 	} );
 
 	describe( '_getDefaultRoot', () => {