From 9212beaf71319676ab48d61447b7996c8efa932f Mon Sep 17 00:00:00 2001 From: Technote Date: Fri, 19 Apr 2019 17:23:10 +0900 Subject: [PATCH 1/7] WordPress/gutenberg#14858 --- packages/rich-text/src/test/to-dom.js | 6 ++++++ packages/rich-text/src/to-dom.js | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/rich-text/src/test/to-dom.js b/packages/rich-text/src/test/to-dom.js index 8db8a52e1b4435..9c7559c9c7f560 100644 --- a/packages/rich-text/src/test/to-dom.js +++ b/packages/rich-text/src/test/to-dom.js @@ -59,6 +59,12 @@ describe( 'applyValue', () => { movedCount: 0, description: 'should not modify', }, + { + current: 'test1 test2 test3', + future: 'test1 test2 test3', + movedCount: 1, + description: 'should apply attributes', + }, ]; cases.forEach( ( { current, future, description, movedCount } ) => { diff --git a/packages/rich-text/src/to-dom.js b/packages/rich-text/src/to-dom.js index 33cc08b27e47df..a9f405e91ab2c3 100644 --- a/packages/rich-text/src/to-dom.js +++ b/packages/rich-text/src/to-dom.js @@ -207,7 +207,7 @@ export function applyValue( future, current ) { const futureAttributes = futureChild.attributes; if ( currentAttributes ) { - for ( let ii = 0; ii < currentAttributes.length; ii++ ) { + for ( let ii = currentAttributes.length; --ii >= 0; ) { const { name } = currentAttributes[ ii ]; if ( ! futureChild.getAttribute( name ) ) { From 6c344af67d959696068f319e4b28c450b776d594 Mon Sep 17 00:00:00 2001 From: Technote Date: Fri, 19 Apr 2019 21:41:56 +0900 Subject: [PATCH 2/7] add test cases, sort attributes before compare (WordPress/gutenberg#14858) --- packages/rich-text/src/test/to-dom.js | 45 ++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/packages/rich-text/src/test/to-dom.js b/packages/rich-text/src/test/to-dom.js index 9c7559c9c7f560..907026e9b78152 100644 --- a/packages/rich-text/src/test/to-dom.js +++ b/packages/rich-text/src/test/to-dom.js @@ -61,8 +61,26 @@ describe( 'applyValue', () => { }, { current: 'test1 test2 test3', + future: 'test1 test2 test3', + movedCount: 1, + description: 'should remove attributes', + }, + { + current: 'test1 test2 test3', future: 'test1 test2 test3', movedCount: 1, + description: 'should add attributes', + }, + { + current: 'test1 test2 test3', + future: 'test1 test2 test3', + movedCount: 1, + description: 'should update attributes', + }, + { + current: 'test1 test2 test3', + future: 'test1 test2 test3', + movedCount: 1, description: 'should apply attributes', }, ]; @@ -76,7 +94,32 @@ describe( 'applyValue', () => { const count = childNodes.reduce( ( acc, { parentNode } ) => { return parentNode === body ? acc + 1 : acc; }, 0 ); - expect( body.innerHTML ).toEqual( future ); + const sortNodeAttributes = function ( node ) { + if ( ! node.attributes ) { + return; + } + const keys = [], values = {}; + for ( let i = node.attributes.length; --i >= 0; ) { + const name = node.attributes[ i ].name; + keys.push( name ); + values[ name ] = node.attributes[ i ].value; + node.removeAttribute( name ); + } + keys.sort( function ( a, b ) { + return a.localeCompare( b ); + } ); + keys.forEach( function ( key ) { + node.setAttribute( key, values[ key ] ); + } ); + }; + body.childNodes.forEach( function ( node ) { + sortNodeAttributes( node ); + } ); + const attributesSortedFutureBody = createElement( document, future ).cloneNode( true ); + attributesSortedFutureBody.childNodes.forEach( function ( node ) { + sortNodeAttributes( node ); + } ); + expect( attributesSortedFutureBody.innerHTML ).toEqual( body.innerHTML ); expect( count ).toEqual( movedCount ); } ); } ); From 462b2c7c080aefd5eefff3369b6536a737be9e39 Mon Sep 17 00:00:00 2001 From: Technote Date: Fri, 19 Apr 2019 21:50:29 +0900 Subject: [PATCH 3/7] fix lint error --- packages/rich-text/src/test/to-dom.js | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/rich-text/src/test/to-dom.js b/packages/rich-text/src/test/to-dom.js index 907026e9b78152..055cacb8c96021 100644 --- a/packages/rich-text/src/test/to-dom.js +++ b/packages/rich-text/src/test/to-dom.js @@ -94,29 +94,30 @@ describe( 'applyValue', () => { const count = childNodes.reduce( ( acc, { parentNode } ) => { return parentNode === body ? acc + 1 : acc; }, 0 ); - const sortNodeAttributes = function ( node ) { + const sortNodeAttributes = function( node ) { if ( ! node.attributes ) { return; } - const keys = [], values = {}; + const keys = []; + const values = {}; for ( let i = node.attributes.length; --i >= 0; ) { const name = node.attributes[ i ].name; keys.push( name ); values[ name ] = node.attributes[ i ].value; node.removeAttribute( name ); } - keys.sort( function ( a, b ) { + keys.sort( function( a, b ) { return a.localeCompare( b ); } ); - keys.forEach( function ( key ) { + keys.forEach( function( key ) { node.setAttribute( key, values[ key ] ); } ); }; - body.childNodes.forEach( function ( node ) { + body.childNodes.forEach( function( node ) { sortNodeAttributes( node ); } ); const attributesSortedFutureBody = createElement( document, future ).cloneNode( true ); - attributesSortedFutureBody.childNodes.forEach( function ( node ) { + attributesSortedFutureBody.childNodes.forEach( function( node ) { sortNodeAttributes( node ); } ); expect( attributesSortedFutureBody.innerHTML ).toEqual( body.innerHTML ); From 6afe2ace5124d5ddf7e0a764cf44248eb648f34a Mon Sep 17 00:00:00 2001 From: Technote Date: Fri, 19 Apr 2019 22:53:15 +0900 Subject: [PATCH 4/7] revert [sort attributes before compare], add test case parameter [expected] (WordPress/gutenberg#14858) --- packages/rich-text/src/test/to-dom.js | 31 +++------------------------ 1 file changed, 3 insertions(+), 28 deletions(-) diff --git a/packages/rich-text/src/test/to-dom.js b/packages/rich-text/src/test/to-dom.js index 055cacb8c96021..3d7819c89e5ba2 100644 --- a/packages/rich-text/src/test/to-dom.js +++ b/packages/rich-text/src/test/to-dom.js @@ -82,10 +82,11 @@ describe( 'applyValue', () => { future: 'test1 test2 test3', movedCount: 1, description: 'should apply attributes', + expected: 'test1 test2 test3', }, ]; - cases.forEach( ( { current, future, description, movedCount } ) => { + cases.forEach( ( { current, future, description, movedCount, expected } ) => { it( description, () => { const body = createElement( document, current ).cloneNode( true ); const futureBody = createElement( document, future ).cloneNode( true ); @@ -94,33 +95,7 @@ describe( 'applyValue', () => { const count = childNodes.reduce( ( acc, { parentNode } ) => { return parentNode === body ? acc + 1 : acc; }, 0 ); - const sortNodeAttributes = function( node ) { - if ( ! node.attributes ) { - return; - } - const keys = []; - const values = {}; - for ( let i = node.attributes.length; --i >= 0; ) { - const name = node.attributes[ i ].name; - keys.push( name ); - values[ name ] = node.attributes[ i ].value; - node.removeAttribute( name ); - } - keys.sort( function( a, b ) { - return a.localeCompare( b ); - } ); - keys.forEach( function( key ) { - node.setAttribute( key, values[ key ] ); - } ); - }; - body.childNodes.forEach( function( node ) { - sortNodeAttributes( node ); - } ); - const attributesSortedFutureBody = createElement( document, future ).cloneNode( true ); - attributesSortedFutureBody.childNodes.forEach( function( node ) { - sortNodeAttributes( node ); - } ); - expect( attributesSortedFutureBody.innerHTML ).toEqual( body.innerHTML ); + expect( undefined === expected ? future : expected ).toEqual( body.innerHTML ); expect( count ).toEqual( movedCount ); } ); } ); From 639edfca32ebacbb4f3d847bf58a4fb2a6821e37 Mon Sep 17 00:00:00 2001 From: Technote Date: Sat, 20 Apr 2019 01:09:01 +0900 Subject: [PATCH 5/7] trivial change --- packages/rich-text/src/test/to-dom.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/rich-text/src/test/to-dom.js b/packages/rich-text/src/test/to-dom.js index 3d7819c89e5ba2..fa1dfaff79d2be 100644 --- a/packages/rich-text/src/test/to-dom.js +++ b/packages/rich-text/src/test/to-dom.js @@ -95,7 +95,7 @@ describe( 'applyValue', () => { const count = childNodes.reduce( ( acc, { parentNode } ) => { return parentNode === body ? acc + 1 : acc; }, 0 ); - expect( undefined === expected ? future : expected ).toEqual( body.innerHTML ); + expect( body.innerHTML ).toEqual( undefined === expected ? future : expected ); expect( count ).toEqual( movedCount ); } ); } ); From 36841c61f258c2d2bfbf4541bc27c9b65899ac73 Mon Sep 17 00:00:00 2001 From: iseulde Date: Wed, 24 Apr 2019 14:58:13 +0200 Subject: [PATCH 6/7] Simplify --- packages/rich-text/src/test/to-dom.js | 41 +++++++++++++++++---------- packages/rich-text/src/to-dom.js | 6 +++- 2 files changed, 31 insertions(+), 16 deletions(-) diff --git a/packages/rich-text/src/test/to-dom.js b/packages/rich-text/src/test/to-dom.js index fa1dfaff79d2be..5d3a9581b9a8ee 100644 --- a/packages/rich-text/src/test/to-dom.js +++ b/packages/rich-text/src/test/to-dom.js @@ -60,29 +60,40 @@ describe( 'applyValue', () => { description: 'should not modify', }, { - current: 'test1 test2 test3', - future: 'test1 test2 test3', - movedCount: 1, + current: 'b', + future: 'b', + movedCount: 0, + description: 'should remove attribute', + }, + { + current: 'b', + future: 'b', + movedCount: 0, description: 'should remove attributes', }, { - current: 'test1 test2 test3', - future: 'test1 test2 test3', - movedCount: 1, + current: 'a', + future: 'c', + movedCount: 0, + description: 'should add attribute', + }, + { + current: 'a', + future: 'c', + movedCount: 0, description: 'should add attributes', }, { - current: 'test1 test2 test3', - future: 'test1 test2 test3', - movedCount: 1, - description: 'should update attributes', + current: 'a', + future: 'a', + movedCount: 0, + description: 'should update attribute', }, { - current: 'test1 test2 test3', - future: 'test1 test2 test3', - movedCount: 1, - description: 'should apply attributes', - expected: 'test1 test2 test3', + current: 'a', + future: 'a', + movedCount: 0, + description: 'should update attributes', }, ]; diff --git a/packages/rich-text/src/to-dom.js b/packages/rich-text/src/to-dom.js index 96f8fe72bf69c1..d99cdd055feda0 100644 --- a/packages/rich-text/src/to-dom.js +++ b/packages/rich-text/src/to-dom.js @@ -207,7 +207,11 @@ export function applyValue( future, current ) { const futureAttributes = futureChild.attributes; if ( currentAttributes ) { - for ( let ii = currentAttributes.length; --ii >= 0; ) { + let ii = currentAttributes.length; + + // Reverse loop because `removeAttribute` on `currentChild` + // changes `currentAttributes`. + while ( ii-- ) { const { name } = currentAttributes[ ii ]; if ( ! futureChild.getAttribute( name ) ) { From c9ae393e1c7787bc1f0c09f67986a80523eba63b Mon Sep 17 00:00:00 2001 From: iseulde Date: Wed, 24 Apr 2019 15:03:12 +0200 Subject: [PATCH 7/7] Remove redundant expected parameter --- packages/rich-text/src/test/to-dom.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/rich-text/src/test/to-dom.js b/packages/rich-text/src/test/to-dom.js index 5d3a9581b9a8ee..2036ba24b80f7a 100644 --- a/packages/rich-text/src/test/to-dom.js +++ b/packages/rich-text/src/test/to-dom.js @@ -97,7 +97,7 @@ describe( 'applyValue', () => { }, ]; - cases.forEach( ( { current, future, description, movedCount, expected } ) => { + cases.forEach( ( { current, future, description, movedCount } ) => { it( description, () => { const body = createElement( document, current ).cloneNode( true ); const futureBody = createElement( document, future ).cloneNode( true ); @@ -106,7 +106,7 @@ describe( 'applyValue', () => { const count = childNodes.reduce( ( acc, { parentNode } ) => { return parentNode === body ? acc + 1 : acc; }, 0 ); - expect( body.innerHTML ).toEqual( undefined === expected ? future : expected ); + expect( body.innerHTML ).toEqual( future ); expect( count ).toEqual( movedCount ); } ); } );