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

Support Prepare RichText tree #11595

Merged
merged 13 commits into from
Nov 9, 2018
1 change: 1 addition & 0 deletions packages/block-library/src/heading/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ export default function HeadingEdit( {
</PanelBody>
</InspectorControls>
<RichText
identifier="content"
wrapperClassName="wp-block-heading"
tagName={ tagName }
value={ content }
Expand Down
1 change: 1 addition & 0 deletions packages/block-library/src/list/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,7 @@ export const settings = {
] }
/>
<RichText
identifier="values"
multiline="li"
tagName={ tagName }
unstableGetSettings={ this.getEditorSettings }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,11 @@ exports[`core/list block edit matches snapshot 1`] = `
data-is-placeholder-visible="true"
role="textbox"
>
<br
data-mce-bogus="1"
/>
<li>
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these changes here normal?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it was actually wrong before. We're expecting a padded multiline element.

<br
data-mce-bogus="1"
/>
</li>
</ul>
<ul
class="editor-rich-text__tinymce"
Expand Down
1 change: 1 addition & 0 deletions packages/block-library/src/paragraph/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ class ParagraphBlock extends Component {
</PanelColorSettings>
</InspectorControls>
<RichText
identifier="content"
tagName="p"
className={ classnames( 'wp-block-paragraph', className, {
'has-text-color': textColor.color,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,11 @@ exports[`core/pullquote block edit matches snapshot 1`] = `
data-is-placeholder-visible="true"
role="textbox"
>
<br
data-mce-bogus="1"
/>
<p>
<br
data-mce-bogus="1"
/>
</p>
</div>
<div
class="editor-rich-text__tinymce"
Expand Down
9 changes: 7 additions & 2 deletions packages/block-library/src/quote/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,18 @@ import {
import { join, split, create, toHTMLString } from '@wordpress/rich-text';
import { G, Path, SVG } from '@wordpress/components';

const ATTRIBUTE_QUOTE = 'value';
const ATTRIBUTE_CITATION = 'citation';

const blockAttributes = {
value: {
[ ATTRIBUTE_QUOTE ]: {
type: 'string',
source: 'html',
selector: 'blockquote',
multiline: 'p',
default: '',
},
citation: {
[ ATTRIBUTE_CITATION ]: {
type: 'string',
source: 'html',
selector: 'cite',
Expand Down Expand Up @@ -201,6 +204,7 @@ export const settings = {
</BlockControls>
<blockquote className={ className } style={ { textAlign: align } }>
<RichText
identifier={ ATTRIBUTE_QUOTE }
multiline
value={ value }
onChange={
Expand All @@ -222,6 +226,7 @@ export const settings = {
/>
{ ( ! RichText.isEmpty( citation ) || isSelected ) && (
<RichText
identifier={ ATTRIBUTE_CITATION }
value={ citation }
onChange={
( nextCitation ) => setAttributes( {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,11 @@ exports[`core/quote block edit matches snapshot 1`] = `
data-is-placeholder-visible="true"
role="textbox"
>
<br
data-mce-bogus="1"
/>
<p>
<br
data-mce-bogus="1"
/>
</p>
</div>
<div
class="editor-rich-text__tinymce"
Expand Down
48 changes: 41 additions & 7 deletions packages/editor/src/components/rich-text/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import {
isCollapsed,
} from '@wordpress/rich-text';
import { decodeEntities } from '@wordpress/html-entities';
import { withFilters } from '@wordpress/components';

/**
* Internal dependencies
Expand Down Expand Up @@ -238,6 +239,7 @@ export class RichText extends Component {
unwrapNode: ( node ) => !! node.getAttribute( 'data-mce-bogus' ),
removeAttribute: ( attribute ) => attribute.indexOf( 'data-mce-' ) === 0,
filterString: ( string ) => string.replace( TINYMCE_ZWSP, '' ),
prepareEditableTree: this.props.prepareEditableTree,
} );
}

Expand All @@ -252,6 +254,7 @@ export class RichText extends Component {
element.setAttribute( 'data-mce-bogus', '1' );
return element;
},
prepareEditableTree: this.props.prepareEditableTree,
} );
}

Expand Down Expand Up @@ -413,10 +416,7 @@ export class RichText extends Component {
const record = this.createRecord();
const transformed = this.patterns.reduce( ( accumlator, transform ) => transform( accumlator ), record );

// Don't apply changes if there's no transform. Content will be up to
// date. In the future we could always let it flow back in the live DOM
// if there are no performance issues.
this.onChange( transformed, record === transformed );
this.onChange( transformed );
Copy link
Member

Choose a reason for hiding this comment

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

This worries me that our standard set of operations for a single input have suddenly become more non-performant. Am I wrong to be worried?

Copy link
Member Author

Choose a reason for hiding this comment

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

The only thing that changes the difference between the created DOM tree by us will be applied to the live DOM. With most input, there will be no changes.

}

/**
Expand Down Expand Up @@ -780,6 +780,22 @@ export class RichText extends Component {
record.end = length;
this.applyRecord( record );
}

// If any format props update, reapply value.
const shouldReapply = Object.keys( this.props ).some( ( name ) => {
if ( name.indexOf( 'format_' ) !== 0 ) {
return false;
}

return Object.keys( this.props[ name ] ).some( ( subName ) => {
return this.props[ name ][ subName ] !== prevProps[ name ][ subName ];
} );
} );

if ( shouldReapply ) {
const record = this.formatToValue( value );
this.applyRecord( record );
}
}

formatToValue( value ) {
Expand Down Expand Up @@ -809,6 +825,20 @@ export class RichText extends Component {
return value;
}

valueToEditableHTML( value ) {
return unstableToDom( {
value,
multilineTag: this.multilineTag,
multilineWrapperTags: this.multilineWrapperTags,
createLinePadding( doc ) {
const element = doc.createElement( 'br' );
element.setAttribute( 'data-mce-bogus', '1' );
return element;
},
prepareEditableTree: this.props.prepareEditableTree,
} ).body.innerHTML;
}

valueToFormat( { formats, text } ) {
// Handle deprecated `children` and `node` sources.
if ( this.usedDeprecatedChildrenSource ) {
Expand All @@ -834,7 +864,6 @@ export class RichText extends Component {
const {
tagName: Tagname = 'div',
style,
value,
wrapperClassName,
className,
inlineToolbar = false,
Expand Down Expand Up @@ -883,7 +912,7 @@ export class RichText extends Component {
getSettings={ this.getSettings }
onSetup={ this.onSetup }
style={ style }
defaultValue={ value }
defaultValue={ this.valueToEditableHTML( record ) }
isPlaceholderVisible={ isPlaceholderVisible }
aria-label={ placeholder }
aria-autocomplete="list"
Expand Down Expand Up @@ -930,19 +959,23 @@ const RichTextContainer = compose( [
withBlockEditContext( ( context, ownProps ) => {
// When explicitly set as not selected, do nothing.
if ( ownProps.isSelected === false ) {
return {};
return {
clientId: context.clientId,
};
}
// When explicitly set as selected, use the value stored in the context instead.
if ( ownProps.isSelected === true ) {
return {
isSelected: context.isSelected,
clientId: context.clientId,
};
}

// Ensures that only one RichText component can be focused.
return {
isSelected: context.isSelected && context.focusedElement === ownProps.instanceId,
setFocusedElement: context.setFocusedElement,
clientId: context.clientId,
};
} ),
withSelect( ( select ) => {
Expand Down Expand Up @@ -973,6 +1006,7 @@ const RichTextContainer = compose( [
};
} ),
withSafeTimeout,
withFilters( 'experimentalRichText' ),
] )( RichText );

RichTextContainer.Content = ( { value, tagName: Tag, multiline, ...props } ) => {
Expand Down
28 changes: 1 addition & 27 deletions packages/editor/src/components/rich-text/tinymce.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@ import classnames from 'classnames';
*/
import { Component, createElement } from '@wordpress/element';
import { BACKSPACE, DELETE, ENTER, LEFT, RIGHT } from '@wordpress/keycodes';
import { toHTMLString } from '@wordpress/rich-text';
import { children } from '@wordpress/blocks';

/**
* Internal dependencies
Expand Down Expand Up @@ -313,8 +311,6 @@ export default class TinyMCE extends Component {
isPlaceholderVisible,
onPaste,
onInput,
multilineTag,
multilineWrapperTags,
onKeyDown,
onKeyUp,
} = this.props;
Expand All @@ -332,28 +328,6 @@ export default class TinyMCE extends Component {
// If a default value is provided, render it into the DOM even before
// TinyMCE finishes initializing. This avoids a short delay by allowing
// us to show and focus the content before it's truly ready to edit.
let initialHTML = defaultValue;

// Guard for blocks passing `null` in onSplit callbacks. May be removed
// if onSplit is revised to not pass a `null` value.
if ( defaultValue === null ) {
initialHTML = '';
// Handle deprecated `children` and `node` sources.
} else if ( Array.isArray( defaultValue ) ) {
initialHTML = children.toHTML( defaultValue );
} else if ( typeof defaultValue !== 'string' ) {
initialHTML = toHTMLString( {
value: defaultValue,
multilineTag,
multilineWrapperTags,
} );
}

if ( initialHTML === '' ) {
// Ensure the field is ready to receive focus by TinyMCE.
initialHTML = '<br data-mce-bogus="1">';
}

return createElement( tagName, {
...ariaProps,
className: classnames( className, 'editor-rich-text__tinymce' ),
Expand All @@ -362,7 +336,7 @@ export default class TinyMCE extends Component {
ref: this.bindEditorNode,
style,
suppressContentEditableWarning: true,
dangerouslySetInnerHTML: { __html: initialHTML },
dangerouslySetInnerHTML: { __html: defaultValue },
onPaste,
onInput,
onFocus: this.onFocus,
Expand Down
16 changes: 11 additions & 5 deletions packages/rich-text/src/create.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ function toFormat( { type, attributes } ) {
return attributes ? { type, attributes } : { type };
}

if ( formatType.__experimentalCreatePrepareEditableTree ) {
return null;
}

if ( ! attributes ) {
return { type: formatType.name };
}
Expand Down Expand Up @@ -359,11 +363,13 @@ function createFromElement( {
} ),
} );

// Reuse the last format if it's equal.
if ( isFormatEqual( newFormat, lastFormat ) ) {
format = lastFormat;
} else {
format = newFormat;
if ( newFormat ) {
// Reuse the last format if it's equal.
if ( isFormatEqual( newFormat, lastFormat ) ) {
format = lastFormat;
} else {
format = newFormat;
}
}
}

Expand Down
43 changes: 30 additions & 13 deletions packages/rich-text/src/register-format-type.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,8 @@
/**
* External dependencies
*/
import { isFunction } from 'lodash';

/**
* WordPress dependencies
*/
import { select, dispatch } from '@wordpress/data';
import { select, dispatch, withSelect } from '@wordpress/data';
import { addFilter } from '@wordpress/hooks';

/**
* Registers a new format provided a unique name and an object defining its
Expand Down Expand Up @@ -45,13 +41,6 @@ export function registerFormatType( name, settings ) {
return;
}

if ( ! settings || ! isFunction( settings.edit ) ) {
window.console.error(
'The "edit" property must be specified and must be a valid function.'
);
return;
}

if (
typeof settings.tagName !== 'string' ||
settings.tagName === ''
Expand Down Expand Up @@ -124,5 +113,33 @@ export function registerFormatType( name, settings ) {

dispatch( 'core/rich-text' ).addFormatTypes( settings );

if (
settings.__experimentalCreatePrepareEditableTree &&
settings.__experimentalGetPropsForEditableTreePreparation
) {
addFilter( 'experimentalRichText', name, ( OriginalComponent ) => {
return withSelect( ( sel, { clientId, identifier } ) => ( {
[ `format_${ name }` ]: settings.__experimentalGetPropsForEditableTreePreparation(
sel,
{
richTextIdentifier: identifier,
blockClientId: clientId,
}
),
} ) )( ( props ) => (
<OriginalComponent
{ ...props }
prepareEditableTree={ [
Copy link
Contributor

Choose a reason for hiding this comment

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

We are generating a new array here on each render, I wonder if this is the source of the rerendering happening in all RichText. I wonder if we could memoize it somehow in withSelect by using settings.__experimentalGetPropsForEditableTreePreparation( sel ) as the memoization key, as we don't want to generate a new array if the the returned value didn't change.

Anyway, it doesn't feel unsolvable, so for the sake of iterations, I'm fine leaving this optimisation for a follow-up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could it also be creating a new object in withSelect every time? But sure, sounds resolvable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we create an issue for this one?

...( props.prepareEditableTree || [] ),
settings.__experimentalCreatePrepareEditableTree( props[ `format_${ name }` ], {
richTextIdentifier: props.identifier,
blockClientId: props.clientId,
} ),
] }
/>
) );
} );
}

return settings;
}
Loading