Support Prepare RichText tree by ellatrix · Pull Request #11595 · WordPress/gutenberg

ellatrix · 2018-11-07T16:10:34Z

Description

To test: type "Gutenberg" in a RichText field. The word will be highlighted if it's the first occurrence, and if the "Block" sidebar is open. Now remove a letter, and highlighting will be gone. Add it back to add highlighting and go to the "Document" sidebar. Highlighting will be gone. It depends on the sidebar state.

Example plugin code:

wp.richText.registerFormatType( 'plugin/annotation', {
	title: 'invisible',
	tagName: 'mark',
	className: 'annotation',
	__experimentalGetPropsForEditableTreePreparation( select ) {
		return {
			isEnabled: select( 'core/edit-post' ).getActiveGeneralSidebarName() === 'edit-post/block',
		};
	},
	__experimentalCreatePrepareEditableTree( props ) {
		return ( formats, text ) => {
			if ( ! props.isEnabled ) {
				return formats;
			}

			const search = 'Gutenberg';
			const index = text.indexOf( search );

			if ( index === -1 ) {
				return formats;
			}

			const start = index;
			const end = index + search.length;

			const newValue = wp.richText.applyFormat( { text, formats }, { type: 'plugin/annotation' }, start, end );

			return newValue.formats;
		};
	},
} );

How has this been tested?

Screenshots

Types of changes

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

aduth · 2018-11-07T16:30:58Z

packages/rich-text/src/to-dom.js

+function prepareFormats( value ) {
+	return getFormatTypes().reduce( ( accumlator, { prepareEditableTree } ) => {
+		if ( prepareEditableTree ) {
+			return prepareEditableTree( accumlator, value.text );


Any reason not to pass the entire value?

Yes. We don't want to alter the text at this stage. Otherwise the internal value is wrong. This should be purely for applying formatting on top of text.

We could also pass the entire value and only use the formats from what is returned. Also maybe warn if the length is different.

youknowriad · 2018-11-08T09:37:40Z

This feels like a great start, can we try to polish the PR for merge.

Also, I'm wondering if we should keep prepareEditableTree as experimental API for some time. Thoughts? Like just rename it __experimentalPrepareEditableTree?

packages/format-library/src/invisible/index.js

packages/editor/src/components/rich-text/index.js

youknowriad · 2018-11-08T16:16:52Z

packages/block-library/src/list/test/__snapshots__/index.js.snap

-          <br
-            data-mce-bogus="1"
-          />
+          <li>


Are these changes here normal?

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

youknowriad · 2018-11-08T16:36:01Z

packages/rich-text/src/register-format-type.js

+			} ) )( ( props ) => (
+				<OriginalComponent
+					{ ...props }
+					prepareEditableTree={ [


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.

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

Should we create an issue for this one?

youknowriad

LGTM 👍

Do you want to give it another look @aduth?

ellatrix · 2018-11-08T17:05:03Z

Ideally this is followed up with some tests and a Christmas light investigation.

gziolo · 2018-11-08T17:28:18Z

packages/rich-text/src/register-format-type.js

+	) {
+		addFilter( 'experimentalRichText', name, ( OriginalComponent ) => {
+			return withSelect( ( sel ) => ( {
+				[ `format_${ name }` ]: settings.__experimentalGetPropsForEditableTreePreparation( sel ),


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.

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

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

…eeValue (#11630)

aduth · 2018-11-09T13:43:28Z

test/e2e/specs/blocks/__snapshots__/quote.test.js.snap

-<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`] = `""`;


Why did this change?

Why did this change?

Why did this change?

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".

aduth · 2018-11-09T13:47:17Z

packages/editor/src/components/rich-text/index.js

-		// 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 );


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?

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.

aduth · 2018-11-09T13:47:56Z

Ideally this is followed up with some tests and a Christmas light investigation.

Before 4.3 I'd hope.

aduth reviewed Nov 7, 2018

View reviewed changes

youknowriad added this to the 4.3 milestone Nov 8, 2018

youknowriad mentioned this pull request Nov 8, 2018

Annotation API #7718

Merged

4 tasks

youknowriad reviewed Nov 8, 2018

View reviewed changes

packages/format-library/src/invisible/index.js Outdated Show resolved Hide resolved

ellatrix commented Nov 8, 2018

View reviewed changes

packages/editor/src/components/rich-text/index.js Outdated Show resolved Hide resolved

ellatrix added 10 commits November 8, 2018 15:51

WIP

dfbca5f

Render editable tree as default value

6612db8

withPrepareEditableTree HoC

614ceb4

Internalise filter

604c1e0

Remove hook on deregister

880123b

prefix props passed

759c6a2

Remove example

ca8e50e

Fix typo

61b0e95

update snapshots

ca71e2f

Remove test for edit function

88740cd

ellatrix force-pushed the try/prepare-editable branch from 98bb0f7 to 88740cd Compare November 8, 2018 14:52

restore _withoutApply

447d63a

youknowriad reviewed Nov 8, 2018

View reviewed changes

Update quote snapshot

aa6f609

youknowriad reviewed Nov 8, 2018

View reviewed changes

youknowriad approved these changes Nov 8, 2018

View reviewed changes

gziolo reviewed Nov 8, 2018

View reviewed changes

Support passing a context variable to the custom format prepareEditTr…

89539fd

…eeValue (#11630)

youknowriad merged commit 4741104 into master Nov 9, 2018

youknowriad deleted the try/prepare-editable branch November 9, 2018 08:01

youknowriad changed the title ~~Proof of Concept for e.g. Spellchecker~~ Support Prepare RichText tree Nov 9, 2018

aduth reviewed Nov 9, 2018

View reviewed changes

This was referenced Nov 14, 2018

Fix annotations text tracking #11861

Merged

RichText: own undo signalling #10650

Merged

Writing Flow: Pressing enter while within inline boundary forces caret position to revert #8877

Closed

Fix RichText rerendering when it shouldn't #12161

Merged

atimmer mentioned this pull request Nov 21, 2018

Annotations: Add end-to-end tests for annotations API #12186

Closed

4 tasks

ellatrix mentioned this pull request Mar 6, 2019

RichText: Fix prepareEditableTree #14284

Merged

5 tasks

mhkuu mentioned this pull request Jan 5, 2023

Adds an identifier prop to the RichText component for captions, allowing annotation #46917

Merged

Conversation

ellatrix commented Nov 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad commented Nov 8, 2018

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad left a comment

Choose a reason for hiding this comment

Uh oh!

ellatrix commented Nov 8, 2018

Uh oh!

gziolo Nov 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth commented Nov 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ellatrix commented Nov 7, 2018 •

edited

Loading

gziolo Nov 8, 2018 •

edited

Loading