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.

Sign up for GitHub

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

Jump to bottom

RichText: only replace range and nodes if different #12547

Merged
merged 8 commits into from Dec 9, 2018
Merged

RichText: only replace range and nodes if different #12547

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
94c4eb2
RichText: only set range if different
ellatrix Dec 3, 2018
11a80b1
Check rangeCount
ellatrix Dec 3, 2018
a8ab4e6
Also compare nodes
ellatrix Dec 4, 2018
1dcd440
Add e2e test
ellatrix Dec 5, 2018
2137124
Simplify
ellatrix Dec 5, 2018
35ea03a
RichText: Document isRangeEqual
aduth Dec 9, 2018
cdddba8
Testing: RichText: Assure subscriber removal
aduth Dec 9, 2018
6e19977
Unsubscribe in page.evaluate
ellatrix Dec 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 25 additions & 11 deletions packages/rich-text/src/to-dom.js
View file
Open in desktop
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 •
edited

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,15 @@ export function applyValue( future, current ) {
}
}

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 +288,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
View file
Open in desktop
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
49 changes: 49 additions & 0 deletions test/e2e/specs/rich-text.test.js
View file
Open in desktop
Expand Up @@ -67,4 +67,53 @@ 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 );

document.addEventListener( 'selectionchange', () => {
// One selection change event is fine.
document.addEventListener( 'selectionchange', () => {
Copy link
Member

Choose a reason for hiding this comment

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

This will be left as a lingering event listener, potentially called in a future test if the page is not reloaded before its assertions are run. Unlikely in practice; though generally a test should never leave any lingering side-effect.

throw new Error( 'Typing should only emit one selection change event.' );
}, { once: true } );
}, { once: true } );
} );

await page.keyboard.type( '4' );

expect( await getEditedPostContent() ).toMatchSnapshot();
} );
} );