WordPress.org

Make WordPress Core

Opened 22 months ago

Last modified 8 weeks ago

#20943 new defect (bug)

Paragraphs get removed in table cells when Visual editor is refreshed

Reported by: JboyJW Owned by:
Milestone: Future Release Priority: normal
Severity: major Version: 3.2
Component: Formatting Keywords: needs-testing needs-unit-tests needs-patch 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 (2)

20943.diff (1.1 KB) - added by Otto42 22 months ago.
Possible patch for wpautop
20943.2.diff (1.8 KB) - added by SergeyBiryukov 22 months ago.

Download all attachments as: .zip

Change History (36)

comment:1 follow-up: Otto4222 months 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 22 months ago by Otto42 (previous) (diff)

comment:2 Otto4222 months ago

  • Component changed from Editor to TinyMCE

comment:3 Otto4222 months ago

See #19917.

Also possibly relevant: #6485.

comment:4 nacin22 months ago

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

comment:5 in reply to: ↑ 1 azaozz22 months 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 22 months ago by azaozz (previous) (diff)

comment:6 azaozz22 months ago

  • Component changed from TinyMCE to Formatting

Otto4222 months ago

Possible patch for wpautop

comment:7 Otto4222 months 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.

SergeyBiryukov22 months ago

comment:8 SergeyBiryukov22 months ago

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

comment:9 follow-up: azaozz22 months 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.

comment:10 in reply to: ↑ 9 ; follow-up: Otto4222 months 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.

comment:11 in reply to: ↑ 10 azaozz22 months 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".

comment:12 JboyJW22 months ago

  • Cc JboyJW added

comment:13 nacin19 months 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.

comment:14 SergeyBiryukov17 months ago

#22397 was marked as a duplicate.

comment:15 difreo17 months 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...

comment:16 palpatine197616 months ago

  • Cc palpatine1976 added

comment:17 markoheijnen16 months 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.

comment:18 sillybean15 months ago

  • Cc steph@… added

comment:19 knutsp15 months ago

  • Cc knut@… added

comment:20 SergeyBiryukov15 months ago

#23324 was marked as a duplicate.

comment:21 SergeyBiryukov15 months ago

#19917 was marked as a duplicate.

comment:22 nic0tin14 months ago

  • Cc nicolas.girardet@… added

comment:23 johanronstrom14 months ago

  • Cc johanronstrom added

comment:24 SergeyBiryukov12 months ago

#24125 was marked as a duplicate.

comment:25 SergeyBiryukov12 months ago

  • Version changed from 3.3.2 to 3.2

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

comment:26 Horttcore9 months 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 9 months ago by Horttcore (previous) (diff)

comment:27 johnellmore8 months ago

  • Cc johnellmore added

comment:28 beardj7 months ago

This issue is still happening in version 3.6.1.

comment:29 beardj7 months ago

  • Cc beardj added

comment:30 JakePT6 months 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.

comment:31 JakePT4 months ago

  • Severity changed from normal to major

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

comment:32 nacin3 months ago

  • Keywords wpautop added

comment:33 nacin3 months 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.)

comment:34 aatospaja8 weeks 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 8 weeks ago by aatospaja (previous) (diff)
Note: See TracTickets for help on using tickets.