Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RichText: only replace range and nodes if different #12547

Merged
merged 8 commits into from
Dec 9, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 35 additions & 11 deletions packages/rich-text/src/to-dom.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is future used after this function is complete? I'm wondering if there's any gains to be had by avoiding the removeChild operation and just stepping through the children.

let i;
for ( i = 0; i < future.childNodes.length; i++ ) {
	const futureChild = future.childNodes[ i ];
	const currentChild = current.childNodes[ i ];

	if ( ! currentChild ) {
		current.appendChild( futureChild );
	} else if ( ! currentChild.isEqualNode( futureChild ) ) {
		current.replaceChild( futureChild, currentChild );
	}
}

while ( current.childNodes[ i ] ) {
	current.removeChild( current.childNodes[ i ] );
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work I think. When you append or replace, the node will be gone from future. So either we have to remove unused nodes so that firstChild is accurate, or we copy all the nods to an array. Not sure if it makes much difference. These nodes are not live.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work I think.

But it passes all tests, so it must work? 😄

So either we have to remove unused nodes so that firstChild is accurate, or we copy all the nods to an array.

There might be an option here with iterating backward from the end of future.childNodes as well.

Not sure if it makes much difference. These nodes are not live.

The theory (unconfirmed) is that removeChild is a non-trivial operation, even if not live in a document.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it’s a misconception that the DOM is slow, but I can make test.

Copy link
Member

@aduth aduth Dec 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put together a benchmark. I'm a bit puzzled by how slow the normal iteration is, but it does show removeChild as being sufficiently performant.

https://jsbin.com/bemuhaj/4/edit?html,js,console,output

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the DOM is pretty fast. Just not the live one... :)

}

i++;
Expand All @@ -251,6 +247,25 @@ 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 ) {
ellatrix marked this conversation as resolved.
Show resolved Hide resolved
return (
a.startContainer === b.startContainer &&
ellatrix marked this conversation as resolved.
Show resolved Hide resolved
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 );
Expand Down Expand Up @@ -283,6 +298,15 @@ export function applySelection( selection, current ) {
range.setEnd( endContainer, endOffset );
}

windowSelection.removeAllRanges();
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.addRange( range );
}
6 changes: 6 additions & 0 deletions test/e2e/specs/__snapshots__/rich-text.test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ exports[`RichText should handle change in tag name gracefully 1`] = `
<!-- /wp:heading -->"
`;

exports[`RichText should only mutate text data on input 1`] = `
"<!-- wp:paragraph -->
<p>1<strong>2</strong>34</p>
<!-- /wp:paragraph -->"
`;

exports[`RichText should transform backtick to code 1`] = `
"<!-- wp:paragraph -->
<p>A <code>backtick</code></p>
Expand Down
71 changes: 71 additions & 0 deletions test/e2e/specs/rich-text.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,4 +67,75 @@ 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 || 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 );

window.unsubscribes = [ () => mutationObserver.disconnect() ];

document.addEventListener( 'selectionchange', () => {
function throwMultipleSelectionChange() {
throw new Error( 'Typing should only emit one selection change event.' );
}

document.addEventListener(
'selectionchange',
throwMultipleSelectionChange,
{ once: true }
);

window.unsubscribes.push( () => {
document.removeEventListener( 'selectionchange', throwMultipleSelectionChange );
} );
}, { once: true } );
} );

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();
} );
} );