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.

Sign up for GitHub

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
Merged

Support Prepare RichText tree #11595

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
dfbca5f
WIP
ellatrix Nov 7, 2018
6612db8
Render editable tree as default value
ellatrix Nov 7, 2018
614ceb4
withPrepareEditableTree HoC
ellatrix Nov 8, 2018
604c1e0
Internalise filter
ellatrix Nov 8, 2018
880123b
Remove hook on deregister
ellatrix Nov 8, 2018
759c6a2
prefix props passed
ellatrix Nov 8, 2018
ca8e50e
Remove example
ellatrix Nov 8, 2018
61b0e95
Fix typo
ellatrix Nov 8, 2018
ca71e2f
update snapshots
ellatrix Nov 8, 2018
88740cd
Remove test for edit function
ellatrix Nov 8, 2018
447d63a
restore _withoutApply
ellatrix Nov 8, 2018
aa6f609
Update quote snapshot
ellatrix Nov 8, 2018
89539fd
Support passing a context variable to the custom format prepareEditTr…
youknowriad Nov 9, 2018
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: 5 additions & 3 deletions packages/block-library/src/list/test/__snapshots__/index.js.snap
View file
Open in desktop
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
8 changes: 5 additions & 3 deletions packages/block-library/src/pullquote/test/__snapshots__/index.js.snap
View file
Open in desktop
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
8 changes: 5 additions & 3 deletions packages/block-library/src/quote/test/__snapshots__/index.js.snap
View file
Open in desktop
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
42 changes: 36 additions & 6 deletions packages/editor/src/components/rich-text/index.js
View file
Open in desktop
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 @@ -973,6 +1002,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
View file
Open in desktop
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
View file
Open in desktop
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
34 changes: 21 additions & 13 deletions packages/rich-text/src/register-format-type.js
View file
Open in desktop
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,24 @@ export function registerFormatType( name, settings ) {

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

if (
settings.__experimentalCreatePrepareEditableTree &&
settings.__experimentalGetPropsForEditableTreePreparation
) {
addFilter( 'experimentalRichText', name, ( OriginalComponent ) => {
return withSelect( ( sel ) => ( {
[ `format_${ name }` ]: settings.__experimentalGetPropsForEditableTreePreparation( sel ),
Copy link
Member

@gziolo gziolo Nov 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I think you can simplify it to:

return withSelect( ( select, ownProps ) => ( {
	prepareEditableTree: [
		...ownProps.prepareEditableTree,
		settings.__experimentalCreatePrepareEditableTree( settings.__experimentalGetPropsForEditableTreePreparation( select ) ),
	],
} ) )( OriginalComponent );

Updated: I thought settings.__experimentalGetPropsForEditableTreePreparation returns a boolean value.

It still has this issue that it will create a new reference each time props change.

Copy link
Member

Choose a reason for hiding this comment

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

☝️ this way you will have only one wrapping HOC instead of two.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried this, it doesn't behave exactly the same.

} ) )( ( 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 }` ] ),
] }
/>
) );
} );
}

return settings;
}
18 changes: 0 additions & 18 deletions packages/rich-text/src/test/register-format-type.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,24 +94,6 @@ describe( 'registerFormatType', () => {
expect( duplicateFormat ).toBeUndefined();
} );

it( 'should error on undefined edit property', () => {
const format = registerFormatType( 'plugin/test', {
...validSettings,
edit: undefined,
} );
expect( console ).toHaveErroredWith( 'The "edit" property must be specified and must be a valid function.' );
expect( format ).toBeUndefined();
} );

it( 'should reject formats with an invalid edit function', () => {
const format = registerFormatType( validName, {
...validSettings,
edit: 'not-a-function',
} );
expect( console ).toHaveErroredWith( 'The "edit" property must be specified and must be a valid function.' );
expect( format ).toBeUndefined();
} );

it( 'should reject formats without tag name', () => {
const settings = { ...validSettings };
delete settings.tagName;
Expand Down
14 changes: 13 additions & 1 deletion packages/rich-text/src/to-dom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,17 +132,27 @@ function padEmptyLines( { element, createLinePadding, multilineWrapperTags } ) {
}
}

function prepareFormats( prepareEditableTree = [], value ) {
return prepareEditableTree.reduce( ( accumlator, fn ) => {
return fn( accumlator, value.text );
}, value.formats );
}

export function toDom( {
value,
multilineTag,
multilineWrapperTags,
createLinePadding,
prepareEditableTree,
} ) {
let startPath = [];
let endPath = [];

const tree = toTree( {
value,
value: {
...value,
formats: prepareFormats( prepareEditableTree, value ),
},
multilineTag,
multilineWrapperTags,
createEmpty,
Expand Down Expand Up @@ -188,13 +198,15 @@ export function apply( {
multilineTag,
multilineWrapperTags,
createLinePadding,
prepareEditableTree,
} ) {
// Construct a new element tree in memory.
const { body, selection } = toDom( {
value,
multilineTag,
multilineWrapperTags,
createLinePadding,
prepareEditableTree,
} );

applyValue( body, current );
Expand Down
8 changes: 8 additions & 0 deletions packages/rich-text/src/unregister-format-type.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* WordPress dependencies
*/
import { select, dispatch } from '@wordpress/data';
import { removeFilter } from '@wordpress/hooks';

/**
* Unregisters a format.
Expand All @@ -21,6 +22,13 @@ export function unregisterFormatType( name ) {
return;
}

if (
oldFormat.__experimentalCreatePrepareEditableTree &&
oldFormat.__experimentalGetPropsForEditableTreePreparation
) {
removeFilter( 'experimentalRichText', name );
}

dispatch( 'core/rich-text' ).removeFormatTypes( name );

return oldFormat;
Expand Down
6 changes: 1 addition & 5 deletions test/e2e/specs/blocks/__snapshots__/quote.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,7 @@ exports[`Quote can be converted to paragraphs and renders a paragraph for the ci
<!-- /wp:paragraph -->"
`;

exports[`Quote can be converted to paragraphs and renders a void paragraph if both the cite and quote are void 1`] = `
"<!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph -->"
`;
exports[`Quote can be converted to paragraphs and renders a void paragraph if both the cite and quote are void 1`] = `""`;
Copy link
Member

Choose a reason for hiding this comment

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

Why did this change?

Copy link
Member

Choose a reason for hiding this comment

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

Why did this change?

Why did this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea tbh. But now this is true, where it wasn't before: "Quote can be converted to paragraphs and renders a void paragraph if both the cite and quote are void".


exports[`Quote can be converted to paragraphs and renders one paragraph block per <p> within quote 1`] = `
"<!-- wp:paragraph -->
Expand Down