Maintain Editable focus synchronously when splitting blocks #635

aduth · 2017-05-03T18:07:04Z

This pull request seeks to try to refactor Editable with an aim toward managing changes synchronously, and assuming more control of value contents as a known React element (setContent as raw, avoid setContent after initialization).

Doing so, it improves behavior slightly by (a) avoiding focus flickering to the previous block when splitting a text block, and (b) potentially better performance by avoiding unnecessary content setting with TinyMCE's default content validation.

Testing instructions:

Ensure that no regressions occur in content splitting and merging to and from separate text blocks, and that in doing so, focus transfers seamlessly respectively to the beginning and end of the split and merged blocks.

aduth · 2017-05-03T21:17:47Z

The issues I was seeing previously with infinite loops was a result of React trying to reconcile changes applied to the element tree, but doing so after TinyMCE had initialized and made its own modifications. To account for this, I've moved <TinyMCE /> initialization into a separate small component which initially renders the content, then prevents updates, instead assuming that control will be taken over by lifecycle methods of the parent component acting on the editor instance.

This should be ready for review now.

ellatrix · 2017-05-03T22:13:22Z

blocks/components/editable/index.js

@@ -12,6 +12,7 @@ import 'element-closest';
 */
 import './style.scss';
 import FormatToolbar from './format-toolbar';
+import TinyMce from './tinymce';


TinyMCE? :)

TinyMCE? :)

This is another convention I think we'll want to settle: handling abbreviations and acronyms in variable naming. JavaScript / DOM APIs set a pretty inconsistent precedent, with examples like getElementById (camel-cased ID), encodeURIComponent (capitalized URI), and my favorite: XMLHttpRequest (¯\_(ツ)_/¯).

While elsewhere I've settled on forced camel-case, I do find that capitalized abbreviations tend to be more the more common convention, both in the language and in the ecosystem.

I'll plan to change.

ellatrix · 2017-05-03T22:13:51Z

blocks/components/editable/tinymce.js

+		return wp.element.createElement( tagName, {
+			ref: ( node ) => this.editorNode = node,
+			contentEditable: true,
+			suppressContentEditableWarning: true,


Any reason to add, then suppress?

Any reason to add, then suppress?

The suppress is needed because otherwise React will complain about rendering into a contentEditable since similar to the reason for creating this separate component, it will lead to a bad time if reconciliation is attempted.

youknowriad

Seems like switching the heading size is broken too, because we're switching the tagname.

youknowriad · 2017-05-04T09:47:14Z

blocks/components/editable/index.js

@@ -263,11 +233,11 @@ export default class Editable extends wp.element.Component {
 		}

 		content = wp.element.renderToString( content );
-		this.editor.setContent( content );
+		this.editor.setContent( content, { format: 'raw' } );


This breaks merging a paragraph into a heading, because this ignore the content sanitization (due to the custom parent node).

The behaviour of this branch appends a p element at the end of the Heading and thus transforms the heading into a multi-paragraph block. We could solve this by dropping the p container in the merge function, but I feel this validation is important to ensure each block contain only what we want it to contain.

We could solve this by dropping the p container in the merge function, but I feel this validation is important to ensure each block contain only what we want it to contain.

I find it weird that we'd rely on TinyMCE to validate this though. Seems like we shouldn't be returning an invalid structure from the Text -> Heading transform (like <h2>foo<p>text</p></h2>).

youknowriad · 2017-05-04T09:50:49Z

Did you try the "batched subscribe" approach to maintain the focus on merge? Do you think, this is something we could consider?

aduth · 2017-05-04T12:25:28Z

Did you try the "batched subscribe" approach to maintain the focus on merge? Do you think, this is something we could consider?

Can you link to the specific project you were considering? I know I'd looked at one at the time which relied on some unstable React APIs, but there seem to be a handful of different options now that I'm searching again.

For this specific need though, I don't see how batching is necessarily relevant to providing merit on its own, aside from again treating the symptoms of the issues we're experiencing.

Whether batching could provide performance benefits, I think would be fair to consider separately.

aduth · 2017-05-04T12:29:48Z

@youknowriad Since @iseulde has mentioned she wants to make additional refactoring to these same components, I'm going to simply drop c1636d7ea06e684f1216b79bb245aaf1caafe5c0 from the changes for now with plans to address separately, then merge this.

See #635 (comment)

ellatrix · 2017-05-04T13:17:30Z

@aduth Hm, this still breaks heading level switching

aduth · 2017-05-04T13:19:08Z

😞 Thanks for noting. I'll plan to take a look shortly.

aduth · 2017-05-04T14:20:29Z

I think this may also have broken changing to Text mode.

ellatrix · 2017-05-04T14:22:51Z

Hah, I was wondering where that broke. Will have a look too then.

aduth · 2017-05-04T14:24:36Z

Actually, I don't think the styling regressions are the fault of the changes here since I can recreate them checked out to a commit prior to this merge, but there is a new warning about React keys since this was introduced (perhaps due to how we're rendering children in the TinyMCE component).

ellatrix · 2017-05-04T14:26:28Z

Yeah, checked the same

aduth · 2017-05-04T14:30:16Z

Tracked style issues to ee98411 (#603)

ellatrix · 2017-05-04T14:31:29Z

We seem to have array of an array in the quote footer.

ellatrix · 2017-05-04T14:40:41Z

If I change the attribute to query( 'footer > p', children() ) and editable tagName to p it works.

aduth · 2017-05-04T14:42:43Z

Is this cause of dom-react always returning an array? Though not sure why the issue would only now become apparent.

Should dom-react return a single object if there's only a single child?

ellatrix · 2017-05-04T14:49:56Z

Hm, we are giving it a nodeList, not sure why it shouldn't give us back an array.

ellatrix · 2017-05-04T14:51:29Z

If we want to disallow an array of ps, then we should make one p editable? If it's a div with ps, then I would expect an array, even if there's only one.

ellatrix · 2017-05-04T15:05:03Z

Hm and I see

{ value && wp.element.Children.map( value, ( paragraph, i ) => (
	<p key={ i }>{ paragraph }</p>
) ) }

for the paragraph, but just <footer>{ citation }</footer> for the footer.

So either it also needs add keys there, or it just needs to make one p editable. Or get rid of the p.

aduth added TinyMCE [Status] In Progress Tracking issues with work in progress labels May 3, 2017

aduth force-pushed the remove/editable-split-settimeout branch from 6287a61 to 7096b93 Compare May 3, 2017 21:15

aduth removed the [Status] In Progress Tracking issues with work in progress label May 3, 2017

aduth requested review from youknowriad and ellatrix May 3, 2017 21:17

ellatrix reviewed May 3, 2017

View reviewed changes

youknowriad previously requested changes May 4, 2017

View reviewed changes

Avoid focus reverting to first of two split blocks

5b754ec

aduth added 2 commits May 4, 2017 08:31

Assign content as children of root Editable element

2a9ffb6

Move TinyMCE into separate component and prevent rerender

9dc2741

aduth force-pushed the remove/editable-split-settimeout branch from 7096b93 to 9dc2741 Compare May 4, 2017 12:33

aduth merged commit e30e71b into master May 4, 2017

aduth deleted the remove/editable-split-settimeout branch May 4, 2017 12:37

ellatrix added a commit that referenced this pull request May 4, 2017

Fix Editable tagName changes after #635

b2b9349

aduth mentioned this pull request May 4, 2017

Fix Editable tagName changes after #635 #661

Merged

ellatrix added a commit that referenced this pull request May 4, 2017

Fix Editable tagName changes after #635

b8e2ce4

aduth mentioned this pull request May 4, 2017

Use raw format when setting Editable content #667

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maintain Editable focus synchronously when splitting blocks #635

Maintain Editable focus synchronously when splitting blocks #635

aduth commented May 3, 2017 •

edited

Loading

aduth commented May 3, 2017 •

edited

Loading

ellatrix May 3, 2017

aduth May 4, 2017 •

edited

Loading

ellatrix May 3, 2017

aduth May 4, 2017

youknowriad left a comment

youknowriad May 4, 2017

aduth May 4, 2017

youknowriad commented May 4, 2017

aduth commented May 4, 2017

aduth commented May 4, 2017

ellatrix commented May 4, 2017

aduth commented May 4, 2017

aduth commented May 4, 2017

ellatrix commented May 4, 2017

aduth commented May 4, 2017 •

edited

Loading

ellatrix commented May 4, 2017

aduth commented May 4, 2017

ellatrix commented May 4, 2017

ellatrix commented May 4, 2017

aduth commented May 4, 2017

ellatrix commented May 4, 2017

ellatrix commented May 4, 2017

ellatrix commented May 4, 2017 •

edited

Loading

Maintain Editable focus synchronously when splitting blocks #635

Maintain Editable focus synchronously when splitting blocks #635

Conversation

aduth commented May 3, 2017 • edited Loading

aduth commented May 3, 2017 • edited Loading

ellatrix May 3, 2017

Choose a reason for hiding this comment

aduth May 4, 2017 • edited Loading

Choose a reason for hiding this comment

ellatrix May 3, 2017

Choose a reason for hiding this comment

aduth May 4, 2017

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

youknowriad May 4, 2017

Choose a reason for hiding this comment

aduth May 4, 2017

Choose a reason for hiding this comment

youknowriad commented May 4, 2017

aduth commented May 4, 2017

aduth commented May 4, 2017

ellatrix commented May 4, 2017

aduth commented May 4, 2017

aduth commented May 4, 2017

ellatrix commented May 4, 2017

aduth commented May 4, 2017 • edited Loading

ellatrix commented May 4, 2017

aduth commented May 4, 2017

ellatrix commented May 4, 2017

ellatrix commented May 4, 2017

aduth commented May 4, 2017

ellatrix commented May 4, 2017

ellatrix commented May 4, 2017

ellatrix commented May 4, 2017 • edited Loading

aduth commented May 3, 2017 •

edited

Loading

aduth commented May 3, 2017 •

edited

Loading

aduth May 4, 2017 •

edited

Loading

aduth commented May 4, 2017 •

edited

Loading

ellatrix commented May 4, 2017 •

edited

Loading