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

Try: TinyMCE: On-demand initialization #9040

Closed
wants to merge 1 commit into from

Conversation

aduth
Copy link
Member

@aduth aduth commented Aug 15, 2018

Related: #8822
Related: #8879

In-Progress: This is a proof-of-concept and as such, is neither complete as presented nor expected to work for all usage.

This pull request seeks to explore a refactor of the TinyMCE component to initialize itself only when it receives focus, and to destroy itself when focus leaves the field.

This serves a dual purpose:

Implementation notes:

Existing selection is reset when initializing TinyMCE, which broke writing flow behaviors. This is a result of TinyMCE resetting the HTML of the element on initialization. A workaround has been implemented to neutralize editor.dom.setHTML during initialization.

Future considerations may include:

  • Can TinyMCE restore the selection which existed prior to the initialization after initialization completes?
  • Can TinyMCE avoid setting HTML if it's not necessary?
    • Even just a if ( html === node.innerHTML ) return; in editor.dom.setHTML

cc @androb

Known issues:

  • There's an error when adding a new paragraph (startContainer property access)
  • There's an error when clicking the bold button in the toolbar when collapsed selection in a paragraph (isCollapsed)

Observations:

A more thorough performance evaluation should be performed.

Anecdotally, I observed that while there was still significant interaction lag, I was able to convert a post containing 2377 paragraphs to blocks. Previously this conversion would completely stall the tab. See demo content at #8720 (this is certainly an extreme case).

@aduth aduth added [Component] TinyMCE [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Performance Related to performance efforts labels Aug 15, 2018
@youknowriad
Copy link
Contributor

Superseded by #10723?

@aduth
Copy link
Member Author

aduth commented Oct 30, 2018

Mostly, yes. #10723 doesn't include anything for destroying the initialized fields on blur.

@ellatrix
Copy link
Member

Needs a rebase after #10723.

@abotteram
Copy link
Contributor

So we are using a lot of RichText blocks in our HowTo structured data block and we are having massive performance issues when people start adding more than say... 10 steps. I have tested our plugin with this PR and it solves our performance issues. We would really like to see this picked up in the near future.

Yoast/wordpress-seo#11766

@androb
Copy link
Contributor

androb commented Jan 23, 2019

Hi @aduth - sorry I missed this. If it is still an issue and your suggestions for TinyMCE can help, do you mind opening a ticket at https://github.com/tinymce/tinymce/issues

@gziolo
Copy link
Member

gziolo commented Jan 29, 2019

@aduth and @iseulde - is still something that we should update and land? There were several iterations on the TinyMCE initialization so I'm not quite sure how far we got.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

See my comment. I'm also marking as Stale to make triaging easier.

@gziolo gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Jan 29, 2019
@ellatrix
Copy link
Member

We'll soon no longer need it, so in my opinion it's good to close. But if there are substantial memory benefits that are needed in the meantime, it's worth to still look into of course.

@ellatrix
Copy link
Member

ellatrix commented Feb 8, 2019

Superseded by #13697.

@ellatrix ellatrix closed this Feb 8, 2019
@ellatrix ellatrix deleted the try/tinymce-on-demand-init branch February 8, 2019 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants