Editable: Treat the value prop as a string #2786

youknowriad · 2017-09-25T11:42:41Z

fixes #2750

This updates the value of the Editable component to a string.

Pros:

simpler code (check additions and deletions)
no more children and node matchers
slightly easier transformations
No need for the block author to understand a new data structure

We may try a tree structure in a separate PR

Testing instructions

It's easy to miss a use case, the more we test this, the better is

testing using and saving blocks with editables (paragraphs, image captions, quotes, lists...)
try transformations
try splitting/merging
try pasting
try pattern transformations

codecov · 2017-09-25T11:48:42Z

Codecov Report

Merging #2786 into master will decrease coverage by 0.6%.
The diff coverage is 26.74%.

@@            Coverage Diff             @@
##           master    #2786      +/-   ##
==========================================
- Coverage   33.77%   33.16%   -0.61%     
==========================================
  Files         191      191              
  Lines        5691     5681      -10     
  Branches      996      983      -13     
==========================================
- Hits         1922     1884      -38     
- Misses       3189     3225      +36     
+ Partials      580      572       -8

Impacted Files	Coverage Δ
blocks/api/source.js	`100% <ø> (+4.54%)`	⬆️
blocks/library/embed/index.js	`44.94% <ø> (-0.62%)`	⬇️
blocks/editable/tinymce.js	`0% <0%> (ø)`	⬆️
blocks/editable/patterns.js	`1.81% <0%> (ø)`	⬆️
blocks/library/button/index.js	`26.66% <100%> (ø)`	⬆️
blocks/library/table/index.js	`41.66% <100%> (ø)`	⬆️
blocks/library/paragraph/index.js	`33.33% <100%> (ø)`	⬆️
blocks/library/image/index.js	`64.7% <100%> (ø)`	⬆️
blocks/library/verse/index.js	`37.5% <100%> (ø)`	⬆️
blocks/library/preformatted/index.js	`50% <100%> (ø)`	⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff1ef2b...03cd05b. Read the comment docs.

ellatrix · 2017-09-25T11:55:31Z

blocks/library/pullquote/index.js

@@ -95,9 +102,11 @@ registerBlockType( 'core/pullquote', {

 		return (
 			<blockquote className={ `align${ align }` }>
-				{ value && value.map( ( paragraph, i ) => <p key={ i }>{ paragraph.props.children }</p> ) }
+				{ value && value.map( ( paragraph, i ) =>
+					<Editable.Value tagName="p" key={ i }>{ paragraph }</Editable.Value>


With Editable.Value, do you think we can get rid of this map now as well and automatically key?

Would also make more sense as in edit the Editable covers the array, but here in save Editable.Value only covers an item in that array.

notice the comment above // Need a better matcher joining the values, we can't do this right now because we can't write a matcher extracting all the p elements into a single string.

We used to be able to use .join when declaring the source but this is not possible since the source is declarative only. Maybe worth adding a div wrapper, it makes sense to me or come up with a concat matcher: value: 'concat( 'p', prop( 'outerHTML' ) )

Sorry didn't see the comment

Yeah, it's not that obvious that it's related. Anyway, the comment helps to surface the issue here.

Do you get the sense that in most of the cases we'd want a joined set of HTML, we'd be better served by supporting block nesting instead (i.e. should we bother with a matcher for this at all?).

Do you get the sense that in most of the cases we'd want a joined set of HTML, we'd be better served by supporting block nesting instead (i.e. should we bother with a matcher for this at all?).

I don't think so, I think would have the same issue even with nested blocks, the hard part is that we're not able to target (or filter) nodes where we miss some structure (no wrapper around the similar p)

ellatrix · 2017-09-25T12:03:38Z

blocks/editable/index.js

+function nodeListToString( nodeList ) {
+	const container = document.createElement( 'div' );
+	nodeList.forEach( ( node ) => container.appendChild( node ) );
+	return container.innerHTML;


Wonders if Array.from( beforeFragment.childNodes ).map( node => node.innerHTML ).join( '' ) could work.

I guess so, with outerHTML thought. but i think the text nodes don't have the outerHTML property, maybe better keeping this way for consistency. I don't know.

Yep, you're right :)

Fragments... 🙄

aduth

From the perspective of the ideal API to integrate with save, I'm not thrilled by tagName. I think eventually we'd want to avoid that, or Editable.Value altogether, which means creating our own renderToString implementation.

aduth · 2017-09-25T16:18:09Z

blocks/editable/index.js

@@ -652,3 +633,9 @@ export default class Editable extends Component {
 Editable.contextTypes = {
 	onUndo: noop,
 };
+
+Editable.Value = function( { tagName: Tag = 'div', children, ...props } ) {


It's a bit unfortunate we need to surface tagName as a prop, but it makes sense given that we need to assign dangerouslySetInnerHTML as a prop to the element, and can't otherwise return just the raw children. Hmmm!

My ideal:

<Editable.Value>{ content }</Editable.Value>

[...] can't otherwise return just the raw children. [...]

We can't? This worked: https://github.com/WordPress/gutenberg/compare/try/editable-state-tree#diff-c2353ca7ccb0e91040faf47baa55d644R175

It works if it's an array structure but not when it's an HTML string

Ah, right, you need an element to use the dangerous attribute.

aduth · 2017-09-25T16:24:02Z

blocks/editable/index.js

 	}

 	getContent() {
-		return nodeListToReact( this.editor.getBody().childNodes || [], createTinyMCEElement );
+		// TODO: Improve performance using childNodes
+		return this.editor.getContent();


In my head, I've been considering a combination of cloneNode and various DOM querying / manipulation. Something like:

const body = this.editor.getBody().cloneNode( true ); const boguses = body.querySelectorAll( '[data-mce-bogus]' ); for ( var i = 0; i < boguses.length; i++ ) { const bogus = boguses[ i ]; if ( 'all' !== bogus.getAttribute( 'data-mce-bogus' ) { while ( bogus.firstChild ) { bogus.parentNode.insertBefore( bogus.firstChild, bogus ); } } bogus.parentNode.removeChild( bogus ); } const content = body.innerHTML;

Not really sure how the performance compares though.

Yeah, I think we still want to keep getting the raw content and strip what we strip now. I think it might make sense to, just like we do now, loop through all elements and attributes as we need to:

Unwrap elements with the data-mce-bogus attribute.

Remove elements and their children with the data-mce-bogus="all" attribute.

Remove attributes that starts with data-mce-.

I'm not feeling confident with all the TinyMCE's nodes requiring cleanup, I wouldn't mind some help in addressing this

I can have a look.

Added in 0ad5fe1.

It's kind of a lot of code, but it's fast.

aduth · 2017-09-25T16:25:35Z

blocks/editable/index.js

@@ -141,6 +127,9 @@ export default class Editable extends Component {

 	onInit() {
 		this.updateFocus();
+		if ( this.props.value ) {


Will this cause the content to flicker in only after TinyMCE initializes? An advantage of the original implementation is that the content would be displayed even while TinyMCE initializes. Seems like we could recreate this by dangerously assigning the default value as inner HTML, which... should be safe?

I don't see any flickering and I don't think it's a good idea. In fact, this allows us to drop the "shouldComponentUpdate/componentWillReceiveProps" entirely which simplifies the TinyMCE component a lot. see 2eb008dd

The flicker is definitely there, and it's sensible to expect as such since we're not setting the content of the field until TinyMCE finishes initializing.

Master: https://cloudup.com/c3NdeDRaboZ
This branch: https://cloudup.com/cnJth3Fbhjg

Gotcha, thanks for the screenshots. I found a way to fix while avoiding the sCU false
7cd4940

aduth · 2017-09-25T16:25:57Z

blocks/editable/index.js

 export default class Editable extends Component {
 	constructor( props ) {
 		super( ...arguments );

 		const { value } = props;
 		if ( 'production' !== process.env.NODE_ENV && undefined !== value &&
-					! Array.isArray( value ) ) {


With these changes, I think we might be able to just remove this warning altogether.

aduth · 2017-09-25T16:32:11Z

blocks/library/pullquote/index.js

@@ -95,9 +102,11 @@ registerBlockType( 'core/pullquote', {

 		return (
 			<blockquote className={ `align${ align }` }>
-				{ value && value.map( ( paragraph, i ) => <p key={ i }>{ paragraph.props.children }</p> ) }
+				{ value && value.map( ( paragraph, i ) =>
+					<Editable.Value tagName="p" key={ i }>{ paragraph }</Editable.Value>


Do you get the sense that in most of the cases we'd want a joined set of HTML, we'd be better served by supporting block nesting instead (i.e. should we bother with a matcher for this at all?).

aduth · 2017-09-25T16:32:48Z

blocks/library/table/index.js

+			type: 'string',
+			source: html( 'table' ),
+			default: `
+				<tbody>
 					<tr><td><br /></td><td><br /></td></tr>


Aside: Should these be marked as data-mce-bogus br? Seems like they should be ejected from the final markup.

youknowriad · 2017-09-26T09:40:23Z

From the perspective of the ideal API to integrate with save, I'm not thrilled by tagName. I think eventually we'd want to avoid that, or Editable.Value altogether, which means creating our own renderToString implementation.

that's a good point, I also thought about this, but rewriting renderToString means we should have a deep understanding of the React's element object shape for all the different use-cases. It might not be that difficult, but with all these moving parts we have now (thinking of supporting CustomElements and maybe a framework agnostic save and edit), I think we should not try to solve this immediately and wait for things to settle down.

youknowriad · 2017-09-27T09:14:28Z

Can I have a second look here and whether we should merge this in this current shape or not?

ellatrix · 2017-09-27T09:34:57Z

blocks/editable/index.js

@@ -220,8 +200,6 @@ export default class Editable extends Component {

 			if ( this.editor.dom.isEmpty( rootNode ) && this.props.onReplace ) {
 				this.props.onReplace( content );
-			} else {
-				this.splitContent( content );


Why did this go?

I don't know, I recall removing it explicitly for a reason but totally forgot why. I'm bringing it back.

ellatrix · 2017-09-27T09:35:59Z

blocks/editable/index.js

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


Do you think we could drop this method then?

no strong opinion here, but maybe we should keep it to avoid handling the default value in the different places we call this.

Yeah, actually makes sense if we keep getContent too.

ellatrix · 2017-09-27T09:39:25Z

blocks/editable/patterns.js

@@ -189,7 +189,7 @@ export default function( editor ) {
 			return;
 		}

-		const firstText = content[ 0 ];
+		const firstText = editor.getBody().textContent;


Maybe worth investigating if this has any side effects. This is no longer firstText.

Right, not sure why we were only targeting the first text, I tried some patterns, it's working but not sure about all the usecases.

I'll have a look at this too.

ellatrix · 2017-09-27T09:43:11Z

blocks/editable/tinymce.js

-		// breaking React's ability to reconcile changes.
-		//
-		// See: https://github.com/facebook/react/issues/6802
-		return false;


I don't fully understand why this is being changed. I would also feel more comfortable if @aduth had a look at these parts. :)

basically we were putting the defaultValue as a children of the TinyMce Node, so we couldn't allow preact to rerender this node, because its children can be mangled by TinyMCE.

With this new approach, it's simpler, we don't touch the children so we can allow React to rerender and update the attributes (styles, classnames,...) but it won't touch the children.

Preact eh? 😄

ellatrix · 2017-09-27T09:48:41Z

blocks/library/list/index.js

-		} else {
-			listItems[ listItems.length - 1 ].props.children.push( element );
+
+	return content.split( /<br\s*?\/?>/ ).reduce( ( memo, item ) => {


This will be one of the less nice things about this change. Now be can't look inside the content that easily... I don't feel comfortable with a regex to parse this, it will lead to bugs. What if there's nested  s? Maybe we should convert this to DOM (like we do with paste) and split on them there.

we can, I was also thinking of removing this transform entirely, it doesn't make sense to me. It made sense a while back because we were adding br's by default when hitting enter, it's not the case anymore.

I think it still makes sense as we still do allow content like that. I believe it was also a request with the old editor. It's also nice that it stays together when you convert it to text.

@youknowriad I'll adjust it

ellatrix · 2017-09-27T09:49:48Z

blocks/library/list/index.js

-				if ( liIndex !== listItems.length - 1 ) {
-					content.push( createElement( 'br' ) );
-				}
+	const { TEXT_NODE } = window.Node;


Could we keep browser dependencies at the top?

ellatrix · 2017-09-27T09:52:13Z

blocks/library/list/index.js

+			if ( 'UL' === element.nodeName || 'OL' === element.nodeName ) { // lists within lists
+				element.childNodes.forEach( appendLiToContent );
+			} else if ( element.nodeType === TEXT_NODE ) {
+				content.push( element.nodeValue );


Would it make this code simpler if you push the elements instead of the HTML, and get innerHTML at the end?

no strong opinions, I guess it doesn't change that much.

youknowriad · 2017-09-28T10:25:24Z

This is looking good to me, thanks @iseulde for the updates. The question now is whether we want to ship this or wait until we try the array structure approach.

ellatrix · 2017-09-28T11:54:21Z

I think we should just go for it then? I would still prefer to see an array in the state, but we can look into it when we actually need it.

…nd node matchers

…value

… Value

… at the top

youknowriad · 2017-09-29T10:38:16Z

Thoughts on merging this @aduth?

youknowriad · 2017-09-29T10:41:15Z

This is a breaking change for block authors, If we go this road, we should make sure to document this in the Release (or somewhere else)

aduth

Evaluating success here as simplifying the experience for a block implementer while not compromising availability of a rich data structure, I see mixed results. Surely, dropping children and node is a huge win to reducing the barrier to entry for block implementers, but <Editable.Value> is fairly awkward in its current usage. Rather than proving that rich data is still easily available, we've opted for RegEx splits on HTML, which is something we'd very much want to discourage. Losing the data structure prevents us from being able to easily assign React children (both in Editable.Value and Editable's initial rendering). And we still seem to have need for some distinction to assume the role of node (outerHTML?). The simplified implementation of Editable is nice, but relatively speaking not an important consideration.

How do we expect to solve some of the above problems?

Do we provide tools to convert HTML back into a rich data structure? Is this the wrong direction to be going at it?
- Granting this has been raised before, but it's not clear how we achieve "rich" data as the default without a separate matcher
How can we resolve some of the issues with assigning HTML as content of React (safely)?
- At least toward a goal of <Editable.Value>{ content }</Editable.Value>

A use case for rich data was described at #2750 (comment).

aduth · 2017-09-29T13:37:14Z

blocks/library/table/index.js

+			default: `
+				<tbody>
+					<tr><td></td><td></td></tr>
+					<tr><td></td><td></td></tr>
 				</tbody>,


This comma is showing verbatim when inserting a new table.

youknowriad · 2017-09-29T14:12:41Z

Rather than proving that rich data is still easily available, we've opted for RegEx splits on HTML

Ella had commits dropping the regex, I accidentally removed them by force pushing. I'm asking Ella to bring them back since she probably still have them locally.

And we still seem to have need for some distinction to assume the role of node (outerHTML?)

Didn't see any use-case where the html matcher was not sufficient. (innerHTML)

Do we provide tools to convert HTML back into a rich data structure? Is this the wrong direction to be going at it?

The tools are already Available IMO aka DOM. Relying on a standard (DOM) for structure is better than relying on a framework's structure (React elements). Is it better than relying on a custom simplified array structure?

maybe not if we'd want server-side manipulation too, but yes if it's only client side.

How can we resolve some of the issues with assigning HTML as content of React (safely)?

I guess this is the tradeoff in this approach. I don't think reimplementing renderToString thus having a dependency to something internal to React is a good thing to do.

Honestly, I don't know if this is a good solution but don't see in the array structure a far better option.

ellatrix · 2017-09-29T17:38:50Z

I'll bring back the commits and have a good look at this PR again. Honestly I don't mind it as much, as long as we're willing to consider other structures in the future that could benefit logic within editable, global features that touch it, and plugins extending it. I'm still quite curious to see opinions about editable state as a string with meta data.

dmsnell · 2017-09-30T23:36:18Z

After some discussion with @aduth I came to question why we don't use <![CDATA[…]]> sections more in HTML to prevent the kinds of parse errors we frequently get (such as only apply filters to plain text) but then sadly I also discovered it's deprecated

Would have been a cool idea to use this in the HTML string form to simplify the parser to know the difference between text and structure.

I'm still having trouble seeing how this is a good idea for the project. The more DOM-parsing which is required to understand a document, the more burden we place on the editors and the more we allow for ambiguity (or at least implementation differences) in the parse. It's nice having a plain text string with regions and attributes indicating modifications to them.

ellatrix · 2017-10-02T12:14:17Z

What's the main reason we want to change the structure right now, before we can figure out the right structure (HTML/Array/String + meta) based on needs and use cases for each (like footnotes, suggestions)? To get the React objects out of the state? In that case we could do it as arrays for now, which is only a small change. a85bfe4 With the introduction of Editable.Value it also becomes easier for us to change later as we don't have to update docs and nothing would break. (Edit: okay, that's not entirely true, for state as HTML, Editable.Value would be different.)

youknowriad · 2017-10-12T09:35:37Z

I'm closing this PR right now, I guess the way to go is opening a PR with a85bfe4 ( cc @iseulde )?

gziolo · 2017-10-13T06:15:30Z

Can you ping me when this PR gets opened? 😃 I will be happy to test together with changes in coediting PR.

youknowriad added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label Sep 25, 2017

youknowriad self-assigned this Sep 25, 2017

youknowriad requested review from aduth, ellatrix and dmsnell September 25, 2017 11:42

youknowriad force-pushed the update/editable-value-string branch from 1927123 to ca06fc6 Compare September 25, 2017 11:44

ellatrix reviewed Sep 25, 2017

View reviewed changes

aduth reviewed Sep 25, 2017

View reviewed changes

youknowriad force-pushed the update/editable-value-string branch from ede9552 to 02c81f9 Compare September 26, 2017 09:35

ellatrix reviewed Sep 27, 2017

View reviewed changes

ellatrix force-pushed the update/editable-value-string branch 2 times, most recently from 43c2fea to 0ad5fe1 Compare September 27, 2017 11:38

youknowriad added 6 commits September 29, 2017 11:31

Editable: Treat the value prop as a string

6eab9f2

Blocks: Treat the Editable Value as string by removing the children a…

1f35528

…nd node matchers

Blocks: Fix transforms to match the string structure of the Editable …

373c656

…value

Editable as string: Fix the patterns transforms

b31f610

Test: fix the unit tests after the editable format refactoring

36a0239

Documentation: Update the docs to reflect the changes to the Editable…

8c2e0f7

… Value

youknowriad added 6 commits September 29, 2017 11:32

Editable: Fix patterns

b7e0875

Editable: remove prop validation

45277ba

Editable: Simplify the TinyMCE component by allowing rerendering

4fb0473

Table Block: Drop br's from the default table block's html

bcf1d46

Editable as string: Bring back pasting in the middle of a block

0fec6c5

Editable as string: Style changes, Rename a variable and move another…

70cc732

… at the top

youknowriad force-pushed the update/editable-value-string branch from 0ad5fe1 to 70cc732 Compare September 29, 2017 10:35

Editable: Fix flickering effect on init

7cd4940

aduth reviewed Sep 29, 2017

View reviewed changes

ellatrix added 2 commits September 29, 2017 23:00

Correct br ↔ li conversions (parse with browser)

9c1fee8

Editable: getContent: filter raw content

03cd05b

youknowriad closed this Oct 12, 2017

ellatrix mentioned this pull request Oct 18, 2017

Editable: state as a cleaned up tree #3048

Closed

gziolo deleted the update/editable-value-string branch October 18, 2017 10:55

Editable: Treat the value prop as a string #2786

Editable: Treat the value prop as a string #2786

Conversation

youknowriad commented Sep 25, 2017 • edited Loading

codecov bot commented Sep 25, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Sep 25, 2017 • edited Loading

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

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

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

ellatrix Sep 27, 2017 • edited Loading

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

youknowriad commented Sep 26, 2017

youknowriad commented Sep 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

youknowriad commented Sep 28, 2017

ellatrix commented Sep 28, 2017

youknowriad commented Sep 29, 2017

youknowriad commented Sep 29, 2017

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Sep 29, 2017

ellatrix commented Sep 29, 2017

dmsnell commented Sep 30, 2017

ellatrix commented Oct 2, 2017 • edited Loading

youknowriad commented Oct 12, 2017

gziolo commented Oct 13, 2017

youknowriad commented Sep 25, 2017 •

edited

Loading

codecov bot commented Sep 25, 2017 •

edited

Loading

youknowriad Sep 25, 2017 •

edited

Loading

ellatrix Sep 27, 2017 •

edited

Loading

ellatrix commented Oct 2, 2017 •

edited

Loading