Make WordPress Core

Opened 12 years ago

Closed 3 years ago

Last modified 18 months ago

#20943 closed defect (bug) (wontfix)

Paragraphs get removed in table cells when Visual editor is refreshed

Reported by: jboyjw's profile JboyJW Owned by: azaozz's profile azaozz
Milestone: Priority: normal
Severity: major Version: 3.2
Component: Formatting Keywords: wpautop
Focuses: Cc:

Description

As far as I know, this issue has been around since 3.1. It's not a bug in 3.0.4. I even stopped upgrading at 3.0.4 for any sites where I knew the client would need to edit tabular data. For security reasons, it's time to upgrade these and I'd REALLY like this issue to be fixed.

The problem happens when using paragraphs in a table cell. When I hit enter to create a new paragraph within a table cell, it looks fine. If I then hit Update/Publish, when the page refreshes, WordPress converts that paragraph break into a single line break. If I then click Update again, WordPress removes that linebreak entirely and the paragraphs are essentially merged.

I can also reproduce this behaviour without even clicking Update. If you just switch to HTML mode and then back to Visual mode, the same issue occurs.

Attachments (4)

20943.diff (1.1 KB) - added by Otto42 12 years ago.
Possible patch for wpautop
20943.2.diff (1.8 KB) - added by SergeyBiryukov 12 years ago.
20943.3.diff (651 bytes) - added by alleynoah 10 years ago.
JS tweak to preserve line breaks in table cells in pre_wpautop
20943_patch.diff (2.1 KB) - added by sandyr 10 years ago.
Patch to fix html, tinymce toggle issue for tables and lists

Download all attachments as: .zip

Change History (61)

#1 follow-up: @Otto42
12 years ago

Confirmed, the issue is in TinyMCE somewhere.

To reproduce, make a new post in HTML mode with this text:

<table>
<tbody>
<tr>
<td>
Test one

Test two

Test three
</td>
</tr>
</tbody>
</table>

Switch to visual and back. Notice that the Test one and two are now combined. Updating the post and then the subsequent reload of the visual editor causes the same problem.

Note that the issue also exists in 3.4.

Last edited 12 years ago by Otto42 (previous) (diff)

#2 @Otto42
12 years ago

  • Component changed from Editor to TinyMCE

#3 @Otto42
12 years ago

See #19917.

Also possibly relevant: #6485.

#4 @nacin
12 years ago

This also affects paragraphs within list items. Or did, anyway.

#5 in reply to: ↑ 1 @azaozz
12 years ago

Replying to Otto42:

Confirmed, the issue is in TinyMCE somewhere.

No, the issue is in wpautop (both JS and PHP versions). It doesn't handle well block elements that can contain other blocks. To reproduce: save a draft with the above html code from the HTML editor and check the source when viewing the post. It is:

<table>
<tbody>
<tr>
<td>
Test one</p>
<p>Test two</p>
<p>Test three
</td>
</tr>
</tbody>
</table>

where only the first paragraph is broken as HTML 5 allows for no closing </p>.

Last edited 12 years ago by azaozz (previous) (diff)

#6 @azaozz
12 years ago

  • Component changed from TinyMCE to Formatting

@Otto42
12 years ago

Possible patch for wpautop

#7 @Otto42
12 years ago

  • Keywords has-patch added

This change to wpautop seems to fix the issue without obvious side effects. However, it would need extensive testing.

Couldn't find the JS version of autop that you are referring to. Not sure where that is.

#8 @SergeyBiryukov
12 years ago

20943.2.diff brings the same change to the JS version.

#9 follow-up: @azaozz
12 years ago

  • Keywords needs-unit-tests added

Unfortunately it's not that easy :)

The patch works for the above example html code but fails for:

<td>Test one

Test two

Test three</td>

The problem with <p> in block tags that can contain other blocks is that it's optional, i.e. it can be <td>some text</td> or <td><p>some text</p></td> or even <td>some text<p>some more text</p></td>.

We will need separate rules for these cases. The alternative is to standardize on always having the <p> in there, but that's undesirable.

Also, this would need good/extensive unit tests if we are to get it right.

#10 in reply to: ↑ 9 ; follow-up: @Otto42
12 years ago

Replying to azaozz:

The alternative is to standardize on always having the <p> in there, but that's undesirable.

I'd be perfectly okay with always adding the p in there. That's sort of the whole point of wpautop. :)

Also, this would need good/extensive unit tests if we are to get it right.

Agreed.

#11 in reply to: ↑ 10 @azaozz
12 years ago

Replying to Otto42:

I'd be perfectly okay with always adding the p in there. That's sort of the whole point of wpautop. :)

That may prove "unpopular". If a user wants a tight table like:

_____________
| abc | 123 |

it will turn it into:

_____________
|     |     |
| abc | 123 |
|     |     |

Also <li> is one of these "block parents" and we don't want to force <li><p>...</p></li> everywhere but still would need to support it.

All that gets even more complicated when adding HTML 5.0 into the mix. Unfortunately in HTML 5 there aren't clearly pre-determined block tags in the same sense as in HTML 4 / XTHML 1. Most of the block rules there are "<p> can contain <a> but not if <a> contains block tag".

#12 @JboyJW
12 years ago

  • Cc JboyJW added

#13 @nacin
12 years ago

My take: Paragraphs, if specified for table cells or list items, should remain and/or be corrected. But they shouldn't be added forcefully if they did not exist.

#14 @SergeyBiryukov
12 years ago

#22397 was marked as a duplicate.

#15 @difreo
12 years ago

  • Keywords needs-patch added; has-patch removed
  • Severity changed from normal to major

This issue is a horror. It also happens when you wrap your text in a <div> Why change something that was good in previous versions? "I know better what you want - (L)USER" - said the program...

#16 @palpatine1976
12 years ago

  • Cc palpatine1976 added

#17 @markoheijnen
12 years ago

  • Severity changed from major to normal

Just out of curiosity if what Nacin said wouldn't that make the patch correct. It seems that it only applies for when you do insert paragraphs yourself.

#18 @sillybean
12 years ago

  • Cc steph@… added

#19 @knutsp
12 years ago

  • Cc knut@… added

#20 @SergeyBiryukov
12 years ago

#23324 was marked as a duplicate.

#21 @SergeyBiryukov
12 years ago

#19917 was marked as a duplicate.

#22 @nic0tin
12 years ago

  • Cc nicolas.girardet@… added

#23 @johanronstrom
12 years ago

  • Cc johanronstrom added

#24 @SergeyBiryukov
11 years ago

#24125 was marked as a duplicate.

#25 @SergeyBiryukov
11 years ago

  • Version changed from 3.3.2 to 3.2

Reproduced in 3.2 as well. Could not reproduce in 3.1.

#26 @Horttcore
11 years ago

A workaround for the moment is to add a class in example

<td>
<p class="void">foo</p>
<p class="void">bar</p>
</td>

This p tag will not be removed.

Last edited 11 years ago by Horttcore (previous) (diff)

#27 @johnellmore
11 years ago

  • Cc johnellmore added

#28 @beardj
11 years ago

This issue is still happening in version 3.6.1.

#29 @beardj
11 years ago

  • Cc beardj added

#30 @JakePT
11 years ago

Still occurring in 3.7. I do notice that it only happens to the first paragraph. Not sure if this was always the case or new to 3.7.

#31 @JakePT
11 years ago

  • Severity changed from normal to major

Still occurring in 3.8 RC2. Makes working with tables in WordPress next to impossible.

#32 @nacin
11 years ago

  • Keywords wpautop added

#33 @nacin
11 years ago

  • Milestone changed from Awaiting Review to Future Release

Paragraph tags might just benefit from special handling compared to other blocks, so they can remain in <td> or <li> (any others?) untouched. Would *love* to fix this. (I personally like paragraph tags inside lists.)

#34 @aatospaja
11 years ago

Still experiencing this when updating the post. Tried with the latest build I just pulled from git.

While switching from visual to text in the editor, it doesn't remove p/linebreaks but clicking update does.

Adding something like <p align="left"> will help keep the p tags (so it doesn't have to be class="something").

Last edited 11 years ago by aatospaja (previous) (diff)

#35 @JakePT
10 years ago

Still occurs in 3.9 with TinyMCE 4.0.

What's happening is that it will remove 1 paragraph break for every update. So if I had three lines:

<td>Line 1

Line 2

Line 3</td>

I press update and go to Text view, and it's become:

<td>Line 1Line 2

Line 3</td>

And the last time:

<td>Line 1Line 2Line 3</td>

#36 @OzzyCzech
10 years ago

Wordpress 3.9.1 same problem

@alleynoah
10 years ago

JS tweak to preserve line breaks in table cells in pre_wpautop

#37 follow-up: @alleynoah
10 years ago

According to my testing, the above fix (20943.3.diff) prevents the destruction of line breaks inside table cells without requiring any changes to the backend wpautop. It also does not force all table cell contents to be wrapped in <p> tags.

I'd be happy to contribute test coverage but couldn't quite parse the existing structure to figure out where it would make sense to do so (aside from generally, I think, in tests/qunit/editor/ somewhere?).

#38 @alleynoah
10 years ago

  • Keywords has-patch added; needs-patch removed

This ticket was mentioned in Slack in #core by alleynoah. View the logs.


10 years ago

#40 @DrewAPicture
10 years ago

  • Milestone changed from Future Release to 4.2
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

@SergeyBiryukov: Any chance you could follow up here, and consult on test coverage re: comment:37?

#41 in reply to: ↑ 37 @Milmor
10 years ago

Replying to alleynoah:

According to my testing, the above fix (20943.3.diff) prevents the destruction of line breaks inside table cells without requiring any changes to the backend wpautop. It also does not force all table cell contents to be wrapped in <p> tags.

With this fix the preview is correct, even if switching between html/text mode.

But when saved, the bug is still there and affects the preview (refreshed), even with wp_autop disabled. It happens with and without <p> (<p> are not kept - lines are not kept)

remove_filter( 'the_content', 'wpautop' );
remove_filter( 'the_excerpt', 'wpautop' );

Same with 20943.2.diff

4.2-alpha-31330

#42 @sandyr
10 years ago

  • Keywords needs-patch added; has-patch removed

The patch doesn't consider other block elements. Adding different block elements like <ul> <li> and toggling from visual to text(source) mode breaks the content (<p> tags) as well as preview.

I tried with :

<table>
<tbody>
<tr>
<td>
<ul>
<li>
<p>Test one</p>
<p>Test two</p>
<p>Test three</p>
</li>
</ul>
</td>
</tr>
</tbody>
</table>

The tinymce editor handles this well (http://www.tinymce.com/tryit/full.php). Open source editor with the sample test data above, the preview as well as the visual mode comes up fine.

Last edited 10 years ago by sandyr (previous) (diff)

#43 @sandyr
10 years ago

The patch doesn't consider other block elements. Adding different block elements like <ul> <li> and toggling from visual to text(source) mode breaks the content (<p> tags) as well as preview.

I tried with :
<table>
<tbody>
<tr>
<td>
<ul>
<li>
<p>Test one</p>
<p>Test two</p>
<p>Test three</p>
</li>
</ul>
</td>
</tr>
</tbody>
</table>

The tinymce editor handles this well (http://www.tinymce.com/tryit/full.php). Open source editor with the sample test data above, the preview as well as the visual mode comes up fine.

Version 0, edited 10 years ago by sandyr (next)

@sandyr
10 years ago

Patch to fix html, tinymce toggle issue for tables and lists

#44 @sandyr
10 years ago

  • Keywords has-patch added; needs-patch removed

Patch attached, as suggested by @nacin: "Paragraph tags might just benefit from special handling compared to other blocks, so they can remain in <td> or <li> (any others?) untouched."

Also fixes the _wp_Nop being invoked for every autosave or toggle from html to tinymce even if there are no changes in the text.

#45 @sandyr
10 years ago

  • Keywords dev-feedback added; needs-unit-tests removed

This ticket was mentioned in Slack in #core by sandeepr. View the logs.


10 years ago

#47 follow-up: @SergeyBiryukov
10 years ago

  • Owner changed from SergeyBiryukov to azaozz

@azaozz or @iseulde would be a better person to provide feedback here.

#48 in reply to: ↑ 47 @azaozz
10 years ago

Replying to SergeyBiryukov:

None of the patches fix this properly. 20943.3.diff works in some cases only, fails for <td>abc<p>123</p></td>. 20943_patch.diff just disables pre_wpautop (the removal of the <p> tags) if there is <table>, <ul> or <ol> in the content. That makes the Text editor inconsistent and causes problems with some of the display filters, like some shortcodes, etc.

Last edited 10 years ago by azaozz (previous) (diff)

#49 @azaozz
10 years ago

  • Keywords has-patch removed

#50 @azaozz
10 years ago

  • Keywords needs-testing dev-feedback removed
  • Milestone changed from 4.2 to Future Release

Fixing this should be a part of a more general fixes/improvements/reworking of wpautop(). Partial changes won't do it, we will just introduce other/different edge cases.

Related: #30644, #25856.

#51 @JakePT
9 years ago

This seems a little better in 4.3. Paragraphs are typically no longer removed during publishing or switching views.

However if a paragraph has a line break in it (Shift+Enter), then that paragraph will collapse into the following paragraph under the same conditions that paragraphs used to fail.

I'll play around a bit more to see what exactly what's happening.

#52 @SergeyBiryukov
9 years ago

#29345 was marked as a duplicate.

#53 @dankop
8 years ago

Is there any progress being made on this issue? Paragraph tags are still getting removed from the first paragraph inside an element (div in my case). In some cases with every switch between text/visual editor, a paragraph gets removed until all that's left is just a hug chunk of text.

#54 @DarcPress
8 years ago

Still bugged the same way as of today.

This is especially annoying when using ACF (Advanced Custom Fields) because sometimes the ACF takes time to load the TinyMCE and when it does, it behaves like moving from Text to Visual.

#55 in reply to: ↑ description @artworking
7 years ago

For anyone still having issues, this plugin has an option to turn off <p> and <br> removal called "Keep paragraph tags". Saved the day for me works when the issue is caused by ACF too.

https://wordpress.org/plugins/tinymce-advanced/

#56 @azaozz
3 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from reviewing to closed

Trying to patch autop doesn't seem relevant any more as it's on its way out.

#57 @sabernhardt
18 months ago

#58259 was marked as a duplicate.

Note: See TracTickets for help on using tickets.