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

Compat: Disable wpautop in Classic Editor when post contains block #2708

Merged
merged 2 commits into from Sep 25, 2017

Conversation

Projects
None yet
4 participants
@aduth
Member

aduth commented Sep 9, 2017

This pull request seeks to resolve an issue where saving a post which contains paragraph blocks in the current editor ("Classic Editor") will cause those blocks to become invalid when returning to the Gutenberg editor.

Implementation notes:

The reason that blocks become invalid is because by default, the current post editor will strip paragraph tags (the removep behavior). Since the Gutenberg editor expects these paragraph tags to exist, it will then flag these blocks as invalid (modified externally). The implementation here detects whether the post contains blocks (using the new gutenberg_post_has_blocks introduced in #1797) and, if so, disables the wpautop behavior for the Classic Editor.

Testing instructions:

  1. (Prerequisite) Create and save a post in Gutenberg with at least on paragraph block
  2. Navigate to Posts
  3. Hover the post created in Step 1 and click Classic Editor
  4. If not already, switch to Visual mode
  5. Press Save
  6. (Depending on whether #2707 is resolve), note that when returning (or redirected to) the post in Gutenberg, the paragraphs are still valid

aduth added some commits Sep 9, 2017

phpcs
fixup

@BE-Webdesign BE-Webdesign requested review from mdawaffe and removed request for mdawaffe Sep 9, 2017

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Sep 14, 2017

Member

Could we also remove the_content filter? If it's a Gutenberg post, we don't need to run all these regular expressions: https://developer.wordpress.org/reference/functions/wpautop/

Member

iseulde commented Sep 14, 2017

Could we also remove the_content filter? If it's a Gutenberg post, we don't need to run all these regular expressions: https://developer.wordpress.org/reference/functions/wpautop/

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 15, 2017

Member

@iseulde Yes, I think so. This only achieves the removep behavior in the current editor, but there are also issues with wpautop being unnecessarily applied to content of Gutenberg-based posts. Do you think bypassing the filtering should be applied on a per-block level, or that we could simply reuse the gutenberg_post_has_blocks function to bypass the filter for a post which has any blocks?

Member

aduth commented Sep 15, 2017

@iseulde Yes, I think so. This only achieves the removep behavior in the current editor, but there are also issues with wpautop being unnecessarily applied to content of Gutenberg-based posts. Do you think bypassing the filtering should be applied on a per-block level, or that we could simply reuse the gutenberg_post_has_blocks function to bypass the filter for a post which has any blocks?

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Sep 22, 2017

Member

I would do the latter. Would you be concerned with that?

Member

iseulde commented Sep 22, 2017

I would do the latter. Would you be concerned with that?

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 22, 2017

Member

I think it is a simpler/performant approach, yes, but I'd want to confirm that it won't have any regressions for posts which were originally edited in the current editor expecting wpautop, then later edited in Gutenberg. Something like:

First Paragraph

Second Paragraph

<!-- wp:core/paragraph-->
<p>A wild Gutenberg paragraph appears!</p>
<!-- /wp:core/paragraph-->

This would be treated as a "Gutenberg" post, and if disabling wpautop were to apply to the entire post, the first two paragraphs would not have a <p> applied. What we might want to do to remedy this is ensure that wpautop is applied to freeform block content during the Gutenberg save step.

Member

aduth commented Sep 22, 2017

I think it is a simpler/performant approach, yes, but I'd want to confirm that it won't have any regressions for posts which were originally edited in the current editor expecting wpautop, then later edited in Gutenberg. Something like:

First Paragraph

Second Paragraph

<!-- wp:core/paragraph-->
<p>A wild Gutenberg paragraph appears!</p>
<!-- /wp:core/paragraph-->

This would be treated as a "Gutenberg" post, and if disabling wpautop were to apply to the entire post, the first two paragraphs would not have a <p> applied. What we might want to do to remedy this is ensure that wpautop is applied to freeform block content during the Gutenberg save step.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Sep 22, 2017

Member

Hm, wouldn't those be wrapped in a freeform block?

Member

iseulde commented Sep 22, 2017

Hm, wouldn't those be wrapped in a freeform block?

@aaronjorbin

This comment has been minimized.

Show comment
Hide comment
@aaronjorbin

aaronjorbin Sep 22, 2017

Member

In addition to the classic editor, we need to consider posts that come in via xml-rpc, native apps, and the rest api. Gutenberg isn't the sole way people will edit content. I think a refined wpautop that only applies to things that aren't in gutenberg blocks makes the most sense for backward compatibility.

Member

aaronjorbin commented Sep 22, 2017

In addition to the classic editor, we need to consider posts that come in via xml-rpc, native apps, and the rest api. Gutenberg isn't the sole way people will edit content. I think a refined wpautop that only applies to things that aren't in gutenberg blocks makes the most sense for backward compatibility.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Sep 22, 2017

Member

In that case, we need to think about how that might occur.

  • Should we convert non-blocks as they come in with APIs? Or should we store the content as we receive it?
  • Would it be strange to expect HTML formatted content from APIs? I think it would be much stranger to expect something that is neither valid HTML nor plain text. Might be a good reason to normalise at save.
  • Similarly, it's always bothered a bit me that we send out invalid HTML to edit with the REST API. WordPress doesn't even supply a script for applications to turn that into HTML for editing. Have fun with that. :) Also, the application will most likely return valid HTML (with Ps), so we now have two different formats.
  • In the case of auto-p, if the user wants to edit a post that way, these conversions should occur at the editor level, and not affect the whole system.
  • Not specific to auto-p, but regarding using other editors in general. What will happen if you dump Gutenberg post content into a normal rich text editor? I'm sure that, if you would dump it in TinyMCE and start editing, you will quickly get to the point where the parser would not make any sense of it. I guess that, if the editor is not plain HTML text, it will need to be Gutenberg-aware.
Member

iseulde commented Sep 22, 2017

In that case, we need to think about how that might occur.

  • Should we convert non-blocks as they come in with APIs? Or should we store the content as we receive it?
  • Would it be strange to expect HTML formatted content from APIs? I think it would be much stranger to expect something that is neither valid HTML nor plain text. Might be a good reason to normalise at save.
  • Similarly, it's always bothered a bit me that we send out invalid HTML to edit with the REST API. WordPress doesn't even supply a script for applications to turn that into HTML for editing. Have fun with that. :) Also, the application will most likely return valid HTML (with Ps), so we now have two different formats.
  • In the case of auto-p, if the user wants to edit a post that way, these conversions should occur at the editor level, and not affect the whole system.
  • Not specific to auto-p, but regarding using other editors in general. What will happen if you dump Gutenberg post content into a normal rich text editor? I'm sure that, if you would dump it in TinyMCE and start editing, you will quickly get to the point where the parser would not make any sense of it. I guess that, if the editor is not plain HTML text, it will need to be Gutenberg-aware.
@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 25, 2017

Member

Hm, wouldn't those be wrapped in a freeform block?

It would be parsed as a freeform block, but not explicitly wrapped with comment delimiters as of #2513.

I don't know that there's any contention on the point of disabling the removep setting for the current editor for Gutenberg posts, so I will merge this as-is and open a new issue to continue discussion on how to handle wpautop for Gutenberg / mixed-source posts.

Member

aduth commented Sep 25, 2017

Hm, wouldn't those be wrapped in a freeform block?

It would be parsed as a freeform block, but not explicitly wrapped with comment delimiters as of #2513.

I don't know that there's any contention on the point of disabling the removep setting for the current editor for Gutenberg posts, so I will merge this as-is and open a new issue to continue discussion on how to handle wpautop for Gutenberg / mixed-source posts.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Sep 25, 2017

Member

Sounds good, sorry for the distraction @aduth!

Member

iseulde commented Sep 25, 2017

Sounds good, sorry for the distraction @aduth!

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 25, 2017

Member

Actually, let's just use #2736 as the canonical issue for tracking application of wpautop to Gutenberg posts.

sorry for the distraction

Certainly not! I'd hoped to have been able to include the necessary changes here, but it's obviously a more complicated issue than I might have imagined, so needs additional discussion.

Member

aduth commented Sep 25, 2017

Actually, let's just use #2736 as the canonical issue for tracking application of wpautop to Gutenberg posts.

sorry for the distraction

Certainly not! I'd hoped to have been able to include the necessary changes here, but it's obviously a more complicated issue than I might have imagined, so needs additional discussion.

@aduth aduth merged commit 95cf846 into master Sep 25, 2017

3 checks passed

codecov/project 33.63% remains the same compared to fbb2d42
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@aduth aduth deleted the fix/removep-gutenberg branch Sep 25, 2017

@gschoppe

This comment has been minimized.

Show comment
Hide comment
@gschoppe

gschoppe Oct 3, 2017

This seems like the wrong approach, if you want to maintain compatibility with third-party tools and outside editors, since they will expect wpautop, and may either blindly insert content that requires it, or may convert existing paragraphs to line breaks, like how the current TinyMCE editor does. It seems like as fundamental a breaking change as replacing the entire format.

gschoppe commented Oct 3, 2017

This seems like the wrong approach, if you want to maintain compatibility with third-party tools and outside editors, since they will expect wpautop, and may either blindly insert content that requires it, or may convert existing paragraphs to line breaks, like how the current TinyMCE editor does. It seems like as fundamental a breaking change as replacing the entire format.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Oct 3, 2017

Member

Hi @gschoppe , thanks for the feedback. The changes here don't do more than disable the paragraph stripping enabled by default in the classic editor, thereby avoiding an issue where [Gutenberg] paragraph blocks were being mangled by the formatting. Ultimately, the result that reaches the front-end of a site is the same: paragraphed content, whether or not the existing post with blocks is edited in the classic editor or in Gutenberg.

Later discussion here and the in-progress work in #2806 is more relevant to the application of automatic paragraphs. I'd considered (but not yet implemented) adding save-time hooks to normalize non-block content to paragraphs, meaning regardless if a third-party editor adds newline-separated content or explicit paragraph tags, the saved content would maintain the same format (with explicit paragraph tags). I will plan to circle back to that pull request in the next few days, and your feedback would be appreciated there.

Member

aduth commented Oct 3, 2017

Hi @gschoppe , thanks for the feedback. The changes here don't do more than disable the paragraph stripping enabled by default in the classic editor, thereby avoiding an issue where [Gutenberg] paragraph blocks were being mangled by the formatting. Ultimately, the result that reaches the front-end of a site is the same: paragraphed content, whether or not the existing post with blocks is edited in the classic editor or in Gutenberg.

Later discussion here and the in-progress work in #2806 is more relevant to the application of automatic paragraphs. I'd considered (but not yet implemented) adding save-time hooks to normalize non-block content to paragraphs, meaning regardless if a third-party editor adds newline-separated content or explicit paragraph tags, the saved content would maintain the same format (with explicit paragraph tags). I will plan to circle back to that pull request in the next few days, and your feedback would be appreciated there.

@gschoppe

This comment has been minimized.

Show comment
Hide comment
@gschoppe

gschoppe Oct 3, 2017

But this means that third-party editors will be unable to properly deal with content within Gutenberg blocks, and may attempt to normalize it to a removep state. Yes, this stops the TinyMCE editor from running removep, but it doesn't stop any third-party tools that emulates that functionality. As much as it is awful, wpautop is part of the current format standard for posts, and removing it in a piecemeal manner breaks compatibility with the current format.

gschoppe commented Oct 3, 2017

But this means that third-party editors will be unable to properly deal with content within Gutenberg blocks, and may attempt to normalize it to a removep state. Yes, this stops the TinyMCE editor from running removep, but it doesn't stop any third-party tools that emulates that functionality. As much as it is awful, wpautop is part of the current format standard for posts, and removing it in a piecemeal manner breaks compatibility with the current format.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Oct 4, 2017

Member

Do you have specific examples in mind of editors mimicking this behavior of removing paragraphs? Gutenberg will accommodate new incoming content additions quite well, even those added by existing editors (the current editor), plugins, or third-party tools. Handling automatic paragraphs poses some challenge, sure, but outside of hypotheticals, it is showing to be largely addressable.

Member

aduth commented Oct 4, 2017

Do you have specific examples in mind of editors mimicking this behavior of removing paragraphs? Gutenberg will accommodate new incoming content additions quite well, even those added by existing editors (the current editor), plugins, or third-party tools. Handling automatic paragraphs poses some challenge, sure, but outside of hypotheticals, it is showing to be largely addressable.

@gschoppe

This comment has been minimized.

Show comment
Hide comment
@gschoppe

gschoppe Oct 4, 2017

I'm working on getting a Mac test environment set up to compare some of the various blogging apps that export to WordPress (most of these are mac-only). I should have more detailed results soon. At a glance, I would expect that issues might crop up with software like the following:

All of these implement a WYSIWYG editor that saves to WordPress, and I believe most can pull posts in from WordPress to edit. As I said, I will have more data soon, but tools like this have a very high likelihood of reimplementing removep and wpautop.

gschoppe commented Oct 4, 2017

I'm working on getting a Mac test environment set up to compare some of the various blogging apps that export to WordPress (most of these are mac-only). I should have more detailed results soon. At a glance, I would expect that issues might crop up with software like the following:

All of these implement a WYSIWYG editor that saves to WordPress, and I believe most can pull posts in from WordPress to edit. As I said, I will have more data soon, but tools like this have a very high likelihood of reimplementing removep and wpautop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment