Rich Text: Fix Format Type Assignment During Parsing #11488

ellatrix · 2018-11-05T11:17:05Z

Description

Fixes e.g. #6648, generally that no format type can use the same tag name, so e.g. you cannot have two format types handling a or span elements.

Currently format types are assigned based on the provided tagName. This was a temporary solution, intended to be expanded with classes and other attributes, to find a format type match for a given element.

This is a proposal to instead normally serialise a class name. Only in cases where it is wanted that the format type can handle any element with the tag name, you can pass null for the class name.

So say you want to register a custom link format type, you can do so by providing a as the tagName and a className, without clashing with any other format types that want to use the same tag, including the core link format.

Additionally, it enables us to still handle e.g. links if a plugin using the anchor tag gets deactivated.

This is an important change to the Format API that would be good to include before the API even gets shipped, in 4.2, so I'm moving it to the milestone for discussion.

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.

ellatrix · 2018-11-05T23:02:19Z

Changed it so that we use classes instead of data attributes with the format name. @gziolo The only thing I wonder now is, since the class has to be unique, maybe we can just use at at the name/unique ID as well? tagName/className or something like that.

ellatrix · 2018-11-06T00:09:30Z

Test format type:

wp.richText.registerFormatType( 'my-plugin/link', {
	title: 'Custom Link',
	tagName: 'a',
	attributes: {
		url: 'href',
	},
	className: 'my-custum-format',
	edit: function( props ) {
		return wp.element.createElement( props.ToolbarButton, {
			icon: 'editor-strikethrough',
			title: 'Custom Link',
			onClick: function() {
				props.onChange( wp.richText.toggleFormat( props.value, { type: 'my-plugin/link', attributes: {
					url: '#test',
				} } ) );
			},
			isActive: props.isActive,
		} );
	},
} );

youknowriad · 2018-11-06T08:02:14Z

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

-		tagName: 'em',
-	},
+	tagName: 'em',
+	className: null,
 	edit( { isActive, value, onChange, ToolbarButton, Shortcut } ) {


We're receiving components as props? That's really weird, I guess it's out of scope of this PR, but I'm interested in revisiting this pattern.

Yes, we should change it. Maybe even before releasing?

youknowriad · 2018-11-06T13:05:33Z

packages/rich-text/src/create.js

+	let formatType;
+
+	if ( attributes && attributes.class ) {
+		formatType = select( 'core/rich-text' ).getFormatTypeForClassName( attributes.class );


Are these two places here the only places where we rely globally on the data module?
I'd like us to avoid this pattern in the future. And only use the data module in withSelect/withDispatch or provide the registry as an argument somehow (or the available formats in this case)

The issue with this pattern is that it relies on a singleton which is not great if you have the same npm module installed twice for instance.

I'm fine keeping them for now, if we already do the same elsewhere in rich-text.

It got the data the same way before, jut through a wrapper function. Sounds good to revisit separately.

Can't we just change the signature of the function to toFormat({ type, attributes }, availableFormats)? In my testing we do this in this function and in fromFormat. I'd prefer if we can avoid breaking changes here by adding this extra argument, but if you think it's a big refactoring, let's delay.

Where should the available formats be given? in the RichText component? And then we pass it down all the way toHTMLString( { availableFormats } ) etc.?

Well, yes I would prefer this or rewrite all these APIs as selectors. It does feel more impactful that I first thought so you have my 👍 for shipping as is.

This would also make writing tests easier as you don't have to include stores in tests. This issue also exists in other places. I agree that we should keep such usage to the absolute minimum. We can iterate on the codebase later.

Thinking a bit more about it, I think that it might be better to avoid introducing more specialized selectors getFormatTypeForClassName and `getFormatTypeForBareElement. Instead, we can perform filtering using utility helpers in this file. I don't think we will need this logic anywhere else.

youknowriad · 2018-11-06T13:42:17Z

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

+	) {
+		window.console.error(
+			'Format tag names must be a string.'
+		);


Not for this PR: Maybe we could leverage yup at some point for this and block registration, a validation schema seems like a good enhancement.

Yes, we need to improve error handling a lot and built some abstraction on top of it.

gziolo · 2018-11-06T15:16:57Z

We discussed two solutions here. This PR implements:

className - mandatory, provide the name which is string to be included as a class name to identify your format type in HTML. Advanced usage: if there is no bare element handler, you can define your own by setting className value to null.

the other approach we discussed would describe as follows:

canHandleBareElement - optional, defaults to false. You will need to provide className unless value is set to true.
className - optional, provides the name which is string to be included as a class name to identify your format type in HTML when canHandleBareElement is set to false. Omit it otherwise or it will throw a validation error.

youknowriad

LGTM 👍

gziolo · 2018-11-06T16:16:40Z

We should add e2e tests as a follow-up. Examples to look at:

and tests that use those plugins.

Using the provided example:
#11488 (comment)

Basically, you put it into a plugin and add some simple test which verifies it shows up in UI. In this case, it should also verify whether block updates properly.

gziolo · 2018-11-06T16:43:33Z

packages/rich-text/src/create.js

+		formatType = select( 'core/rich-text' ).getFormatTypeForClassName( attributes.class );
+
+		if ( formatType ) {
+			attributes.class = ` ${ attributes.class } `.replace( ` ${ formatType.className } `, ' ' ).trim();


What is happening here, can you explain? It's mostly me trying to understand why we need it. Maybe we should add an inline comment.

We need the preserve any other classes. E.g. a plugin may add extra classes, or if the the plugin doesn't use the class attribute, user added classes should be preserved.

Right, make sense now.

gziolo · 2018-11-06T16:44:16Z

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

+	) {
+		window.console.error(
+			'Format tag names must be a string.'
+		);


Yes, we need to improve error handling a lot and built some abstraction on top of it.

gziolo · 2018-11-06T16:48:00Z

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

+		}
+	} else {
+		const formatTypeForClassName = select( 'core/rich-text' )
+			.getFormatTypeForClassName( settings.className );


In theory, you could allow to reuse the same class name with different tag names. We can relax it later though. Not sure about it.

Yeah, easier to relax than be more strict.

gziolo · 2018-11-06T16:48:44Z

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

+		typeof settings.tagName !== 'string' ||
+		settings.tagName === ''
+	) {
+		window.console.error(


Minor: When no tag name is provided, it could show a different error.

gziolo · 2018-11-06T16:51:03Z

packages/rich-text/src/store/selectors.js

+			return false;
+		}
+
+		return ` ${ elementClassName } `.indexOf( ` ${ className } ` ) >= 0;


I think I get why we add wrapping spaces :)

gziolo · 2018-11-06T16:53:29Z

packages/rich-text/src/test/create.js

+		value,
+	} ) => {
+		it( description, () => {
+			if ( formatName ) {


Nit: for simplicity, it might be easier to add the test without formatName in their own block.

What do you mean?

gziolo · 2018-11-06T16:55:10Z

packages/rich-text/src/test/create.js

+		formatName,
+		formatType,
+		html,
+		value,


Can we rename value to expectedValue? It's hard to follow spec data without a context.

gziolo · 2018-11-06T16:57:12Z

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

+import { registerFormatType } from '../register-format-type';
+import { unregisterFormatType } from '../unregister-format-type';
+
+describe( 'registerFormatType', () => {


Nice one. I personally prefer more code but to have all test pieces put together in one place :)

gziolo · 2018-11-06T17:01:08Z

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

-	}
-
-	const elementAttributes = {};
+	const elementAttributes = unregisteredAttributes ? { ...unregisteredAttributes } : {};


Skip it! I'm guessing, but it might also work as:

const elementAttributes = { ...unregisteredAttributes }

I tried with

{ ...null } // {} { ...undefined } // {}

Just sharing how JS tries to be smart :)

gziolo · 2018-11-06T17:03:06Z

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

 				const parent = getParent( pointer );
-				const newNode = append( parent, fromFormat( { type, attributes, object } ) );
+				const newNode = append( parent, fromFormat( format ) );


Do we no longer need to clone it? Just making sure it was done on purpose.

No, the cloning is not needed. I think it's a remnant, also to destruct object.

gziolo · 2018-11-06T17:29:22Z

Update example after recent changes in master:

wp.richText.registerFormatType( 'my-plugin/link', {
	title: 'Custom Link',
	tagName: 'a',
	attributes: {
		url: 'href',
	},
	className: 'my-custum-format',
	edit: function( props ) {
console.log( props ); 
		return wp.element.createElement( wp.editor.RichTextToolbarButton, {
			icon: 'editor-strikethrough',
			title: 'Custom Link',
			onClick: function() {
				props.onChange( wp.richText.toggleFormat( props.value, { type: 'my-plugin/link', attributes: {
					url: '#test',
				} } ) );
			},
			isActive: props.isActive,
		} );
	},
} );```

gziolo

It works as advertised in my testing. Let's move forward and address anything that is valid as a follow up. I will work on e2e tests tomorrow.

Nice work 👍

gziolo · 2018-11-06T17:56:17Z

packages/rich-text/src/test/create.js

@@ -61,7 +61,7 @@ describe( 'create', () => {
 		formatName,
 		formatType,
 		html,
-		value,
+		value: expectedValue,


I thought we could update it inside the definition,too :)

I can do it myself when working on e2e tests.

Oh. It is used for other tests as well where value is the input.

ellatrix added [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Priority] High Used to indicate top priority items that need quick attention labels Nov 5, 2018

ellatrix requested review from a team and gziolo November 5, 2018 11:33

ellatrix mentioned this pull request Nov 5, 2018

Annotation API #7718

Merged

4 tasks

ellatrix added this to the 4.2 milestone Nov 5, 2018

youknowriad reviewed Nov 6, 2018

View reviewed changes

ellatrix requested a review from a team November 6, 2018 09:44

youknowriad reviewed Nov 6, 2018

View reviewed changes

ellatrix mentioned this pull request Nov 6, 2018

Import Format API components instead of passing as props #11545

Merged

4 tasks

youknowriad approved these changes Nov 6, 2018

View reviewed changes

Fix format parsing

8ff7d19

ellatrix force-pushed the fix/format-parsing branch from 00b2afd to 8ff7d19 Compare November 6, 2018 16:02

gziolo reviewed Nov 6, 2018

View reviewed changes

gziolo approved these changes Nov 6, 2018

View reviewed changes

Address some feedback

c0ba9a2

gziolo reviewed Nov 6, 2018

View reviewed changes

youknowriad merged commit 2d5dec7 into master Nov 6, 2018

youknowriad deleted the fix/format-parsing branch November 6, 2018 18:07

youknowriad pushed a commit that referenced this pull request Nov 6, 2018

Rich Text: Fix Format Type Assignment During Parsing (#11488)

f0edb12

gziolo mentioned this pull request Nov 7, 2018

Testing: Add automated testing for Format API #11583

Closed

ellatrix mentioned this pull request Nov 16, 2018

TinyMCE editor events and transformation APIs #6648

Closed

technote-space mentioned this pull request Feb 6, 2019

RichText FormatType which has same tag but different class is applied #13687

Closed

jeherve mentioned this pull request Nov 9, 2023

Rankmath AI and Jetpack AI Conflict Automattic/jetpack#34033

Closed

Rich Text: Fix Format Type Assignment During Parsing #11488

Rich Text: Fix Format Type Assignment During Parsing #11488

Conversation

ellatrix commented Nov 5, 2018 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

ellatrix commented Nov 5, 2018

ellatrix commented Nov 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Nov 6, 2018 • 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

gziolo commented Nov 6, 2018

youknowriad left a comment

Choose a reason for hiding this comment

gziolo commented Nov 6, 2018 • 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

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

gziolo commented Nov 6, 2018

gziolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Nov 5, 2018 •

edited

Loading

youknowriad Nov 6, 2018 •

edited

Loading

gziolo commented Nov 6, 2018 •

edited

Loading