RichText: Reuse DOM document across calls to createEmpty #12402

aduth · 2018-11-28T22:13:18Z

This pull request seeks to optimize the behavior of RichText's internal createEmpty implementation for DOM tree operations. It reuses a single reference to a DOM document rather than creating a new instance for each invocation.

Implementation notes:

Using a shared reference could potentially have unintended side effects if the reference is held for later use. Given that this function is internal to the module, I think it's an acceptable compromise. I made an attempt to be as clear as possible about this potential gotcha through inline code comments.

As an aside, I noted that this function is called 6 times for every key press in a paragraph. This is something which should be evaluated for future optimization, but is not strictly relevant to the changes proposed here.

Possibly related: #11602

Testing instructions:

Ensure unit tests pass:

npm run test-unit packages/rich-text

Verify there are no regressions in the general behavior of text input in RichText (e.g. paragraph).

ellatrix · 2018-11-29T09:11:01Z

It seems that one e2e test fails consistently... Most likely some other commit causing it.

FAIL test/e2e/specs/adding-inline-tokens.test.js (32.718s)
  ● adding inline tokens › should insert inline image
    waiting for selector ".media-modal li[aria-label="e6a0d94f-6b73-4fad-9f9d-855089c67602"]" failed: timeout 30000ms exceeded
      39 | 
      40 | 		// Wait for upload.
    > 41 | 		await page.waitForSelector( `.media-modal li[aria-label="${ filename }"]` );
         | 		           ^
      42 | 
      43 | 		// Insert the uploaded image.
      44 | 		await page.click( '.media-modal button.media-button-select' );
      at new WaitTask (node_modules/puppeteer/lib/FrameManager.js:840:28)
      at Frame._waitForSelectorOrXPath (node_modules/puppeteer/lib/FrameManager.js:731:12)
      at Frame.waitForSelector (node_modules/puppeteer/lib/FrameManager.js:690:17)
      at Page.waitForSelector (node_modules/puppeteer/lib/Page.js:1008:29)
      at Object.waitForSelector (test/e2e/specs/adding-inline-tokens.test.js:41:14)
      at tryCatch (node_modules/regenerator-runtime/runtime.js:62:40)
      at Generator.invoke [as _invoke] (node_modules/regenerator-runtime/runtime.js:288:22)
      at Generator.prototype.(anonymous function) [as next] (node_modules/regenerator-runtime/runtime.js:114:21)
      at asyncGeneratorStep (test/e2e/specs/adding-inline-tokens.test.js:21:103)
      at _next (test/e2e/specs/adding-inline-tokens.test.js:23:194)

ellatrix

Looks good!

ellatrix · 2018-11-29T09:12:15Z

packages/rich-text/CHANGELOG.md

+
+### Internal
+
+- `unstableToDom` will no longer create a new DOM document on each call; a single document will be shared across each invocation.


I thought we were not documenting unstable APIs?

I hesitated, but ultimately decided that it's good to include something in the CHANGELOG for any pull request.

I wouldn't so much call this out as documenting the unstable API, since a CHANGELOG is not documentation. The "Internal" heading was meant to indicate that it's more a refactoring / housekeeping than anything else.

Maybe I can revise this a bit to avoid specifically mentioning unstableToDom, while keeping the essence of the note otherwise intact.

ellatrix · 2018-11-29T09:13:13Z

packages/rich-text/src/to-dom.js

+	// function being internal to `rich-text` (full control in avoiding a risk
+	// of asynchronous operations on the shared reference), a single document
+	// is reused and reset for each call to the function.
+	if ( createEmpty.body ) {


ellatrix · 2018-11-30T08:59:19Z

As an aside, I noted that this function is called 6 times for every key press in a paragraph. This is something which should be evaluated for future optimization, but is not strictly relevant to the changes proposed here.

@aduth I see this only happening once for every key press?

aduth · 2018-11-30T14:46:33Z

@aduth I see this only happening once for every key press?

Judging by #12460, were you able to reproduce? It appears to be resolved after (and only after) that pull request was merged.

Testing diff:

diff --git a/packages/rich-text/src/to-dom.js b/packages/rich-text/src/to-dom.js
index b582c7708..93e4a61e0 100644
--- a/packages/rich-text/src/to-dom.js
+++ b/packages/rich-text/src/to-dom.js
@@ -60,6 +60,7 @@ function getNodeByPath( node, path ) {
 
 function createEmpty() {
 	const { body } = document.implementation.createHTMLDocument( '' );
+	console.count( 'createEmpty' );
 	return body;
 }

ellatrix · 2018-11-30T15:15:41Z

@aduth Yes. I mistakenly thought this PR was updating createElement in the same package. That function could use the same logic.

ellatrix · 2018-11-30T15:36:41Z

packages/rich-text/src/to-dom.js

+ * the value to operate upon asynchronously, as it may have unexpected results.
+ *
+ * @return {WPRichTextTree} RichText tree.
+ */
 function createEmpty() {


There's also createElement, see #12470. It would be great if we could merge these two functions into one?

Maybe we can update the PR here to also address createElement and only keep one.

aduth · 2018-11-30T15:53:28Z

@iseulde I cherry-picked f0c9aa5 from #12470 into this branch and added revisions of both #12470 (comment) (bdce846) and #12470 (comment) (f9e9e89).

How does it look?

ellatrix

Looks good to me!

aduth added [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Performance Related to performance efforts labels Nov 28, 2018

aduth requested a review from ellatrix November 28, 2018 22:13

aduth added the [Package] Rich text /packages/rich-text label Nov 28, 2018

ellatrix approved these changes Nov 29, 2018

View reviewed changes

ellatrix reviewed Nov 29, 2018

View reviewed changes

ellatrix mentioned this pull request Nov 30, 2018

RichText: Do not run valueToEditableHTML on every render #12460

Merged

4 tasks

aduth added this to the 4.7 milestone Nov 30, 2018

ellatrix mentioned this pull request Nov 30, 2018

RichText: Reuse document in createElement #12470

Closed

4 tasks

ellatrix reviewed Nov 30, 2018

View reviewed changes

RichText: Reuse DOM document across calls to createEmpty

038769c

aduth force-pushed the update/rich-text-reuse-document branch from de614ed to 038769c Compare November 30, 2018 15:37

ellatrix and others added 3 commits November 30, 2018 10:46

RichText: Reuse document in createElement

f0c9aa5

Rich Text: Update tests to clone node to retain reference

bdce846

Rich Text: Refactor createEmpty to use createElement

f9e9e89

ellatrix approved these changes Nov 30, 2018

View reviewed changes

aduth merged commit 4178f2d into master Nov 30, 2018

aduth deleted the update/rich-text-reuse-document branch November 30, 2018 16:35

aduth mentioned this pull request Dec 4, 2018

Performance: Improve the initial loading time and the feeling fo smoothness while typing in blocks. #11782

Closed

aduth mentioned this pull request Jun 17, 2022

Add CSV Download button to reports (LG-5913) GSA-TTS/identity-reporting#57

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RichText: Reuse DOM document across calls to createEmpty #12402

RichText: Reuse DOM document across calls to createEmpty #12402

aduth commented Nov 28, 2018

ellatrix commented Nov 29, 2018

ellatrix left a comment

ellatrix Nov 29, 2018

aduth Nov 29, 2018

aduth Nov 29, 2018

ellatrix Nov 29, 2018

ellatrix commented Nov 30, 2018

aduth commented Nov 30, 2018

ellatrix commented Nov 30, 2018

ellatrix Nov 30, 2018

ellatrix Nov 30, 2018

aduth commented Nov 30, 2018

ellatrix left a comment


		### Internal

		- `unstableToDom` will no longer create a new DOM document on each call; a single document will be shared across each invocation.

RichText: Reuse DOM document across calls to createEmpty #12402

RichText: Reuse DOM document across calls to createEmpty #12402

Conversation

aduth commented Nov 28, 2018

ellatrix commented Nov 29, 2018

ellatrix left a comment

Choose a reason for hiding this comment

ellatrix Nov 29, 2018

Choose a reason for hiding this comment

aduth Nov 29, 2018

Choose a reason for hiding this comment

aduth Nov 29, 2018

Choose a reason for hiding this comment

ellatrix Nov 29, 2018

Choose a reason for hiding this comment

ellatrix commented Nov 30, 2018

aduth commented Nov 30, 2018

ellatrix commented Nov 30, 2018

ellatrix Nov 30, 2018

Choose a reason for hiding this comment

ellatrix Nov 30, 2018

Choose a reason for hiding this comment

aduth commented Nov 30, 2018

ellatrix left a comment

Choose a reason for hiding this comment