Fix: Pasting a tag that is part of a transform and not matched ignores the content. #11244

jorgefilipecosta · 2018-10-30T14:50:05Z

On #11057 @jrmd discovered that if we remove the preformatted block (wp.blocks.unregisterBlockType('core/preformatted')) when pasting some content inside the pre tag e.g <pre>Pre</pre>the content is simply ignored.

What happens is that we have other blocks that implement a transform for the PRE tag (the code block) but simple pre-content is not matched by the code block raw handler so in this case, we just returned nothing and ignored the content.

This PR changes the situation when a tag contains a transform that handles it but does not match its content we now create an HTML block to contain that content (filtered to make sure it is something the user can insert on the site).

How has this been tested?

I removed the preformatted block wp.blocks.unregisterBlockType('core/preformatted').
I pasted some HTML content inside a PRE tag e.g:<pre>Pre</pre> and checked it was added inside an HTML block.

ellatrix · 2018-10-30T15:43:26Z

Could (integration) tests be added?

jorgefilipecosta · 2018-10-30T15:48:56Z

Hi @iseulde,
I was thinking of adding an end 2 end tests to check the case of removed preformatted I'm not certain if with integration we can test without preformatted being registered but I will have a look. Thank you for the suggestion.

noisysocks · 2018-11-06T04:16:05Z

packages/blocks/src/api/raw-handling/index.js


-				return;
+				return createBlock( 'core/html', {
+					content: originalPieceFiltered,


This isn't quite right. We will want to return the HTML of the current node here, not the entire piece.

return createBlock( 'core/html', { content: node.outerHTML, } );

You can see this bug by:

Create a new post

Run wp.blocks.unregisterBlockType( 'core/preformatted' ) in the console

Copy the entire text of this comment including the code snippet above

Paste it into Gutenberg

Only the code snippet above should appear as a HTML block. Instead, the entire comment appears in the HTML block.

GIF of the above:

youknowriad · 2018-11-08T08:45:31Z

Moving out of 4.3 as it doesn't seem urgent or ready.

jorgefilipecosta · 2018-11-16T19:26:17Z

I applied a series of changes to this PR, the problems identified should be fixed.
I went deeper on the problem and the root cause was that blocks may define a tag in their raw transformation schema and also contain a isMatcher that ignores a given node with this tag. If the content is not matched it should be filtered as if tag was not part of the schema, but instead, it was being discarded in the filter. This PR updates that.
Right now removing the preformatted block pastes <pre>Pre</pre> as a paragraph as it pastes in master if we remove preformatted and code block (contains a schema for the pre tag but does not matches this case).

youknowriad · 2018-11-19T14:40:20Z

I tested and confirmed that it works as advertised but I'm not sure I understand why we create a paragraph block instead of an HTML block like suggested initially?

jorgefilipecosta · 2018-11-19T15:02:06Z

Hi @youknowriad,

Our algorithm when we find some tag that we don't handle is to parse the children inside it as if the tag did not exist.
So right now (in master and this branch) if we paste <form>aaaa</form> we get a paragraph with <p>aaaa</p>.
The pre tag is only accepted because two blocks (preformatted and code) set a handle for it. In master, if we remove both of them pasting <pre>Pre</pre> gives us <p>Pre</p>.
What happened was that the code block accepted the pre tag but had a matcher to just accept some types of pre-content. We had a bug, where if a tag was accepted because of a specific raw handler transform, and its matcher did match the content, the content was discarded. In this PR we change that, the content now follows the general algorithm (parse the children inside it as if the tag did not exist).

The previous approach of returning HTML was not possible because it created a bug as @noisysocks discovered even if we are pasting content that can be handled (together with non matched content) our algorithm was not executed and we were just returning giant HTML block.

youknowriad · 2018-11-19T15:10:32Z

The previous approach of returning HTML was not possible because it created a bug as @noisysocks discovered even if we are pasting content that can be handled (together with non matched content) our algorithm was not executed and we were just returning giant HTML block.

This feels like the algorithm should be reversed so that we consider this as a fallback or something but granted I don't know enough about it to say whether it's possible/good ...

I'll probably defer this review to @iseulde @mcsf

catehstn · 2018-11-21T11:16:52Z

@iseulde @mcsf Can you take a look at this PR when you have time, please?

ellatrix · 2018-11-21T11:54:04Z

I think it needs a rebase after #12029. What issues are fixed here that that PR doesn't fix?

jorgefilipecosta · 2018-11-21T16:37:36Z

Hi @iseulde, this PR was rebased.
The core issue this PR address is still solved and the test case added here should fail on master.
The pre tag is only accepted on Gutenberg raw handling mechanism because two blocks preformatted and code registered raw transforms for it.
On master if we unregister the core block wp.blocks.unregisterBlockType('core/preformatted');, and then we paste <pre>aaaaa</pre> we get an HTML block with an empty pre tag, the content was lost (bug).
If on master we continue the tests and also unregister code block wp.blocks.unregisterBlockType('core/code');, we get a paragraph with aaaaa, this is right the content was not lost.

The bug is that if a given tag is only accepted because a block set a raw handler for it, but then is not matched on isMatch function we loose content.
On this branch, if a tag is accepted because of a rawHandler but unmatched, we do the same as if the tag was not supported so we don't lose the content.
On this branch, if we just unregister preformatted we don't lose the content it is pasted as a paragraph.

ellatrix · 2018-11-22T11:00:19Z

packages/blocks/src/api/raw-handling/utils.js

+				if ( ! objValue || ! srcValue ) {
+					return undefined;
+				}
+				return ( ...args ) => {


Would it be possible to comment on this? It's hard to follow this if right away.

A description of the overall flow of the mergeWith call would help too.

ellatrix · 2018-11-22T11:00:34Z

packages/blocks/src/api/raw-handling/utils.js

 				schema[ tag ].attributes.push( 'id' );
 			}
 		}
+		if ( isMatch ) {


Same here. Would it be possible to comment.?

ellatrix · 2018-11-22T11:01:23Z

packages/blocks/src/api/raw-handling/utils.js

-		if ( schema.hasOwnProperty( tag ) ) {
+		if (
+			schema.hasOwnProperty( tag ) &&
+			( ! schema[ tag ].isMatch || schema[ tag ].isMatch( node ) )


Same, let's leave a comment?

ellatrix

Sounds good to me. Would love some more inline comments.

…s the content.

the problem raised was addressed

mcsf · 2018-12-03T16:07:47Z

packages/blocks/src/api/raw-handling/utils.js

+		// If an isMatch function exists add it to each schema tag that it applies to.
+		if ( isMatch ) {
+			for ( const tag in schema ) {
+				schema[ tag ].isMatch = isMatch;


Is this mutating the block type? Should it?

Hi @mcsf, the transforms passed to this function are always the result of getRawTransformations and this function copies the transforms, so we are mutating that copy and not the block type.
https://github.com/WordPress/gutenberg/blob/master/packages/blocks/src/api/raw-handling/index.js#L57-L65
This function was already mutating the input, but I think given that it is a getter it is unexpected that this function mutates the input and I will open a follow-up PR that removes the mutations.

jorgefilipecosta added the [Type] Bug An existing feature does not function as intended label Oct 30, 2018

jorgefilipecosta added this to the WordPress 5.0 milestone Oct 30, 2018

gziolo modified the milestones: WordPress 5.0, 4.3 Nov 5, 2018

jorgefilipecosta force-pushed the fix/pasting-non-matched-tags-ignores-the-content branch from 3c7b4a7 to d33a274 Compare November 5, 2018 19:37

noisysocks previously requested changes Nov 6, 2018

View reviewed changes

youknowriad modified the milestones: 4.3, 4.4 Nov 8, 2018

youknowriad modified the milestones: 4.4, 4.5 Nov 15, 2018

jorgefilipecosta force-pushed the fix/pasting-non-matched-tags-ignores-the-content branch from d33a274 to e6ef9c6 Compare November 16, 2018 12:24

gziolo added the [Priority] High Used to indicate top priority items that need quick attention label Nov 19, 2018

mtias modified the milestones: 4.5, 4.6 Nov 19, 2018

mtias removed the [Priority] High Used to indicate top priority items that need quick attention label Nov 19, 2018

ellatrix mentioned this pull request Nov 20, 2018

Convert to blocks does not handle invalid lists and HTML without block equivalent #12029

Closed

jorgefilipecosta force-pushed the fix/pasting-non-matched-tags-ignores-the-content branch from e6ef9c6 to 57e4527 Compare November 21, 2018 15:38

ellatrix reviewed Nov 22, 2018

View reviewed changes

ellatrix approved these changes Nov 22, 2018

View reviewed changes

mtias modified the milestones: 4.6, 4.7 Nov 22, 2018

Fix: Pasting a tag that is part of a transform but non matched ignore…

9e01f2e

…s the content.

jorgefilipecosta force-pushed the fix/pasting-non-matched-tags-ignores-the-content branch from 57e4527 to 9e01f2e Compare November 22, 2018 22:52

jorgefilipecosta merged commit f4fc47e into master Nov 23, 2018

jorgefilipecosta deleted the fix/pasting-non-matched-tags-ignores-the-content branch November 23, 2018 11:55

mcsf reviewed Dec 3, 2018

View reviewed changes

mtias added the [Feature] Paste label Dec 6, 2018

jorgefilipecosta mentioned this pull request Dec 13, 2018

Refactor getBlockContentSchema to not mutate its input #12859

Merged

Fix: Pasting a tag that is part of a transform and not matched ignores the content. #11244

Fix: Pasting a tag that is part of a transform and not matched ignores the content. #11244

Uh oh!

Conversation

jorgefilipecosta commented Oct 30, 2018

How has this been tested?

Uh oh!

ellatrix commented Oct 30, 2018

Uh oh!

jorgefilipecosta commented Oct 30, 2018

Uh oh!

noisysocks Nov 6, 2018

Choose a reason for hiding this comment

Uh oh!

noisysocks Nov 6, 2018

Choose a reason for hiding this comment

Uh oh!

youknowriad commented Nov 8, 2018

Uh oh!

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

Uh oh!

Uh oh!

youknowriad commented Nov 19, 2018

Uh oh!

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

Uh oh!

Uh oh!

youknowriad commented Nov 19, 2018

Uh oh!

catehstn commented Nov 21, 2018

Uh oh!

ellatrix commented Nov 21, 2018

Uh oh!

jorgefilipecosta commented Nov 21, 2018

Uh oh!

ellatrix Nov 22, 2018

Choose a reason for hiding this comment

Uh oh!

mcsf Dec 3, 2018

Choose a reason for hiding this comment

Uh oh!

ellatrix Nov 22, 2018

Choose a reason for hiding this comment

Uh oh!

ellatrix Nov 22, 2018

Choose a reason for hiding this comment

Uh oh!

ellatrix left a comment

Choose a reason for hiding this comment

Uh oh!

mcsf Dec 3, 2018

Choose a reason for hiding this comment

Uh oh!

jorgefilipecosta Dec 9, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jorgefilipecosta commented Nov 16, 2018 •

edited

Loading

jorgefilipecosta commented Nov 19, 2018 •

edited

Loading

jorgefilipecosta Dec 9, 2018 •

edited

Loading