Editable: implement TinyMCE on the core Editable component #330

youknowriad · 2017-03-27T10:42:21Z

In this PR, I'm setting up TinyMCE in the Editable component to ease building blocks using this generic component. Details on this component on the Doc PR https://github.com/WordPress/gutenberg/pull/302/files#diff-8973b2df4ecb1bd89ade51059ee90658R157

Notes

I'm reusing the WordPress TinyMCE scripts (v3)
I had to expose the React Component class, I wonder if this should be available for block creators or not.
For now, the editable component is very basic. It takes a value prop and triggers onChange callback on Focus out (to avoid cursor jumping).
With all this .bind calls, I felt the need for the class properties babel plugin.

youknowriad · 2017-03-27T10:49:28Z

@aduth @nylen The tests are failing because The editable component is being loaded and it requires the wp globals.

I personally think we should recreate the registration.js file to isolate the block registration from the other unrelated stuff and this should resolve this issue by not loading the editbale component while testing.
We can also mock the globals in bootstrap-test.js

Thoughts?

mtias · 2017-03-27T13:01:05Z

index.php

@@ -40,8 +40,9 @@ function gutenberg_register_scripts() {
 	wp_register_script( 'react-dom', 'https://unpkg.com/react-dom@15/dist/react-dom' . $suffix . '.js', array( 'react' ) );

 	// Editor
+	wp_register_script( 'tinymce_js', includes_url( 'js/tinymce/' ) . 'wp-tinymce.php', array( 'jquery' ) );


I think we should get the trunk version, which has the link boundaries.

aduth · 2017-03-27T18:34:31Z

this should resolve this issue by not loading the editable component while testing.

Sounds more like a workaround than a proper fix.

aduth · 2017-03-27T18:35:58Z

blocks/components/editable/index.js

+
+export default class Editable extends wp.element.Component {
+	constructor( ...args ) {
+		super( ...args );


Minor: ...arguments works just as well and might (a) more clearly indicate you're not really using the arguments and (b) avoid needing to update the call if ever you decide to use one of the arguments.

aduth · 2017-03-27T18:37:16Z

blocks/components/editable/index.js

+		}
+		this.editor.setContent( this.props.value );
+		if ( this.isFocused ) {
+			this.editor.selection.moveToBookmark( bookmark );


Out of curiosity, does it make any difference if we were to call this while the editor does not have focus?

It can cause the cursor to jump to the beginning of the content editable. I think right now, it does not happen because we're not updating the content from outside the component yet.
I've had some hard time fixing these kind of bugs on the per-block prototype.

Yeah my thinking was if the editor wasn't focused anyways, setting content and moving bookmark wouldn't have any impact on the current active focus. I did some basic testing and this appears to be the case? I was entering this in my console at page load:

var bookmark = tinymce.editors[ 1 ].selection.getBookmark( 2, true ); tinymce.editors[ 1 ].setContent( tinymce.editors[ 0 ].getContent() ); tinymce.editors[ 1 ].selection.moveToBookmark( bookmark );

You're right! Can't remember exactly why I had those on the per-block editor. Removing these checks for now

aduth · 2017-03-27T18:37:32Z

blocks/components/editable/index.js

+
+	componentDidUpdate( prevProps ) {
+		if ( this.props.value !== prevProps.value ) {
+			this.updateContent( !! prevProps.focusConfig );


We're not using the argument passed?

aduth · 2017-03-27T18:37:56Z

blocks/components/editable/index.js

+	render() {
+		return el( 'div', {
+			id: this.id,
+			contentEditable: true


Shouldn't need to assign this, as TinyMCE will do so on our behalf.

aduth · 2017-03-27T18:38:01Z

blocks/components/editable/index.js

+	}
+
+	render() {
+		return el( 'div', {


aduth · 2017-03-27T18:40:09Z

blocks/components/editable/index.js

+
+	onChange() {
+		const value = this.editor.getContent();
+		if ( value === this.props.value ) {


Would isDirty work here?

Alternatively, would specifying format: raw to getContent (much more performant) save us at all, or would we expect most calls to this function to pass the condition anyways?

Using isDirty coupled with save is possible I think 👍
And yeah most calls will pass the condition here, I think

aduth · 2017-03-27T18:41:27Z

editor/blocks/text-block/index.js


 wp.blocks.registerBlock( 'wp/text', {
 	title: 'Text',
 	icon: 'text',

 	attributes: {
-		value: query( 'p', html() )
+		value: html()


Might still be value in specifying 'p' argument to at least limit value to text within the <p> element of the block?

This is a good question. If we limit the value to the text within the p, we should limit the Editable to create a unique p and the onChange callback of the Editable component should extract the content of the p before calling the onChange prop.

This is probably what we want, I was thinking we could leave this to a separate PR.

How does specifying p here affect wpautop() type of stuff?

I think this is unrelated to wpautop.

The wpautop needs to be implemented by catching the Enter key press and checking whether we should create a new block after the current one or not. (Similar behaviour can be seen on the per-block prototype)

Well, sort of. What I was interested in was what is actually saved. If I look at the post content of my current blog, there are a lot of text paragraphs separated by one blank line. There are not actual p tags. Somewhere along the way to display wpautop puts p tags? Will the new editor save the p tag that the old editor did not?

I'm assuming we're talking about the visual editor. Then yes, in its current form this block saves the content with the p tag. A block declares its saved markup in the save function. You can see that the current wp/text block wraps the content with a p tag.

But this is far from being the final version of the text block, it's just here to serve as an example of block while we're building the "surroundings", how we parse the content, how we save it back, how we render the editables etc...

Yes, it's an example, so I would hope you "do it right". If it saves the p to the content (where the old editor did not), does that affect wpautop?
I get that "it's not the final version" answer a lot, but my experience is that those things are then forgotten and harder to change later. I'm just trying to think of consequences down the line, instead of "building this block now".

This is the exact reason why we're trying to build this as a plugin and to tie it to real data (real posts), for now, my answer to this is "I don't know". We'll have to try the editor in the context of real data to figure out these consequences (and fix all the unwanted behaviour).

aduth · 2017-03-27T18:42:22Z

element/index.js

+/**
+ * A base class to create WordPress Components (Refs, state and lifecycle hooks)
+ */
+export const Component = ReactComponent;


Prefixed imports bothering me a bit. We could instead do...

import { Component } from 'react'; // ... export { Component };

What do you think?

I'm ok either way :)

aduth · 2017-03-27T18:44:59Z

blocks/components/editable/index.js

+		this.onFocusIn = this.onFocusIn.bind( this );
+		this.onFocusOut = this.onFocusOut.bind( this );
+		this.id = `tinymce-instance-${ editorInstance }`;
+		editorInstance++;


Doesn't make a whole lot of difference, but we could assign the incremented value as a static property of the class:

export default class Editable extends wp.element.Component { static instance = 1; constructor() { // ... this.id = `tinymce-instance-${ this.constructor.instance }`; this.constructor.instance++; // or: (increment slightly less obvious) this.id = `tinymce-instance-${ this.constructor.instance++ }`; }

This will be removed since I'm updating to Tiny4 which allows us to use the dom node instead of generating a random id :)

nylen · 2017-03-27T21:20:26Z

We can also mock the globals in bootstrap-test.js

This, or mocking the editable component itself, would be my preference. I'd rather avoid structuring the code with a fairly arbitrary division imposed by the test setup.

Also worth thinking about how we will write full-browser tests including the editable component.

youknowriad · 2017-03-28T08:40:50Z

This, or mocking the editable component itself, would be my preference.

Not sure how to do that.

I'd rather avoid structuring the code with a fairly arbitrary division imposed by the test setup.

This is true, but I was thinking we should move the registration API to its own file regardless of the way we fix the test setup

aduth · 2017-03-28T12:40:37Z

bootstrap-test.js

@@ -13,3 +13,8 @@ global.document = require( 'jsdom' ).jsdom( '', {
 } );
 global.window = document.defaultView;
 global.navigator = window.navigator;
+global.wp = {


The issue seems to be that the globals are only defined in the main bundle configuration, whereas the entry point of the tests bundle includes only test files. I might toy with the configuration a bit to see if I can fix it so we don't need to stub these out.

aduth · 2017-03-28T13:12:13Z

Should we remove contenteditable from main wrapper div at this point?

aduth · 2017-03-28T14:07:23Z

In 97ba143 I reconfigured Webpack to ensure that globals from each submodule are exposed for the tests. Unfortunately this means pulling in React again separate from the one we define in the plugin, since in the context of the Node tests it can't pull from the CDN like it does in the browser. We might want to think of a better way to avoid managing two separate copies of these vendor dependencies.

More importantly to avoid potential naming clash with existing plugins. “Nightly” might be more accurately a version description, but this is all temporary anyways.

aduth

Looks good 👍

ellatrix · 2017-03-28T14:21:45Z

blocks/components/editable/index.js

+	}
+
+	initialize() {
+		const config = {


To recreate default contenteditable behaviour, we should probably add browser_spellcheck: true.

spocke · 2017-03-28T15:39:20Z

blocks/components/editable/index.js

+
+	componentWillUnmount() {
+		if ( this.editor ) {
+			this.editor.destroy();


Use this.editor.remove(); destroy is more for memory cleanups on unload of the page not really a problem now days on modern browsers. Destroy is called by remove.

spocke · 2017-03-28T15:41:01Z

blocks/components/editable/index.js

+	onSetup( editor ) {
+		this.editor = editor;
+		editor.on( 'init', this.onInit );
+		editor.on( 'focusout', this.onChange );


focusout will when focus is shifted internally within nested contenteditables. Use "blur" instead or possible 'change' since that is called on every undo level add including 'blur' since that adds an undo level. Might be to frequent though.

Since this is a "controlled" TinyMCE (each time the content prop changes, we update the content), I had issues while using change on the per-block prototype. I haven't tried blur though

youknowriad added Framework Issues related to broader framework topics, especially as it relates to javascript TinyMCE labels Mar 27, 2017

youknowriad self-assigned this Mar 27, 2017

youknowriad requested review from mtias and aduth March 27, 2017 10:42

mtias reviewed Mar 27, 2017

View reviewed changes

mtias changed the title ~~TinyMCE: V1 of the Editable Component initializing TinyMCE~~ Editable: implement TinyMCE on the core Editable component Mar 27, 2017

aduth reviewed Mar 27, 2017

View reviewed changes

youknowriad added 2 commits March 28, 2017 09:48

TinyMCE: V1 of the Editable Component initializing TinyMCE

d7c9bb7

Changes per review

4b7a51a

youknowriad force-pushed the add/setup-tinymce-in-plugin branch from 747babc to 4b7a51a Compare March 28, 2017 08:55

Mock Component to fix unit tests

a6bfbcf

aduth reviewed Mar 28, 2017

View reviewed changes

youknowriad and others added 3 commits March 28, 2017 14:49

Drop the global contenteditable

cf04a33

Remove the isFocused check

f80239f

Expose package globals in test build

97ba143

Rename TinyMCE dependency to clarify nightly

84f31be

More importantly to avoid potential naming clash with existing plugins. “Nightly” might be more accurately a version description, but this is all temporary anyways.

aduth approved these changes Mar 28, 2017

View reviewed changes

ellatrix reviewed Mar 28, 2017

View reviewed changes

Adding native browser spellcheck

b33ed3a

youknowriad merged commit ca4111c into master Mar 28, 2017

youknowriad deleted the add/setup-tinymce-in-plugin branch March 28, 2017 14:38

aduth mentioned this pull request Mar 28, 2017

Editor Chrome: Adding the editor mode switcher #339

Merged

spocke reviewed Mar 28, 2017

View reviewed changes

aduth mentioned this pull request Jun 28, 2018

Rich Text: Remove updateContent function #7620

Merged

Verdinjoshua26 mentioned this pull request Nov 27, 2023

[Snyk] Fix for 5 vulnerabilities Verdinjoshua26/gutenberg#93

Open

Editable: implement TinyMCE on the core Editable component #330

Editable: implement TinyMCE on the core Editable component #330

Conversation

youknowriad commented Mar 27, 2017

youknowriad commented Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

aduth commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nylen commented Mar 27, 2017

youknowriad commented Mar 28, 2017

Choose a reason for hiding this comment

aduth commented Mar 28, 2017

aduth commented Mar 28, 2017

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Mar 27, 2017 •

edited

Loading