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

Editable: Treat the value prop as a string #2786

Closed
wants to merge 15 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions blocks/api/paste/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { equal, deepEqual } from 'assert';
import paste from '../index';
import { registerBlockType, unregisterBlockType, setUnknownTypeHandlerName } from '../../registration';
import { createBlock } from '../../factory';
import { children, prop } from '../../source';
import { html, prop } from '../../source';

describe( 'paste', () => {
beforeAll( () => {
Expand All @@ -18,8 +18,8 @@ describe( 'paste', () => {
title: 'test figure',
attributes: {
content: {
type: 'array',
source: children( 'figure' ),
type: 'string',
source: html( 'figure' ),
},
},
transforms: {
Expand Down Expand Up @@ -56,7 +56,7 @@ describe( 'paste', () => {

it( 'should convert recognised pasted content', () => {
const pastedBlock = paste( { HTML: '<figure>test</figure>' } )[ 0 ];
const block = createBlock( 'test/figure', { content: [ 'test' ] } );
const block = createBlock( 'test/figure', { content: 'test' } );

equal( pastedBlock.name, block.name );
deepEqual( pastedBlock.attributes, block.attributes );
Expand Down
6 changes: 3 additions & 3 deletions blocks/api/paste/test/integration/google-docs-out.html
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
<!-- wp:core/paragraph -->
<p>This is a <strong>title</strong><br/></p>
<p>This is a <strong>title</strong><br></p>
<!-- /wp:core/paragraph -->

<!-- wp:core/heading -->
<h2>This is a <em>heading</em></h2>
<!-- /wp:core/heading -->

<!-- wp:core/paragraph -->
<p>This is a <strong>paragraph</strong> with a <a href="https://w.org">link</a>.<br/></p>
<p>This is a <strong>paragraph</strong> with a <a href="https://w.org">link</a>.<br></p>
<!-- /wp:core/paragraph -->

<!-- wp:core/list -->
Expand Down Expand Up @@ -44,7 +44,7 @@ <h2>This is a <em>heading</em></h2>
<!-- /wp:core/list -->

<!-- wp:core/paragraph -->
<p>An image:<br/></p>
<p>An image:<br></p>
<!-- /wp:core/paragraph -->

<!-- wp:core/image -->
Expand Down
20 changes: 10 additions & 10 deletions blocks/api/paste/test/integration/ms-word-online-out.html
Original file line number Diff line number Diff line change
@@ -1,44 +1,44 @@
<!-- wp:core/paragraph -->
<p>This is a <em>heading</em> </p>
<p>This is&nbsp;a <em>heading</em>&nbsp;</p>
<!-- /wp:core/paragraph -->

<!-- wp:core/paragraph -->
<p>This is a <strong>paragraph </strong>with a <a href="https://w.org/">link</a>. </p>
<p>This is a <strong>paragraph </strong>with a <a href="https://w.org/">link</a>.&nbsp;</p>
<!-- /wp:core/paragraph -->

<!-- wp:core/list -->
<ul>
<li>
<p>A </p>
<p>A&nbsp;</p>
</li>
<li>
<p>Bulleted </p>
<p>Bulleted&nbsp;</p>
</li>
<li>
<p>Indented </p>
<p>Indented&nbsp;</p>
</li>
<li>
<p>List </p>
<p>List&nbsp;</p>
</li>
</ul>
<!-- /wp:core/list -->

<!-- wp:core/list -->
<ol>
<li>
<p>One </p>
<p>One&nbsp;</p>
</li>
<li>
<p>Two </p>
<p>Two&nbsp;</p>
</li>
<li>
<p>Three </p>
<p>Three&nbsp;</p>
</li>
</ol>
<!-- /wp:core/list -->

<!-- wp:core/paragraph -->
<p>An image: </p>
<p>An image:&nbsp;</p>
<!-- /wp:core/paragraph -->

<!-- wp:core/image -->
Expand Down
4 changes: 2 additions & 2 deletions blocks/api/paste/test/integration/plain-out.html
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<!-- wp:core/paragraph -->
<p>test<br/>test</p>
<p>test<br>test</p>
<!-- /wp:core/paragraph -->

<!-- wp:core/paragraph -->
<p>test<br/></p>
<p>test<br></p>
<!-- /wp:core/paragraph -->
32 changes: 0 additions & 32 deletions blocks/api/source.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,6 @@
/**
* WordPress dependencies
*/
import { createElement } from '@wordpress/element';

/**
* External dependencies
*/
import { nodeListToReact, nodeToReact } from 'dom-react';
import { flow } from 'lodash';
import {
attr as originalAttr,
Expand Down Expand Up @@ -35,29 +29,3 @@ export const prop = withKnownSourceFlag( originalProp );
export const html = withKnownSourceFlag( originalHtml );
export const text = withKnownSourceFlag( originalText );
export const query = withKnownSourceFlag( originalQuery );
export const children = withKnownSourceFlag( ( selector ) => {
return ( domNode ) => {
let match = domNode;

if ( selector ) {
match = domNode.querySelector( selector );
}

if ( match ) {
return nodeListToReact( match.childNodes || [], createElement );
}

return [];
};
} );
export const node = withKnownSourceFlag( ( selector ) => {
return ( domNode ) => {
let match = domNode;

if ( selector ) {
match = domNode.querySelector( selector );
}

return nodeToReact( match, createElement );
};
} );
44 changes: 0 additions & 44 deletions blocks/api/test/source.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,3 @@
/**
* External dependencies
*/
import { parse } from 'hpq';

/**
* WordPress dependencies
*/
import { renderToString } from '@wordpress/element';

/**
* Internal dependencies
*/
Expand All @@ -19,38 +9,4 @@ describe( 'sources', () => {
expect( sources[ sourceFn ]()._wpBlocksKnownSource ).toBe( true );
}
} );

describe( 'children()', () => {
it( 'should return a source function', () => {
const source = sources.children();

expect( typeof source ).toBe( 'function' );
} );

it( 'should return HTML equivalent WPElement of matched element', () => {
// Assumption here is that we can cleanly convert back and forth
// between a string and WPElement representation
const html = '<blockquote><p>A delicious sundae dessert</p></blockquote>';
const match = parse( html, sources.children() );

expect( renderToString( match ) ).toBe( html );
} );
} );

describe( 'node()', () => {
it( 'should return a source function', () => {
const source = sources.node();

expect( typeof source ).toBe( 'function' );
} );

it( 'should return HTML equivalent WPElement of matched element', () => {
// Assumption here is that we can cleanly convert back and forth
// between a string and WPElement representation
const html = '<blockquote><p>A delicious sundae dessert</p></blockquote>';
const match = parse( html, sources.node() );

expect( renderToString( match ) ).toBe( `<body>${ html }</body>` );
} );
} );
} );
101 changes: 53 additions & 48 deletions blocks/editable/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,20 @@ import classnames from 'classnames';
import {
last,
isEqual,
omitBy,
forEach,
merge,
identity,
find,
defer,
noop,
} from 'lodash';
import { nodeListToReact } from 'dom-react';
import { Fill } from 'react-slot-fill';
import 'element-closest';

/**
* WordPress dependencies
*/
import { createElement, Component, renderToString } from '@wordpress/element';
import { Component } from '@wordpress/element';
import { keycodes } from '@wordpress/utils';

/**
Expand All @@ -36,43 +34,22 @@ import { EVENTS } from './constants';

const { BACKSPACE, DELETE, ENTER } = keycodes;

function createTinyMCEElement( type, props, ...children ) {
if ( props[ 'data-mce-bogus' ] === 'all' ) {
return null;
}

if ( props.hasOwnProperty( 'data-mce-bogus' ) ) {
return children;
}

return createElement(
type,
omitBy( props, ( value, key ) => key.indexOf( 'data-mce-' ) === 0 ),
...children
);
}

function isLinkBoundary( fragment ) {
return fragment.childNodes && fragment.childNodes.length === 1 &&
fragment.childNodes[ 0 ].nodeName === 'A' && fragment.childNodes[ 0 ].text.length === 1 &&
fragment.childNodes[ 0 ].text[ 0 ] === '\uFEFF';
}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, you're right :)

Copy link
Member

Choose a reason for hiding this comment

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

Fragments... 🙄

}

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

const { value } = props;
if ( 'production' !== process.env.NODE_ENV && undefined !== value &&
! Array.isArray( value ) ) {
// eslint-disable-next-line no-console
console.error(
`Invalid value of type ${ typeof value } passed to Editable ` +
'(expected array). Attribute values should be sourced using ' +
'the `children` source when used with Editable.\n\n' +
'See: http://gutenberg-devdoc.surge.sh/reference/attributes/#children'
);
}

this.onInit = this.onInit.bind( this );
this.getSettings = this.getSettings.bind( this );
Expand Down Expand Up @@ -373,8 +350,8 @@ export default class Editable extends Component {
const index = dom.nodeIndex( selectedNode );
const beforeNodes = childNodes.slice( 0, index );
const afterNodes = childNodes.slice( index + 1 );
const beforeElement = nodeListToReact( beforeNodes, createTinyMCEElement );
const afterElement = nodeListToReact( afterNodes, createTinyMCEElement );
const beforeElement = nodeListToString( beforeNodes );
const afterElement = nodeListToString( afterNodes );

this.setContent( beforeElement );
this.props.onSplit( beforeElement, afterElement );
Expand Down Expand Up @@ -413,14 +390,14 @@ export default class Editable extends Component {
const beforeFragment = beforeRange.extractContents();
const afterFragment = afterRange.extractContents();

const beforeElement = nodeListToReact( beforeFragment.childNodes, createTinyMCEElement );
const afterElement = isLinkBoundary( afterFragment ) ? [] : nodeListToReact( afterFragment.childNodes, createTinyMCEElement );
const beforeElement = nodeListToString( beforeFragment.childNodes );
const afterElement = isLinkBoundary( afterFragment ) ? '' : nodeListToString( afterFragment.childNodes );

this.setContent( beforeElement );
this.props.onSplit( beforeElement, afterElement, ...blocks );
} else {
this.setContent( [] );
this.props.onSplit( [], [], ...blocks );
this.setContent( '' );
this.props.onSplit( '', '', ...blocks );
}
}

Expand Down Expand Up @@ -467,8 +444,8 @@ export default class Editable extends Component {
this.setContent( this.props.value );

this.props.onSplit(
nodeListToReact( before, createTinyMCEElement ),
nodeListToReact( after, createTinyMCEElement )
nodeListToString( before ),
nodeListToString( after )
);
}

Expand Down Expand Up @@ -497,16 +474,40 @@ export default class Editable extends Component {
}

setContent( content ) {
if ( ! content ) {
content = '';
}

content = renderToString( content );
this.editor.setContent( content, { format: 'raw' } );
this.editor.setContent( content || '' );
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we could drop this method then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, actually makes sense if we keep getContent too.

}

getContent() {
return nodeListToReact( this.editor.getBody().childNodes || [], createTinyMCEElement );
function filter( nodeList = [] ) {
Array.from( nodeList ).forEach( ( node ) => {
filter( node.children );

if ( node.hasAttribute( 'data-mce-bogus' ) ) {
const parent = node.parentNode;

if ( node.getAttribute( 'data-mce-bogus' ) !== 'all' ) {
while ( node.firstChild ) {
parent.insertBefore( node.firstChild, node );
}
}

parent.removeChild( node );
} else if ( node.hasAttributes() ) {
Array.from( node.attributes ).forEach( ( { name } ) => {
if ( name.indexOf( 'data-mce-' ) === 0 ) {
node.removeAttribute( name );
}
} );
}
} );
}

const clone = this.editor.getBody().cloneNode( true );

// Only loop through element nodes.
filter( clone.children );

return clone.innerHTML;
}

updateFocus() {
Expand Down Expand Up @@ -541,9 +542,7 @@ export default class Editable extends Component {
if (
this.props.tagName === prevProps.tagName &&
this.props.value !== prevProps.value &&
this.props.value !== this.savedContent &&
! isEqual( this.props.value, prevProps.value ) &&
! isEqual( this.props.value, this.savedContent )
this.props.value !== this.savedContent
) {
this.updateContent();
}
Expand Down Expand Up @@ -662,3 +661,9 @@ export default class Editable extends Component {
Editable.contextTypes = {
onUndo: noop,
};

Editable.Value = function( { tagName: Tag = 'div', children, ...props } ) {
Copy link
Member

Choose a reason for hiding this comment

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

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:

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

return (
<Tag { ...props } dangerouslySetInnerHTML={ { __html: children } } />
);
};
Loading