From 94c4eb2125ba2f1bc78ff0bd2121f836381e6a60 Mon Sep 17 00:00:00 2001 From: iseulde Date: Mon, 3 Dec 2018 14:08:26 +0100 Subject: [PATCH 1/8] RichText: only set range if different --- packages/rich-text/src/to-dom.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/packages/rich-text/src/to-dom.js b/packages/rich-text/src/to-dom.js index 1c34d8680dd2b..10b2826efae99 100644 --- a/packages/rich-text/src/to-dom.js +++ b/packages/rich-text/src/to-dom.js @@ -251,6 +251,15 @@ export function applyValue( future, current ) { } } +function isRangeEqual( a, b ) { + return ( + a.startContainer === b.startContainer && + a.startOffset === b.startOffset && + a.endContainer === b.endContainer && + a.endOffset === b.endOffset + ); +} + export function applySelection( selection, current ) { const { node: startContainer, offset: startOffset } = getNodeByPath( current, selection.startPath ); const { node: endContainer, offset: endOffset } = getNodeByPath( current, selection.endPath ); @@ -283,6 +292,11 @@ export function applySelection( selection, current ) { range.setEnd( endContainer, endOffset ); } + // If ranges the live range is the same, there's no need to remove and add. + if ( isRangeEqual( range, windowSelection.getRangeAt( 0 ) ) ) { + return; + } + windowSelection.removeAllRanges(); windowSelection.addRange( range ); } From 11a80b1a9e47fe9691929843c2b951378a952815 Mon Sep 17 00:00:00 2001 From: iseulde Date: Mon, 3 Dec 2018 15:46:16 +0100 Subject: [PATCH 2/8] Check rangeCount --- packages/rich-text/src/to-dom.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/rich-text/src/to-dom.js b/packages/rich-text/src/to-dom.js index 10b2826efae99..4b27aeeb8a73e 100644 --- a/packages/rich-text/src/to-dom.js +++ b/packages/rich-text/src/to-dom.js @@ -292,11 +292,15 @@ export function applySelection( selection, current ) { range.setEnd( endContainer, endOffset ); } - // If ranges the live range is the same, there's no need to remove and add. - if ( isRangeEqual( range, windowSelection.getRangeAt( 0 ) ) ) { - return; + if ( windowSelection.rangeCount > 0 ) { + // If the to be added range and the live range are the same, there's no + // need to remove the live range and add the equivalent range. + if ( isRangeEqual( range, windowSelection.getRangeAt( 0 ) ) ) { + return; + } + + windowSelection.removeAllRanges(); } - windowSelection.removeAllRanges(); windowSelection.addRange( range ); } From a8ab4e6709d3bfad6a9ca874247a65686d8e4fe7 Mon Sep 17 00:00:00 2001 From: iseulde Date: Tue, 4 Dec 2018 10:51:27 +0100 Subject: [PATCH 3/8] Also compare nodes --- packages/rich-text/src/to-dom.js | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/packages/rich-text/src/to-dom.js b/packages/rich-text/src/to-dom.js index 4b27aeeb8a73e..86e375a1626a8 100644 --- a/packages/rich-text/src/to-dom.js +++ b/packages/rich-text/src/to-dom.js @@ -226,21 +226,17 @@ export function apply( { export function applyValue( future, current ) { let i = 0; + let futureChild; - while ( future.firstChild ) { + while ( ( futureChild = future.firstChild ) ) { const currentChild = current.childNodes[ i ]; - const futureNodeType = future.firstChild.nodeType; if ( ! currentChild ) { - current.appendChild( future.firstChild ); - } else if ( - futureNodeType !== currentChild.nodeType || - futureNodeType !== TEXT_NODE || - future.firstChild.nodeValue !== currentChild.nodeValue - ) { - current.replaceChild( future.firstChild, currentChild ); + current.appendChild( futureChild ); + } else if ( ! currentChild.isEqualNode( futureChild ) ) { + current.replaceChild( futureChild, currentChild ); } else { - future.removeChild( future.firstChild ); + future.removeChild( futureChild ); } i++; From 1dcd44087cec09452e363ae2c71cc58b0cc182c1 Mon Sep 17 00:00:00 2001 From: iseulde Date: Wed, 5 Dec 2018 11:01:17 +0100 Subject: [PATCH 4/8] Add e2e test --- .../__snapshots__/rich-text.test.js.snap | 6 +++ test/e2e/specs/rich-text.test.js | 53 +++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/test/e2e/specs/__snapshots__/rich-text.test.js.snap b/test/e2e/specs/__snapshots__/rich-text.test.js.snap index e911f407edd68..275080b436f2d 100644 --- a/test/e2e/specs/__snapshots__/rich-text.test.js.snap +++ b/test/e2e/specs/__snapshots__/rich-text.test.js.snap @@ -24,6 +24,12 @@ exports[`RichText should handle change in tag name gracefully 1`] = ` " `; +exports[`RichText should only mutate text data on input 1`] = ` +" +

1234

+" +`; + exports[`RichText should transform backtick to code 1`] = ` "

A backtick

diff --git a/test/e2e/specs/rich-text.test.js b/test/e2e/specs/rich-text.test.js index dca248c828931..dd2e1d2c56d68 100644 --- a/test/e2e/specs/rich-text.test.js +++ b/test/e2e/specs/rich-text.test.js @@ -67,4 +67,57 @@ describe( 'RichText', () => { expect( await getEditedPostContent() ).toMatchSnapshot(); } ); + + it( 'should only mutate text data on input', async () => { + await clickBlockAppender(); + await page.keyboard.type( '1' ); + await pressWithModifier( 'primary', 'b' ); + await page.keyboard.type( '2' ); + await pressWithModifier( 'primary', 'b' ); + await page.keyboard.type( '3' ); + + await page.evaluate( () => { + let called; + const { body } = document; + const config = { + attributes: true, + childList: true, + characterData: true, + subtree: true, + }; + + const mutationObserver = new MutationObserver( ( records ) => { + if ( called ) { + throw new Error( 'Typing should only mutate once.' ); + } + + if ( records.length > 1 ) { + throw new Error( 'Typing should only mutate once.' ); + } + + records.forEach( ( record ) => { + if ( record.type !== 'characterData' ) { + throw new Error( + `Typing mutated more than character data: ${ record.type }` + ); + } + } ); + + called = true; + } ); + + mutationObserver.observe( body, config ); + + document.addEventListener( 'selectionchange', () => { + // One selection change event is fine. + document.addEventListener( 'selectionchange', () => { + throw new Error( 'Typing should only emit one selection change event.' ); + }, { once: true } ); + }, { once: true } ); + } ); + + await page.keyboard.type( '4' ); + + expect( await getEditedPostContent() ).toMatchSnapshot(); + } ); } ); From 21371243a13c14af21c5167b458e0765f60ff6e0 Mon Sep 17 00:00:00 2001 From: iseulde Date: Wed, 5 Dec 2018 11:02:35 +0100 Subject: [PATCH 5/8] Simplify --- test/e2e/specs/rich-text.test.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/test/e2e/specs/rich-text.test.js b/test/e2e/specs/rich-text.test.js index dd2e1d2c56d68..c6401cb95a65a 100644 --- a/test/e2e/specs/rich-text.test.js +++ b/test/e2e/specs/rich-text.test.js @@ -87,11 +87,7 @@ describe( 'RichText', () => { }; const mutationObserver = new MutationObserver( ( records ) => { - if ( called ) { - throw new Error( 'Typing should only mutate once.' ); - } - - if ( records.length > 1 ) { + if ( called || records.length > 1 ) { throw new Error( 'Typing should only mutate once.' ); } From 35ea03af1075a29565a10016faf8835890c11beb Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Sun, 9 Dec 2018 12:15:42 -0600 Subject: [PATCH 6/8] RichText: Document isRangeEqual --- packages/rich-text/src/to-dom.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/rich-text/src/to-dom.js b/packages/rich-text/src/to-dom.js index 86e375a1626a8..c811bbc618059 100644 --- a/packages/rich-text/src/to-dom.js +++ b/packages/rich-text/src/to-dom.js @@ -247,6 +247,16 @@ export function applyValue( future, current ) { } } +/** + * Returns true if two ranges are equal, or false otherwise. Ranges are + * considered equal if their start and end occur in the same container and + * offset. + * + * @param {Range} a First range object to test. + * @param {Range} b First range object to test. + * + * @return {boolean} Whether the two ranges are equal. + */ function isRangeEqual( a, b ) { return ( a.startContainer === b.startContainer && From cdddba8210f54e1b557267767019b819e7b3fbb9 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Sun, 9 Dec 2018 12:21:51 -0600 Subject: [PATCH 7/8] Testing: RichText: Assure subscriber removal --- test/e2e/specs/rich-text.test.js | 33 +++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/test/e2e/specs/rich-text.test.js b/test/e2e/specs/rich-text.test.js index c6401cb95a65a..a67c609dc1148 100644 --- a/test/e2e/specs/rich-text.test.js +++ b/test/e2e/specs/rich-text.test.js @@ -10,10 +10,18 @@ import { } from '../support/utils'; describe( 'RichText', () => { + let unsubscribes; + beforeEach( async () => { + unsubscribes = []; + await newPost(); } ); + afterEach( () => { + unsubscribes.forEach( ( unsubscribe ) => unsubscribe() ); + } ); + it( 'should handle change in tag name gracefully', async () => { // Regression test: The heading block changes the tag name of its // RichText element. Historically this has been prone to breakage, @@ -69,6 +77,8 @@ describe( 'RichText', () => { } ); it( 'should only mutate text data on input', async () => { + expect.assertions( 2 ); + await clickBlockAppender(); await page.keyboard.type( '1' ); await pressWithModifier( 'primary', 'b' ); @@ -103,12 +113,29 @@ describe( 'RichText', () => { } ); mutationObserver.observe( body, config ); + unsubscribes.push( () => mutationObserver.disconnect() ); document.addEventListener( 'selectionchange', () => { - // One selection change event is fine. - document.addEventListener( 'selectionchange', () => { + // One selection change event is fine. This assertion exists + // to satisfy the `expect.assertions` expected calls. It's + // acceptable that the `selectionchange` listener not be + // removed given that the test must fail if it never reaches + // this point. + expect( true ).toBe( true ); + + function throwMultipleSelectionChange() { throw new Error( 'Typing should only emit one selection change event.' ); - }, { once: true } ); + } + + document.addEventListener( + 'selectionchange', + throwMultipleSelectionChange, + { once: true } + ); + + unsubscribes.push( () => { + document.removeEventListener( 'selectionchange', throwMultipleSelectionChange ); + } ); }, { once: true } ); } ); From 6e1997790bc3168c779d6feb3516356c15599546 Mon Sep 17 00:00:00 2001 From: iseulde Date: Sun, 9 Dec 2018 13:21:31 -0600 Subject: [PATCH 8/8] Unsubscribe in page.evaluate --- test/e2e/specs/rich-text.test.js | 33 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/test/e2e/specs/rich-text.test.js b/test/e2e/specs/rich-text.test.js index a67c609dc1148..2f1012766444f 100644 --- a/test/e2e/specs/rich-text.test.js +++ b/test/e2e/specs/rich-text.test.js @@ -10,18 +10,10 @@ import { } from '../support/utils'; describe( 'RichText', () => { - let unsubscribes; - beforeEach( async () => { - unsubscribes = []; - await newPost(); } ); - afterEach( () => { - unsubscribes.forEach( ( unsubscribe ) => unsubscribe() ); - } ); - it( 'should handle change in tag name gracefully', async () => { // Regression test: The heading block changes the tag name of its // RichText element. Historically this has been prone to breakage, @@ -77,8 +69,6 @@ describe( 'RichText', () => { } ); it( 'should only mutate text data on input', async () => { - expect.assertions( 2 ); - await clickBlockAppender(); await page.keyboard.type( '1' ); await pressWithModifier( 'primary', 'b' ); @@ -113,16 +103,10 @@ describe( 'RichText', () => { } ); mutationObserver.observe( body, config ); - unsubscribes.push( () => mutationObserver.disconnect() ); - document.addEventListener( 'selectionchange', () => { - // One selection change event is fine. This assertion exists - // to satisfy the `expect.assertions` expected calls. It's - // acceptable that the `selectionchange` listener not be - // removed given that the test must fail if it never reaches - // this point. - expect( true ).toBe( true ); + window.unsubscribes = [ () => mutationObserver.disconnect() ]; + document.addEventListener( 'selectionchange', () => { function throwMultipleSelectionChange() { throw new Error( 'Typing should only emit one selection change event.' ); } @@ -133,7 +117,7 @@ describe( 'RichText', () => { { once: true } ); - unsubscribes.push( () => { + window.unsubscribes.push( () => { document.removeEventListener( 'selectionchange', throwMultipleSelectionChange ); } ); }, { once: true } ); @@ -141,6 +125,17 @@ describe( 'RichText', () => { await page.keyboard.type( '4' ); + await page.evaluate( () => { + // The selection change event should be called once. If there's only + // one item in `window.unsubscribes`, it means that only one + // function is present to disconnect the `mutationObserver`. + if ( window.unsubscribes.length === 1 ) { + throw new Error( 'The selection change event listener was never called.' ); + } + + window.unsubscribes.forEach( ( unsubscribe ) => unsubscribe() ); + } ); + expect( await getEditedPostContent() ).toMatchSnapshot(); } ); } );