浏览代码

Merge pull request #327 from ckeditor/i/3147

Fix: Fixed various cases with typing multi-byte unicode sequences (e.g. emojis). Closes ckeditor/ckeditor5#3147. Closes ckeditor/ckeditor5#6495.
Piotrek Koszuliński 5 年之前
父节点
当前提交
b913149456

+ 8 - 3
packages/ckeditor5-utils/src/fastdiff.js

@@ -100,13 +100,18 @@ export default function fastDiff( a, b, cmp, atomicChanges = false ) {
 		return a === b;
 	};
 
-	// Transform text or any iterable into arrays for easier, consistent processing.
+	// Convert the string (or any array-like object - eg. NodeList) to an array by using the slice() method because,
+	// unlike Array.from(), it returns array of UTF-16 code units instead of the code points of a string.
+	// One code point might be a surrogate pair of two code units. All text offsets are expected to be in code units.
+	// See ckeditor/ckeditor5#3147.
+	//
+	// We need to make sure here that fastDiff() works identical to diff().
 	if ( !Array.isArray( a ) ) {
-		a = Array.from( a );
+		a = Array.prototype.slice.call( a );
 	}
 
 	if ( !Array.isArray( b ) ) {
-		b = Array.from( b );
+		b = Array.prototype.slice.call( b );
 	}
 
 	// Find first and last change.

+ 99 - 7
packages/ckeditor5-utils/tests/diff.js

@@ -18,33 +18,33 @@ describe( 'diff', () => {
 	} );
 
 	it( 'should diff strings', () => {
-		expect( diff( 'aba', 'acca' ) ).to.deep.equals( [ 'equal', 'insert', 'insert', 'delete', 'equal' ] );
+		expect( diff( 'aba', 'acca' ) ).to.deep.equal( [ 'equal', 'insert', 'insert', 'delete', 'equal' ] );
 		testUtils.sinon.assert.notCalled( fastDiffSpy );
 	} );
 
 	it( 'should diff arrays', () => {
-		expect( diff( Array.from( 'aba' ), Array.from( 'acca' ) ) ).to.deep.equals( [ 'equal', 'insert', 'insert', 'delete', 'equal' ] );
+		expect( diff( Array.from( 'aba' ), Array.from( 'acca' ) ) ).to.deep.equal( [ 'equal', 'insert', 'insert', 'delete', 'equal' ] );
 		testUtils.sinon.assert.notCalled( fastDiffSpy );
 	} );
 
 	it( 'should reverse result if the second string is shorter', () => {
-		expect( diff( 'acca', 'aba' ) ).to.deep.equals( [ 'equal', 'delete', 'delete', 'insert', 'equal' ] );
+		expect( diff( 'acca', 'aba' ) ).to.deep.equal( [ 'equal', 'delete', 'delete', 'insert', 'equal' ] );
 		testUtils.sinon.assert.notCalled( fastDiffSpy );
 	} );
 
 	it( 'should diff if strings are same', () => {
-		expect( diff( 'abc', 'abc' ) ).to.deep.equals( [ 'equal', 'equal', 'equal' ] );
+		expect( diff( 'abc', 'abc' ) ).to.deep.equal( [ 'equal', 'equal', 'equal' ] );
 		testUtils.sinon.assert.notCalled( fastDiffSpy );
 	} );
 
 	it( 'should diff if one string is empty', () => {
-		expect( diff( '', 'abc' ) ).to.deep.equals( [ 'insert', 'insert', 'insert' ] );
+		expect( diff( '', 'abc' ) ).to.deep.equal( [ 'insert', 'insert', 'insert' ] );
 		testUtils.sinon.assert.notCalled( fastDiffSpy );
 	} );
 
 	it( 'should use custom comparator', () => {
-		expect( diff( 'aBc', 'abc' ) ).to.deep.equals( [ 'equal', 'insert', 'delete', 'equal' ] );
-		expect( diff( 'aBc', 'abc', ( a, b ) => a.toLowerCase() == b.toLowerCase() ) ).to.deep.equals( [ 'equal', 'equal', 'equal' ] );
+		expect( diff( 'aBc', 'abc' ) ).to.deep.equal( [ 'equal', 'insert', 'delete', 'equal' ] );
+		expect( diff( 'aBc', 'abc', ( a, b ) => a.toLowerCase() == b.toLowerCase() ) ).to.deep.equal( [ 'equal', 'equal', 'equal' ] );
 		testUtils.sinon.assert.notCalled( fastDiffSpy );
 	} );
 
@@ -67,4 +67,96 @@ describe( 'diff', () => {
 		diff( getLongText( 10 ), getLongText( 1990 ) );
 		testUtils.sinon.assert.called( fastDiffSpy );
 	} );
+
+	describe( 'with multi-byte unicode', () => {
+		describe( 'simple emoji - single unicode code point', () => {
+			// 🙂 = '\ud83d\ude42' = 2 chars
+			const emojiLength = '🙂'.length;
+			const emojiDiffInsert = new Array( emojiLength ).fill( 'insert' );
+			const emojiDiffEqual = new Array( emojiLength ).fill( 'equal' );
+			const emojiDiffDelete = new Array( emojiLength ).fill( 'delete' );
+
+			it( 'should properly handle emoji insertion', () => {
+				expect( diff( 'abc', 'ab🙂c' ) ).to.deep.equal( [ 'equal', 'equal', ...emojiDiffInsert, 'equal' ] );
+			} );
+
+			it( 'should properly handle emoji insertion on the end', () => {
+				expect( diff( 'abc', 'abc🙂' ) ).to.deep.equal( [ 'equal', 'equal', 'equal', ...emojiDiffInsert ] );
+			} );
+
+			it( 'should properly handle appending to string containing emoji', () => {
+				expect( diff( 'abc🙂', 'abc🙂d' ) ).to.deep.equal( [ 'equal', 'equal', 'equal', ...emojiDiffEqual, 'insert' ] );
+			} );
+
+			it( 'should properly handle insertion to string containing emoji', () => {
+				expect( diff( 'ab🙂cd', 'ab🙂cde' ) ).to.deep.equal( [ 'equal', 'equal', ...emojiDiffEqual, 'equal', 'equal', 'insert' ] );
+			} );
+
+			it( 'should properly remove emoji', () => {
+				expect( diff( 'a🙂b', 'ab' ) ).to.deep.equal( [ 'equal', ...emojiDiffDelete, 'equal' ] );
+			} );
+
+			it( 'should properly replace emoji', () => {
+				expect( diff( 'a🙂b', 'axb' ) ).to.deep.equal( [ 'equal', ...emojiDiffDelete, 'insert', 'equal' ] );
+			} );
+
+			it( 'should properly replace one emoji with another', () => {
+				// 😄 = '\ud83d\ude04' = 2 chars
+				// Note both emoji have same first code unit
+				expect( diff( 'a🙂b', 'a😄b' ) ).to.deep.equal(
+					[ 'equal', 'equal', ...emojiDiffInsert.slice( 1 ), ...emojiDiffDelete.slice( 1 ), 'equal' ]
+				);
+			} );
+		} );
+
+		describe( 'combined emoji - unicode ZWJ sequence', () => {
+			// 👩‍🦰 = '\ud83d\udc69\u200d\ud83e\uddB0' = 5 chars
+			const emojiLength = '👩‍🦰'.length;
+			const emojiDiffInsert = new Array( emojiLength ).fill( 'insert' );
+			const emojiDiffEqual = new Array( emojiLength ).fill( 'equal' );
+			const emojiDiffDelete = new Array( emojiLength ).fill( 'delete' );
+
+			it( 'should properly handle emoji insertion (with ZWJ)', () => {
+				expect( diff( 'abc', 'ab👩‍🦰c' ) ).to.deep.equal( [ 'equal', 'equal', ...emojiDiffInsert, 'equal' ] );
+			} );
+
+			it( 'should properly handle emoji insertion on the end (with ZWJ)', () => {
+				expect( diff( 'abc', 'abc👩‍🦰' ) ).to.deep.equal( [ 'equal', 'equal', 'equal', ...emojiDiffInsert ] );
+			} );
+
+			it( 'should properly handle appending to string containing emoji (with ZWJ)', () => {
+				expect( diff( 'ab👩‍🦰', 'ab👩‍🦰c' ) ).to.deep.equal( [ 'equal', 'equal', ...emojiDiffEqual, 'insert' ] );
+			} );
+
+			it( 'should properly handle insertion to string containing emoji (with ZWJ)', () => {
+				expect( diff( 'a👩‍🦰b', 'a👩‍🦰bc' ) ).to.deep.equal( [ 'equal', ...emojiDiffEqual, 'equal', 'insert' ] );
+			} );
+
+			it( 'should properly remove emoji (with ZWJ)', () => {
+				expect( diff( 'a👩‍🦰b', 'ab' ) ).to.deep.equal( [ 'equal', ...emojiDiffDelete, 'equal' ] );
+			} );
+
+			it( 'should properly replace emoji (with ZWJ)', () => {
+				expect( diff( 'a👩‍🦰b', 'axb' ) ).to.deep.equal( [ 'equal', ...emojiDiffDelete, 'insert', 'equal' ] );
+			} );
+
+			it( 'should properly replace ZWJ sequence with simple emoji', () => {
+				const simpleEmojiDiffInsert = new Array( '🙂'.length ).fill( 'insert' );
+
+				// Note that first char of both emoji is the same.
+				expect( diff( 'a👩‍🦰b', 'a🙂b' ) ).to.deep.equal( [
+					'equal', 'equal', ...emojiDiffDelete.slice( 1 ), ...simpleEmojiDiffInsert.slice( 1 ), 'equal'
+				] );
+			} );
+
+			it( 'should properly replace simple emoji with ZWJ sequence', () => {
+				const simpleEmojiDiffDelete = new Array( '🙂'.length ).fill( 'delete' );
+
+				// Note that first char of both emoji is the same.
+				expect( diff( 'a🙂b', 'a👩‍🦰b' ) ).to.deep.equal( [
+					'equal', 'equal', ...emojiDiffInsert.slice( 1 ), ...simpleEmojiDiffDelete.slice( 1 ), 'equal'
+				] );
+			} );
+		} );
+	} );
 } );

+ 139 - 4
packages/ckeditor5-utils/tests/fastdiff.js

@@ -120,6 +120,48 @@ describe( 'fastDiff', () => {
 					{ index: 2, type: 'insert', values: [ { text: 'baz' } ] }
 				], true, ( a, b ) => a.text === b.text );
 			} );
+
+			describe( 'with multi-byte unicode', () => {
+				describe( 'simple emoji - single unicode code point', () => {
+					// 🙂 = '\ud83d\ude42' = 2 chars
+
+					it( 'should properly handle emoji insertion', () => {
+						expectDiff( 'abc', 'ab🙂c', [ { index: 2, type: 'insert', values: '🙂'.split( '' ) } ] );
+					} );
+
+					it( 'should properly handle emoji insertion on the end', () => {
+						expectDiff( 'abc', 'abc🙂', [ { index: 3, type: 'insert', values: '🙂'.split( '' ) } ] );
+					} );
+
+					it( 'should properly handle appending to string containing emoji', () => {
+						expectDiff( 'abc🙂', 'abc🙂d', [ { index: 5, type: 'insert', values: [ 'd' ] } ] );
+					} );
+
+					it( 'should properly handle insertion to string containing emoji', () => {
+						expectDiff( 'ab🙂cd', 'ab🙂cde', [ { index: 6, type: 'insert', values: [ 'e' ] } ] );
+					} );
+				} );
+
+				describe( 'combined emoji - unicode ZWJ sequence', () => {
+					// 👩‍🦰 = '\ud83d\udc69\u200d\ud83e\uddB0' = 5 chars
+
+					it( 'should properly handle emoji with ZWJ insertion', () => {
+						expectDiff( 'abc', 'ab👩‍🦰c', [ { index: 2, type: 'insert', values: '👩‍🦰'.split( '' ) } ] );
+					} );
+
+					it( 'should properly handle emoji (with ZWJ) insertion on the end', () => {
+						expectDiff( 'abc', 'abc👩‍🦰', [ { index: 3, type: 'insert', values: '👩‍🦰'.split( '' ) } ] );
+					} );
+
+					it( 'should properly handle appending to string containing emoji (with ZWJ)', () => {
+						expectDiff( 'ab👩‍🦰', 'ab👩‍🦰c', [ { index: 7, type: 'insert', values: [ 'c' ] } ] );
+					} );
+
+					it( 'should properly handle insertion to string containing emoji (with ZWJ)', () => {
+						expectDiff( 'a👩‍🦰b', 'a👩‍🦰bc', [ { index: 7, type: 'insert', values: [ 'c' ] } ] );
+					} );
+				} );
+			} );
 		} );
 
 		describe( 'deletion', () => {
@@ -196,6 +238,42 @@ describe( 'fastDiff', () => {
 					{ index: 0, type: 'delete', howMany: 1 }
 				], true, ( a, b ) => a.text === b.text );
 			} );
+
+			describe( 'with multi-byte unicode', () => {
+				describe( 'simple emoji - single unicode code point', () => {
+					// 🙂 = '\ud83d\ude42' = 2 chars
+					const emojiLength = '🙂'.split( '' ).length;
+
+					it( 'should properly handle emoji delete', () => {
+						expectDiff( 'ab🙂c', 'abc', [ { index: 2, type: 'delete', howMany: emojiLength } ] );
+					} );
+
+					it( 'should properly handle emoji delete at end', () => {
+						expectDiff( 'ab🙂', 'ab', [ { index: 2, type: 'delete', howMany: emojiLength } ] );
+					} );
+
+					it( 'should properly handle emoji delete at beginning', () => {
+						expectDiff( '🙂ab', 'ab', [ { index: 0, type: 'delete', howMany: emojiLength } ] );
+					} );
+				} );
+
+				describe( 'combined emoji - unicode ZWJ sequence', () => {
+					// 👩‍🦰 = '\ud83d\udc69\u200d\ud83e\uddB0' = 5 chars
+					const emojiLength = '👩‍🦰'.split( '' ).length;
+
+					it( 'should properly handle emoji delete (with ZWJ)', () => {
+						expectDiff( 'ab👩‍🦰c', 'abc', [ { index: 2, type: 'delete', howMany: emojiLength } ] );
+					} );
+
+					it( 'should properly handle emoji delete at end (with ZWJ)', () => {
+						expectDiff( 'ab👩‍🦰', 'ab', [ { index: 2, type: 'delete', howMany: emojiLength } ] );
+					} );
+
+					it( 'should properly handle emoji delete at beginning (with ZWJ)', () => {
+						expectDiff( '👩‍🦰ab', 'ab', [ { index: 0, type: 'delete', howMany: emojiLength } ] );
+					} );
+				} );
+			} );
 		} );
 
 		describe( 'replacement', () => {
@@ -293,6 +371,63 @@ describe( 'fastDiff', () => {
 					{ index: 2, type: 'delete', howMany: 1 }
 				], true, ( a, b ) => a.text === b.text );
 			} );
+
+			describe( 'with multi-byte unicode', () => {
+				// 🙂 = '\ud83d\ude42' = 2 chars
+				const smileEmoji = '🙂'.split( '' );
+
+				// 👩 = '\ud83d\udc69' = 2 chars
+				const womanEmoji = '👩'.split( '' );
+
+				// 👩‍🦰 = '\ud83d\udc69\u200d\ud83e\uddB0' = 5 chars
+				const womanRedHairEmoji = '👩‍🦰'.split( '' );
+
+				// Do not check compatibility with 'diffToChanges' as it generates:
+				// [ { index: 1, type: 'delete', howMany: 2 }, { index: 1, type: 'insert', values: [ 'x' ] } ]
+				it( 'should properly replace emoji with text', () => {
+					expectDiff( 'a🙂b', 'axb', [
+						{ index: 1, type: 'insert', values: [ 'x' ] },
+						{ index: 2, type: 'delete', howMany: smileEmoji.length }
+					], false );
+				} );
+
+				it( 'should properly replace text with emoji', () => {
+					expectDiff( 'abc', 'a👩c', [
+						{ index: 1, type: 'insert', values: womanEmoji },
+						{ index: 3, type: 'delete', howMany: 1 }
+					] );
+				} );
+
+				it( 'should properly replace emoji with emoji', () => {
+					// Note that first char of both emoji is the same.
+					expectDiff( 'a👩b', 'a🙂b', [
+						{ index: 2, type: 'insert', values: smileEmoji.slice( 1 ) },
+						{ index: 3, type: 'delete', howMany: 1 }
+					] );
+				} );
+
+				it( 'should properly replace simple emoji with ZWJ sequence of it', () => {
+					// Note that first 2 chars of both emoji are the same.
+					expectDiff( 'a👩b', 'a👩‍🦰b', [
+						{ index: 3, type: 'insert', values: womanRedHairEmoji.slice( 2 ) }
+					] );
+				} );
+
+				it( 'should properly replace ZWJ sequence with simple emoji (part of sequence)', () => {
+					// Note that first 2 chars of both emoji are the same.
+					expectDiff( 'a👩‍🦰b', 'a👩b', [
+						{ index: 3, type: 'delete', howMany: 3 }
+					] );
+				} );
+
+				it( 'should properly replace simple emoji with other ZWJ sequence', () => {
+					// Note that first char of both emoji is the same.
+					expectDiff( 'a🙂b', 'a👩‍🦰b', [
+						{ index: 2, type: 'insert', values: womanRedHairEmoji.slice( 1 ) },
+						{ index: 6, type: 'delete', howMany: 1 }
+					] );
+				} );
+			} );
 		} );
 	} );
 
@@ -487,10 +622,10 @@ describe( 'fastDiff', () => {
 function expectDiff( oldText, newText, expected, checkDiffToChangesCompatibility = true, comparator = null ) {
 	const result = fastDiff( oldText, newText, comparator );
 
-	expect( result ).to.deep.equals( expected, 'fastDiff changes failed' );
+	expect( result ).to.deep.equal( expected, 'fastDiff changes failed' );
 
 	if ( checkDiffToChangesCompatibility ) {
-		expect( result ).to.deep.equals(
+		expect( result ).to.deep.equal(
 			diffToChanges( diff( oldText, newText, comparator ), newText ), 'diffToChanges compatibility failed' );
 	}
 }
@@ -500,9 +635,9 @@ function expectDiffLinear( oldText, newText, expected, checkDiffCompatibility =
 	const expectedArray = expected.split( '' ).map( item => actions[ item ] );
 	const result = fastDiff( oldText, newText, comparator, true );
 
-	expect( result ).to.deep.equals( expectedArray, 'fastDiff linear result failed' );
+	expect( result ).to.deep.equal( expectedArray, 'fastDiff linear result failed' );
 
 	if ( checkDiffCompatibility ) {
-		expect( result ).to.deep.equals( diff( oldText, newText, comparator ), 'diff compatibility failed' );
+		expect( result ).to.deep.equal( diff( oldText, newText, comparator ), 'diff compatibility failed' );
 	}
 }