Run `wp.oldEditor.removep()` when transforming shortcodes by danielbachhuber · Pull Request #8077 · WordPress/gutenberg

danielbachhuber · 2018-07-20T11:58:31Z

Description

Multi-line shortcodes transformed to Shortcode Blocks end up with <p>
in awkward places. Thanks to @pento's quick thinking, we can use some
existing WordPress magic to solve the immediate problem without falling
into the deeper rabbit hole.

Fixes #3900

How has this been tested?

Create a post in the Classic Editor with the following text:

[column]

I daresay that Fry has discovered the smelliest object in the known universe! For the last time, I don't like lilacs! Your 'first' wife was the one who liked lilacs! No argument here. A true inspiration for the children.

[/column]

Open the post in Gutenberg and click "Convert to Blocks"
You should see the shortcode converted into a Shortcode Block without paragraph tags:

The prior behavior looked like this:

Types of changes

Bug fix.

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

Can we use removep and autop from the @wordpress/autop package instead of relying on the global? (Should also then add wp-autop as a script dependency of the wp-core-blocks handle)

danielbachhuber · 2018-07-20T13:02:40Z

Can we use removep and autop from the @wordpress/autop package instead of relying on the global?

Yep e81c6ab

pento · 2018-07-25T06:57:51Z

I've added an extra check removep() when saving a single freeform block.

When Gutenberg saves a freeform block, it doesn't wrap it in the comment delimiters. When there's a single freeform block in the post, there's no way to tell that the post has been touched by Gutenberg, so Gutenberg needs to clean up after itself.

This should possible be a seperate PR, but let's make this one the "fix the two issues reported #3900" PR.

danielbachhuber · 2018-07-25T10:47:39Z

This should possible be a seperate PR, but let's make this one the "fix the two issues reported #3900" PR.

@pento Could you clarify what you mean by this? Are we going down the path of landing this and also recreating unbalanced / broken paragraph tags on the inner content prior to rendering the shortcode?

danielbachhuber · 2018-07-25T13:14:01Z

@gziolo I'm not sure what's remaining on this. Do you know?

Changes requested done in e81c6ab.

pento · 2018-07-26T07:31:26Z

I've turned core/shortcode into a dynamic block, so we can make sure it's correctly autop-ed when it's rendered.

Includes unit tests and such.

@pento

Multi-line shortcodes transformed to Shortcode Blocks end up with `<p>` in awkward places. Thanks to @pento's quick thinking, we can use some existing WordPress magic to solve the immediate problem without falling into the deeper rabbit hole.

…reeform block.

…rectly.

pento · 2018-07-26T07:37:23Z

Also, I rebased/force pushed this branch, to get the latest version of the server fixtures, which I didn't really need to do, because server-registered.json only cares about dynamic blocks that register attributes. 🙂

danielbachhuber · 2018-07-26T12:34:28Z

💯 I tested this locally and it works exactly as expected.

danielbachhuber · 2018-07-26T14:41:24Z

@WordPress/gutenberg-core This is ready for a final review/merge per #3900 (comment) All issues are addressed.

pento

🎉

aduth · 2018-07-30T13:44:13Z

core-blocks/shortcode/index.js

 /**
 * WordPress dependencies
 */
+import { removep, autop } from '@wordpress/autop';


core-blocks does not have a dependency defined for wp-autop, so its existence is purely incidental and this could be prone to future breakage.

aduth · 2018-07-30T13:44:37Z

core-blocks/shortcode/index.js

 						type: 'string',
 						shortcode: ( attrs, { content } ) => {
-							return content;
+							return removep( autop( content ) );


It's not evident why we're doing this. An inline code comment would have been appropriate.

aduth · 2018-07-30T13:48:07Z

editor/store/selectors.js

-		return serialize( getBlocks( state ) );
+		const blocks = getBlocks( state );
+
+		if ( blocks.length === 1 && blocks[ 0 ].name === getUnknownTypeHandlerName() ) {


It's not evident why we're doing this. An inline code comment would have been appropriate.

aduth · 2018-07-30T13:48:34Z

editor/store/selectors.js

 import { __ } from '@wordpress/i18n';
 import { moment } from '@wordpress/date';
 import deprecated from '@wordpress/deprecated';
+import { removep } from '@wordpress/autop';


editor does not have a dependency defined for wp-autop, so its existence is purely incidental and this could be prone to future breakage.

aduth · 2018-07-30T13:50:50Z

phpunit/class-do-blocks-test.php

+	 * Test that shortcode blocks get the same HTML as shortcodes in Classic content.
+	 */
+	function test_the_content() {
+		add_shortcode( 'someshortcode', array( $this, 'handle_shortcode' ) );


I'm not as familiar with PHP testing, but should we be tearing down anything we create, i.e. a complementing remove_shortcode?

aduth · 2018-07-30T13:51:48Z

core-blocks/shortcode/index.php

+ */
+
+/**
+ * Performs wpautop() on the shortcode block content.


Could be improved to explain why we're autop-ing, not simply expressing that we are, which is already evident in the code itself.

Alternatively, it could be considered an implementation detail that the block's render applies autop, not necessarily appropriate for the callback function. In other words, this function should be documented along lines of "Renders the shortcode", and the implementation detail explained by an inline code comment.

aduth · 2018-07-30T13:54:47Z

None of my comments are blocking for Try Callout, but could be considered for further improvement.

aduth · 2018-07-30T18:45:07Z

Also noting that the getEditedPostContent selector is not the only place we serialize blocks (search), but as implemented here is the only place where we adopt this behavior of applying autop to the freeform-only content. It's not clear to me whether this is a "good thing", but the inconsistency seems prone to future unintended consequences.

danielbachhuber added [Type] Bug An existing feature does not function as intended [Feature] Paste [Feature] Shortcodes Related to shortcode functionality labels Jul 20, 2018

danielbachhuber added this to the 3.3 milestone Jul 20, 2018

danielbachhuber requested a review from a team July 20, 2018 11:58

danielbachhuber mentioned this pull request Jul 20, 2018

1.9.1: Gutenberg breaks "classic" posts w/ shortcodes by carelessly wrapping shortcodes into <p> tags #3900

Closed

aduth previously requested changes Jul 20, 2018

View reviewed changes

danielbachhuber requested review from a team and aduth July 20, 2018 13:02

aduth removed this from the 3.3 milestone Jul 20, 2018

pento added this to the Try Callout milestone Jul 25, 2018

gziolo assigned danielbachhuber Jul 25, 2018

danielbachhuber and others added 5 commits July 26, 2018 17:35

Avoid dependency on wp global

70790bd

Run removep() on serialised content when we're only saving a single f…

b9429e9

…reeform block.

Pass the block content to dynamic block render functions.

7deaecd

Register core/shortcode as a dynamic block, so it can be autop-ed cor…

e75c8ce

…rectly.

pento force-pushed the 3900-unautop branch from e40e063 to e75c8ce Compare July 26, 2018 07:36

danielbachhuber modified the milestones: Try Callout, 3.4 Jul 26, 2018

danielbachhuber removed their assignment Jul 26, 2018

pento approved these changes Jul 27, 2018

View reviewed changes

pento merged commit 68519eb into master Jul 27, 2018

pento deleted the 3900-unautop branch July 27, 2018 01:22

pento mentioned this pull request Jul 30, 2018

Pass content to render callback #5760

Closed

aduth reviewed Jul 30, 2018

View reviewed changes

danielbachhuber mentioned this pull request Aug 22, 2018

Pass content to block render callback #6239

Closed

4 tasks

aduth mentioned this pull request Sep 11, 2018

Editor: Consider single unmodified default block as empty content #9808

Merged

aduth mentioned this pull request Apr 23, 2019

Line Break Tags Added to Scripts and Styles #9056

Open

DAreRodz mentioned this pull request Nov 20, 2023

Short code introduces closing paragraph tags #54136

Open

Conversation

danielbachhuber commented Jul 20, 2018

Description

How has this been tested?

Types of changes

Checklist:

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

danielbachhuber commented Jul 20, 2018

Uh oh!

pento commented Jul 25, 2018

Uh oh!

danielbachhuber commented Jul 25, 2018

Uh oh!

danielbachhuber commented Jul 25, 2018

Uh oh!

pento commented Jul 26, 2018

Uh oh!

pento commented Jul 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danielbachhuber commented Jul 26, 2018

Uh oh!

danielbachhuber commented Jul 26, 2018

Uh oh!

pento left a comment

Choose a reason for hiding this comment

Uh oh!

aduth Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

aduth Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

aduth Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

aduth Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

aduth Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

aduth Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

aduth commented Jul 30, 2018

Uh oh!

aduth commented Jul 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pento commented Jul 26, 2018 •

edited

Loading