-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
1.9.1: Gutenberg breaks "classic" posts w/ shortcodes by carelessly wrapping shortcodes into <p> tags #3900
Comments
Tested on 1.9.0, situation is exactly the same. |
Does the classic editor handle this gracefully when viewing a post with this markup on the front-end? |
Yes. (Maybe also have to account for https://core.trac.wordpress.org/ticket/14050) @aduth but may I ask, how is this question related to Gutenberg wrapping shortcodes into |
@lkraav My question was to try to clarify whether the Gutenberg behavior is in-fact the expected behavior, depending on how this would have been treated for pre-Gutenberg content. |
@aduth OK, but wouldn't the backend-relevant question be about "does classic editor have the same effect when switching between Text vs Visual modes"? But indeed, if you're saying Gutenberg backend transforms are already supposed to accurately mirror frontend rendering output, then I do see the point of the question, but how can this broken paragraph wrapping be expected output? Such expected output would have been breaking HTML for all opening+closing block level shortcodes for over a decade, no? |
The answer is "technically: no" because this is handled by Seems Gutenberg would need some sort of |
Tested broken in 2.2.0. (EDIT 2018-03-06 Tested broken in 2.3.0) |
@lkraav I took a look into this issue this morning. Based on my understanding, the problem of
Is my statement an accurate representation of your expectation, or is there additional detail I'm missing? |
Thanks for taking it on. Above output looks a bit suspect to me, because how would that look like in Visual Mode? We def. need to keep block-style visual editing intact between the opening and closing shortcodes (=== all blocks visual usage available). I think we'd be heading into Shortcake / Nested blocks territory here, which is a whole world on its own. Expected output for me, at least for starters, would be:
|
Possibly related: #5940. |
Oh, I think we have different expectations here. I don't expect you'll be able to visually edit the inner content of a shortcode. Your UI will look more like this: Specifically, the shortcode block adopts a Visually editing the inner content of a shortcode sounds like Pandora's box to me. |
Perhaps, but such a UX would be a world ahead of being restricted to plain text and/or html between opening and closing shortcodes. Isn't this exactly what Gutenberg is trying to get away from? OTOH, if shortcodes are being phased out for blocks, text-only could be a decent enough of a stop-gap solution. Primary goal should be to just keep markup from radically breaking, such as shown in the opening comment. (EDIT 2018-07-04 Tested broken in 3.1.1) |
Correct, this is my thinking.
Yes, I agree. |
With #7892, this Classic text:
Transforms into the following block:
Progress, but still not quite there. |
Thanks a ton for pushing this. Reminds me of the work I did for https://core.trac.wordpress.org/ticket/14050 which has stalled for years, because the risk surface and complexity of regexing through shortcodes is massive. I still use my own micro-plugin I posted there on all sites, as my mini-updates to the monster regex have proven to be completely production safe, and eliminated most of those But that's PHP |
Some conversation with @azaozz in the thread of https://wordpress.slack.com/messages/C02QB2JS7/ The suggestion to explore is:
|
If a fix is bad, but it works, is it actually bad? 🙂 $ git diff
diff --git a/core-blocks/shortcode/index.js b/core-blocks/shortcode/index.js
index 3f8d0b049..829f34d48 100644
--- a/core-blocks/shortcode/index.js
+++ b/core-blocks/shortcode/index.js
@@ -46,7 +46,7 @@ export const settings = {
text: {
type: 'string',
shortcode: ( attrs, { content } ) => {
- return content;
+ return wp.oldEditor.removep( wp.oldEditor.autop( content ) );
},
},
}, |
Lol, sounds similar to "violence is not the answer... but it works!" I still don't know the general answer to either. |
🎉 Honestly, I think it's better than @azaozz because it limits the scope of the change. With #3900 (comment), I'd be slightly terrified of something breaking in an unexpected way. |
I've done basically no testing on this fix, just a few cases to show that it removes all the Are there more complex cases that we need to deal with? Nested shortcodes, shortcodes with HTML inside them, that kind of thing? |
I think it's a reasonable assumption that
"Complex cases" is shortcode's middle name. I think #8077 is better than what we have now, and we'll discover whether there are other edge cases to accommodate for when Gutenberg sees a wider audience. |
While it surely looks strange, I don't know that this is strictly wrong. With Gutenberg completely disabled, this is exactly what's passed as the <?php
/**
* Plugin Name: Demo Shortcode
*/
add_shortcode( 'column', function( $attrs, $content ) {
error_log( $content );
return 'foo';
} );
So it doesn't seem unexpected that Gutenberg, with its ignoring
In fact, it's only with the proposed
This may be partly due to how Lines 134 to 149 in 5a65727
|
This assessment is correct. @lkraav Can you share more detail about how you handle this in the Classic Editor, and what your expectations would be about Gutenberg? @aduth Do you have a proposed resolution to suggest at this point? |
I was inclined to think that this would break, but in fact the rendered output is identical (though both the "original" and "converted" rendered outputs have non-balanced tags). I think this is because the server-side shortcode renderer replaces the opening Original: <h2>Shinier than yours, meatbag.</h2>
You can crush me but you can't crush my spirit! Is today's hectic lifestyle making you tense and impatient? It's a T. It goes "tuh". I'll tell them you went down prying the wedding ring off his cold, dead finger.
[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]
Hello world. Converted: <!-- wp:heading -->
<h2>Shinier than yours, meatbag.</h2>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>You can crush me but you can't crush my spirit! Is today's hectic lifestyle making you tense and impatient? It's a T. It goes "tuh". I'll tell them you went down prying the wedding ring off his cold, dead finger.</p>
<!-- /wp:paragraph -->
<!-- wp:shortcode -->
[column]</p>
<p>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.</p>
<p>
[/column]
<!-- /wp:shortcode -->
<!-- wp:paragraph -->
<p>Hello world.</p>
<!-- /wp:paragraph --> Original Rendered: <h2>Shinier than yours, meatbag.</h2>
<p>You can crush me but you can’t crush my spirit! Is today’s hectic lifestyle making you tense and impatient? It’s a T. It goes “tuh”. I’ll tell them you went down prying the wedding ring off his cold, dead finger.</p>
</p>
<p>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.</p>
<p>
<p>Hello world.</p> Converted Rendered: <h2>Shinier than yours, meatbag.</h2>
<p>You can crush me but you can’t crush my spirit! Is today’s hectic lifestyle making you tense and impatient? It’s a T. It goes “tuh”. I’ll tell them you went down prying the wedding ring off his cold, dead finger.</p>
</p>
<p>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.</p>
<p>
<p>Hello world.</p> |
Ok. My one remaining concern at this point is that a user who sees Removing the
Generally, I think the expected behavior is:
At this point though, I'd be fine to close as Or, we could land #8077 and also recreate unbalanced / broken paragraph tags on the inner content prior to rendering the shortcode. I'll wait for @lkraav to weigh in before closing. |
I rebased First thing noticed: opening comment use case still doesn't work off the bat. If the user just opens the example document in Gutenberg, saves it, markup will become silently broken, esp. if no editing operation was performed. Looking at #3900 (comment) it seems like it somehow became the expectation that the user should know to execute "Convert to Blocks" operation. No such assumption can be safely made, but I'm betting this is known so perhaps I'm misreading where exactly we're at. First and foremost, I would still expect Gutenberg to be able to maintain original comment document health. Convert to blocksGood news is, #8077 seems to indeed now provide correct transformation on the opening comment example document. Nice step forward, for sure. This working well would help admins all across the world perform safer one-click getaways from legacy markup. Can't imagine shipping 5.0 with anything less. Stretch goal convertSure would be dreamy if shortcode content would run through transformation chain as well. Feels like the infrastructure is already in place, anything specific stopping it? Dreamy output:
|
Can we clarify what is specifically broken? The markup changes, introducing paragraph tags which did not exist previously, but in what way is this not effectively the same as it was previously? When taking the markup from the original comment and viewing the output both as input into the Classic editor and then saved again in Gutenberg, I see no difference. |
Here's the breakage, using ORIGINAL CONTENT
GUTENBERG WRAPS SHORTCODES IN
I do see @pento pushing a new (to me) strategy in #8077 (comment) maybe this a game-changer here? |
Thanks, can attend in 2 hrs. |
<h2>Shinier than yours, meatbag.</h2>
<p>This is a post <strong>WITHOUT</strong> Gutenberg markup.</p>
<p><a href="https://github.com/WordPress/gutenberg/issues/3900">#3900</a></p>
<p>You can crush me but you can’t crush my spirit! Is today’s hectic lifestyle making you tense and impatient? It’s a T. It goes “tuh”. I’ll tell them you went down prying the wedding ring off his cold, dead finger.</p>
<div class="column-grid column-grid-2"><div class="column column-span-1 column-push-0 column-first"></p><!-- HERE -->
<p>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.</p>
<p>
</div>
<div class="column column-span-1 column-push-0 column-last"></p><!-- HERE -->
<p>Hello world.</p>
<p>
</div></div> Fortunately, this break seems to be https://core.trac.wordpress.org/ticket/14050 territory. After activating my "improved regex" microplugin, broken paragraphs disappear as they should (and have for the past x years). I also verified w/ 3.3.0 - breakage is the same, so #8077 or SUMMARYConsidering the only thing missing is the "stretch goal" of "Convert to blocks" also being able to blockify content between enclosing shortcodes
I'd suggest calling #3900 resolved and I file a new issue for enclosing shortcode blockification. It's definitely a big win that we got "Convert to blocks" simply convert shortcodes better right now, even if the content in there stays as is. Frontend rendering |
Sounds good, thanks @lkraav |
When transforming classic content to a shortcode block, or having a post with a single freeform block in it, `post_content` ended up in a state that wasn't backward compatible. This change ensures that un-blockified posts can continue to be used in the Classic Editor as expected, and that the shortcode block both renders in Gutenberg correctly, and on the front end. Fixes #3900.
(EDIT 2018-02-23 still broken in 2.2.0)
(EDIT 2018-03-06 still broken in 2.3.0)
(EDIT 2018-07-04 still broken in 3.1.1)
(EDIT 2018-07-21 still broken in 3.3.0)
Issue Overview
It is not safe to open classic posts in Gutenberg, shortcodes get unexpectedly wrapped in
<p>
tags.Steps to Reproduce (for bugs)
Expected Behavior
I'm not sure what the latest transformation goal for classic posts is.
<!-- wp:freeform -->
seems to be gone? Is #2454 relevant to the issue at hand?Current Behavior
I see the following transformation in Code Editor, which I consider broken for the shortcodes, since the result is broken rendering on the frontend:
"Convert to blocks" also gets completely confused by this result and fails to separate shortcodes into their own blocks.
Possible Solution
Not sure, could use pointers. What instructions should I provide users to avoid insta-breaking all classic content saved in Gutenberg?
Screenshots / Video
N/A
Related Issues and/or PRs
Not sure, could use pointers.
The text was updated successfully, but these errors were encountered: