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.

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

Blocks: Shortcode: Support multi-line shortcodes #4942

Closed
wants to merge 4 commits into from

Conversation

mcsf
Copy link
Contributor

@mcsf mcsf commented Feb 7, 2018

Description

Partly fixes #4456

In progress: this doesn't yet preserve newlines within a Shortcode block if reloading the block (when switching between Visual and Code modes or by reloading Gutenberg).

When converting content to blocks (either explicitly via the Convert to blocks action or by pasting content), content fragments that are recognized as shortcodes (but don't have a specific block to handle it) are then sent through the generic Shortcode block's 'shortcode' transform. Since raw content is canonically treated as HTML, the Shortcode transform receives any newline as <br />. This PR converts these into \n inside the shortcode transform.

This was an issue because the Shortcode block intentionally treats its contents literally. This is because it is meant to be a place where HTML entities are not decoded, so as to better support the extent of WordPress shortcode usage. See #3609 for history.

How Has This Been Tested?

  1. Attempt to repeat the reported bug by following the steps in New lines in shortcode content are ignored #4456. In a nutshell, try to convert from classic to blocks or try to paste content with a multi-line shortcode such as:
[bwcsv]One,Two,Three
A,B,C
D,E,F
[/bwcsv]

and make sure the process creates a Shortcode block that respects the original line breaks.

Caveat: this is a remaining issue to be solved concerning pasting for shortcodes containing underscores. In other words, the issue author's [bw_csv] shortcode will not be correctly parsed upon pasting. It will, however, be handled correctly when converting from classic.

  1. Make sure that one-line shortcodes are still supported.
  2. Make sure that a fragment of text that reads <br /> will not be treated as a newline.

Screenshots (jpeg or gifs if applicable):

Before After
Before After

Types of changes

Checklist:

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

@mcsf mcsf added [Feature] Paste Backwards Compatibility Issues or PRs that impact backwards compatability [Status] In Progress Tracking issues with work in progress labels Feb 7, 2018
@mcsf mcsf mentioned this pull request Feb 8, 2018
3 tasks
@mcsf mcsf added this to the 2.8 milestone Apr 20, 2018
@mcsf
Copy link
Contributor Author

mcsf commented Apr 26, 2018

The main hurdle here is that, during content serialization, line breaks inside the shortcode block are destroyed by the beautifier:

return getUnknownTypeHandlerName() === block.name || ! saveContent ? saveContent : getBeautifulContent( saveContent );

Now, there's ways to handle this. One is that there may be other block types out there where total control of a block's inner HTML is desired. For this, something like supports would (and does, on my test site) work:

// in core-blocks/shortcode/index.js
supports: { , rawContent: true }

// in `blocks/api/serializer.js`'s `getBlockContent`:
return (
	getUnknownTypeHandlerName() === block.name ||
	hasBlockSupport( blockType, 'rawContent' ) ||
	! saveContent
) ? saveContent : getBeautifulContent( saveContent );

However, with @aduth and @iseulde we've wondered whether it still made sense, going forward, to systematically beautify blocks' inner HTML. If it doesn't, then we shouldn't rush to add a new support term. Should we then add an exception for shortcode, e.g.

+++ core-blocks/index.js
 setDefaultBlockName( paragraph.name );
 setUnknownTypeHandlerName( freeform.name );
+addRawContentBlockType( shortcode.name );
return (
	getUnknownTypeHandlerName() === block.name ||
	isRawContentBlockType( block.name ) ||
  	! saveContent
) ? saveContent : getBeautifulContent( saveContent );

?

@mcsf mcsf force-pushed the update/shortcode-block-handle-newlines branch from bba5d6b to 15a3736 Compare April 27, 2018 14:35
@mcsf
Copy link
Contributor Author

mcsf commented Apr 27, 2018

The last two commits work and represent the two approaches I described above.

@aduth
Copy link
Member

aduth commented Apr 27, 2018

In general I wish we'd just solve this from the serializer (no additional processing on RawHTML) and drop the beautifier altogether. I remarked in #4456 (comment) that I have this mostly working, but some questions remain on exactly what format the indentation should take.

Which is to say... can you tell me what beauty means to you? :troll:

@mcsf
Copy link
Contributor Author

mcsf commented Apr 27, 2018

Which is to say... can you tell me what beauty means to you? :troll:

I think we all know that the Beautiful is found only in LISP-style S-expressions.

More seriously:

In general I wish we'd just solve this from the serializer (no additional processing on RawHTML)

This makes sense, and what the comment you pointed to suggests seems reasonable to me. What action items do we have? How soon can we work on the serializer?

@ellatrix
Copy link
Member

The beautifier is not really about beauty... more about readability, no? Without it the text editor is not that useful? Is there no option to preserve line breaks with it?

@aduth
Copy link
Member

aduth commented Apr 30, 2018

There is actually a --preserve-newlines option (which I assume has a JS property equivalent), which may be fine as a short-term resolution:

https://github.com/beautify-web/js-beautify#css--html

To be added as an option to getBeautifulContent:

export function getBeautifulContent( content ) {
return beautifyHtml( content, {
indent_inner_html: true,
indent_with_tabs: true,
wrap_line_length: 0,
} );
}

Since the element serializer outputs no newlines of its own (except for those hard-coded), I expect this should work without conflict.

@@ -45,7 +44,7 @@ export const settings = {
text: {
type: 'string',
shortcode: ( attrs, { content } ) => {
return content;
return content.replace( /<br \/>/g, '' );
Copy link
Member

Choose a reason for hiding this comment

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

How do we know if these are added by the browser? Guess we would need to check if it's plain text, and only remove then?

Btw, the current editor also destroys the following.

[bwcsv]One,Two,Three
A,B,C
D,E,F
[/bwcsv]

becomes

<p>[bwcsv]One,Two,Three A,B,C D,E,F [/bwcsv]</p>

Copy link
Member

Choose a reason for hiding this comment

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

And if pasted in TinyMCE it becomes:

<p>[bw_csv]One,Two,Three<br />A,B,C<br />D,E,F<br />[/bw_csv]</p>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw, the current editor also destroys the following.

Not if manually entered, right?

Regardless, a block whose purpose is to hold shortcodes should be capable of preserving arbitrary formatting, IMO.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it should preserve it, agreed. But this line here is about input. Now I just realised this is actually the Markdown converter inserting these line break elements? With the simpleLineBreaks option. 😰

@aduth aduth modified the milestones: 2.8, 2.9 May 3, 2018
@aduth
Copy link
Member

aduth commented May 11, 2018

preserve_newlines is defaulted to true for HTML beautification, so this unfortunately doesn't seem to be helping the original issue.

@aduth
Copy link
Member

aduth commented Jun 18, 2018

Note: Alternative solution at #6716.

@aduth aduth modified the milestones: 3.1, 3.2 Jun 21, 2018
@mcsf
Copy link
Contributor Author

mcsf commented Jun 27, 2018

I'm happier with a broader fix like #6716, so I'll close this one. If we change our minds, we can open it back.

@mcsf mcsf closed this Jun 27, 2018
@mcsf mcsf deleted the update/shortcode-block-handle-newlines branch June 27, 2018 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Feature] Paste [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New lines in shortcode content are ignored
5 participants