Make WordPress Core

Opened 19 years ago

Closed 4 years ago

#4298 closed defect (bug) (wontfix)

wpautop: Avoid wrapping <table> tags with a paragraph

Reported by: Denis-de-Bernardy Owned by: markjaquith
Priority: low Milestone:
Component: Formatting Version: 2.7
Severity: minor Keywords: needs-patch needs-unit-tests wpautop
Cc: Focuses:

Description

wpautop should at least ignore multiline html tags, and possibly ignore any area enclosed within html.

<table
 style="width: 120px; height: 92px; text-align: left; margin-left: auto; margin-right: auto;"
 border="1" cellpadding="7" cellspacing="2">
  <tbody>
    <tr>
      <td><small><small style="font-family: Arial;">Once
I saw it here, I instantly knew what I wanted. I love my new woven wood
shades.</small></small><br>
      <small><small>- Ginny Good</small></small></td>
    </tr>
  </tbody>
</table>

gets formatted as:

<table<br />
 style="width: 120px; height: 92px; text-align: left; margin-left: auto; margin-right: auto;"<br />
 border="1" cellpadding="7" cellspacing="2"><br />
<tbody>
<tr>
<td><small><small style="font-family: Arial;">Once<br />
I saw it here, I instantly knew what I wanted. I love my new woven wood<br />
shades.</small></small><br /><br />
      <small><small>- Ginny Good</small></small></td>

</tr>
</tbody>
</table>

Attachments (2)

4298.diff​ (1.2 KB ) - added by Denis-de-Bernardy 18 years ago.
4298.2.diff​ (1.5 KB ) - added by Denis-de-Bernardy 17 years ago.
new patch on pre_ filters

Download all attachments as: .zip

Change History (44)

#1 @foolswisdom
19 years ago

  • Milestone 2.2.1 → 2.4

#2 @darkdragon
19 years ago

  • Component Administration → Template

#3 @Denis-de-Bernardy
19 years ago

Reformatting tags like this before auto-formatting might work:

$str = preg_replace("/

(<[>]+)
\n
([
>]*>)
/x", , $str);

D.

#4 @Denis-de-Bernardy
19 years ago

  • Priority normal → low
  • Severity normal → minor

It should occur after escaping stuff we don't want to autop, though, i.e. scripts, objects, etc.

#5 @lloydbudd
19 years ago

  • Milestone 2.5 → 2.6

#6 @Denis-de-Bernardy
18 years ago

  • Keywords has-patch tested dev-feedback added
  • Milestone 2.9 → 2.8
  • Version 2.2 → 2.7

#8 @ryan
17 years ago

  • Component Template → Formatting
  • Owner anonymous removed

#9 @Denis-de-Bernardy
17 years ago

  • Keywords needs-patch added; has-patch tested dev-feedback removed
  • Milestone 2.8 → Future Release

better approach would be to apply this before saving the post's contents and excerpts.

#10 @Denis-de-Bernardy
17 years ago

code I use in my plugin:

		add_filter('content_save_pre', array('sem_fixes', 'fix_wpautop'), 0);
		add_filter('excerpt_save_pre', array('sem_fixes', 'fix_wpautop'), 0);
		add_filter('pre_term_description', array('sem_fixes', 'fix_wpautop'), 0);
		add_filter('pre_user_description', array('sem_fixes', 'fix_wpautop'), 0);
		add_filter('pre_link_description', array('sem_fixes', 'fix_wpautop'), 0);

	#
	# fix_wpautop()
	# http://core.trac.wordpress.org/ticket/4298
	#
	
	function fix_wpautop($content) {
		$content = str_replace(array("\r\n", "\r"), "\n", $content);
		
		while ( preg_match("/<[^<>]*\n/", $content) ) {
			$content = preg_replace("/(<[^<>]*)\n+/", "$1", $content);
		}
		
		return $content;
	} # fix_wpautop()

@Denis-de-Bernardy
17 years ago

new patch on pre_ filters

#12 @Denis-de-Bernardy
17 years ago

  • Keywords has-patch tested commit added; needs-patch removed
  • Milestone Future Release → 2.8

#13 @Denis-de-Bernardy
17 years ago

  • Owner set to Denis-de-Bernardy
  • Status new → accepted

#14 @westi
17 years ago

Before we carry on modifying autop I would really like to see a set of test cases written using the wordpress-tests setup so that we can really get a defined behaviour for this function and iron out all the bugs ;-)

I'm not sure we should make any changes to it's behaviour without these tests.

#15 @Denis-de-Bernardy
17 years ago

  • Milestone 2.8 → Future Release

agreed to a large extent. but I won't be writing them (I currently have a miles long list of things to do already).

punting this to future pending the unit tests.

#16 @Denis-de-Bernardy
17 years ago

  • Milestone Future Release → 2.8

doh, wait. I thought this was another ticket.

it's not a wpautop ticket, or rather it is, but it fixes a genuine bug in it. you don't need a unit test for this one. the stuff in the patch is done by the latest tinymce already. it's the php version that has a problem.

it merely turns stuff like:

<p
  class="foo">

... which gets wrecked by wpautop, into:

<p class="foo">

it has been working successfully for over a year on thousands of sites, and bugs would have been reported long ago if any were present.

the while loop stands for: until there is no open tag (<) followed by a newline char, turn the newline char into a space.

#17 @Denis-de-Bernardy
17 years ago

until there is no open tag (<) followed by a newline char before it's closed, turn the newline char into a space. even.

#18 @Denis-de-Bernardy
17 years ago

the ticket that needs unit tests is #3833. this one needs absolutely none of it. it's a bug, and the suggested patch is absolutely harmless.

#19 @Denis-de-Bernardy
17 years ago

  • Keywords early added
  • Milestone 2.8 → 2.9

can wait until 2.9

#20 @Denis-de-Bernardy
17 years ago

still applies clean

#21 @Denis-de-Bernardy
17 years ago

  • Owner Denis-de-Bernardy removed
  • Status accepted → assigned

#22 @hakre
17 years ago

Another long-player in the formatting bug league, +1 for getting a
definition, unit-tests and merging with related ones.

Versus: #3833, #4152

#23 @markjaquith
17 years ago

  • Milestone 2.9 → 3.0
  • Owner set to markjaquith
  • Status assigned → accepted

Early 3.0, for sure. Just hadn't seen this until now.

#24 @Denis-de-Bernardy
17 years ago

  • Keywords needs-patch added; has-patch tested commit removed

A quick note on this one, because an issue was brought to my attention a few weeks ago. My patch actually breaks things such as:

<?php
$foo = $bar;
?>

What I'm currently doing, in case it inspires a better solution:

	/**
	 * fix_wpautop()
	 *
	 * @param string $content
	 * @return string $content
	 **/

	function fix_wpautop($content) {
		$content = str_replace(array("\r\n", "\r"), "\n", $content);
		
		if ( !preg_match("/<[a-z][^<>]*\n/i", $content) )
			return $content;
		
		global $sem_fixes_escape;
		$sem_fixes_escape = array();
		
		$content = preg_replace_callback("/<\?php.+\?>/is", array('sem_fixes_admin', 'escape_php_callback'), $content);
		
		while ( preg_match("/<[a-z][^<>]*\n/i", $content) ) {
			$content = preg_replace("/(<[a-z][^<>]*)\n+/i", "$1", $content);
		}
		
		if ( $sem_fixes_escape )
			$content = str_replace(array_keys($sem_fixes_escape), array_values($sem_fixes_escape), $content);
		
		return $content;
	} # fix_wpautop()
	
	
	/**
	 * escape_php_callback()
	 *
	 * @param array $match
	 * @return string $out
	 **/

	function escape_php_callback($match) {
		global $sem_fixes_escape;
		
		$tag_id = "----sem_fixes_escape:" . md5($match[0]) . "----";
		$sem_fixes_escape[$tag_id] = $match[0];
		
		return $tag_id;
	} # escape_php_callback()

# http://core.trac.wordpress.org/ticket/4298
add_filter('content_save_pre', array('sem_fixes_admin', 'fix_wpautop'), 0);
add_filter('excerpt_save_pre', array('sem_fixes_admin', 'fix_wpautop'), 0);
add_filter('pre_term_description', array('sem_fixes_admin', 'fix_wpautop'), 0);
add_filter('pre_user_description', array('sem_fixes_admin', 'fix_wpautop'), 0);
add_filter('pre_link_description', array('sem_fixes_admin', 'fix_wpautop'), 0);

#25 follow-up: @azaozz
17 years ago

Think this is mostly fixed now. It can only happen when using the HTML editor. By default the visual editor removes all line breaks (except in <pre> tag) as they are insignificant and the JS version of autop removes any breaks inside tags when switching HTML to visual. What remains is perhaps post by email, atompub, etc.

#26 in reply to: ↑ 25 @Denis-de-Bernardy
17 years ago

Replying to azaozz:

Think this is mostly fixed now. It can only happen when using the HTML editor. By default the visual editor removes all line breaks (except in <pre> tag) as they are insignificant and the JS version of autop removes any breaks inside tags when switching HTML to visual. What remains is perhaps post by email, atompub, etc.

The user who reported it was pasting HTML code from Dreamweaver, I think, into the HTML editor. It was a real annoyance (for him anyway). You're welcome to close if you're not interested in fixing it; only a handful of users reported the issue to me, and I've a plugin that adds the above-mentioned filters.

#27 @Denis-de-Bernardy
17 years ago

@andrew: shall we close, or should I massage a patch?

#28 @hakre
17 years ago

  • Keywords dev-feedback added

@andrew/azzozz: please decide.

#29 @azaozz
17 years ago

There are several tickets about autop and HTML block elements that can contain other blocks. Think we should handle them together but first will need well made test cases as @westi suggests. Without them don't think we will be able to do this properly.

#30 @nacin
17 years ago

  • Keywords wpautop added
  • Milestone 3.0 → Future Release

Moving to future, we need test cases and maybe we can take care of the whole group of wpautop tickets.

#31 @c3mdigital
13 years ago

  • Keywords wpautop early dev-feedback removed

Tested: Works in visual fails in Text.

#32 @nacin
13 years ago

  • Keywords wpautop added

#33 @chriscct7
11 years ago

  • Keywords needs-unit-tests added; wpautop removed
  • Milestone Future Release
  • Resolution → wontfix
  • Status accepted → closed

Closing as wontfix. Complete lack of interest on the ticket over the last 6 years. Feel free to reopen when more interest re-emerges (particularly if there's a patch)

#34 @miqrogroove
11 years ago

  • Keywords wpautop added
  • Milestone → Future Release
  • Resolution wontfix
  • Status closed → reopened

#35 @Mte90
4 years ago

Just tested:

<table
 style="width: 120px; height: 92px; text-align: left; margin-left: auto; margin-right: auto;"
 border="1" cellpadding="7" cellspacing="2">
  <tbody>
    <tr>
      <td><small><small style="font-family: Arial;">Once
I saw it here, I instantly knew what I wanted. I love my new woven wood
shades.</small></small><br>
      <small><small>- Ginny Good</small></small></td>
    </tr>
  </tbody>
</table>

And 6.2 alpha now generates:

<p><table style="width: 120px; height: 92px; text-align: left; margin-left: auto; margin-right: auto;" cellspacing="2" cellpadding="7" border="1">
  <tbody>
    <tr>
      <td><small><small style="font-family: Arial;">Once
I saw it here, I instantly knew what I wanted. I love my new woven wood
shades.</small></small><br>
      <small><small>&#8211; Ginny Good</small></small></td>
    </tr>
  </tbody>
</table></p>

We can see that just ignored all the HTML content and added a paragraph to wrap everything.

Last edited 4 years ago by Mte90 (previous) (diff)

#36 @Mte90
4 years ago

  • Keywords close added

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


4 years ago

#38 @Mte90
4 years ago

So our discussion right now is to understand if it is valid that a paragraph can include a table or not as for the spec ​https://developer.mozilla.org/en-US/docs/Web/HTML/Element/p is not clear.

I will investigate on that.

#39 follow-up: @audrasjb
4 years ago

  • Keywords close removed
  • Summary wpautop bugs → wpautop: Avoid wrapping <table> tags with a paragraph

As per today's Old Tickets Triage session, it looks like there is still an issue: <p> element can't contain <table> elements (see ​MDN Specs).

#40 @Mte90
4 years ago

Just a HTML example to test in the W3C validator (​https://validator.w3.org/#validate_by_input):

<!DOCTYPE html>
<html lang="en">
<head>
<title>My Table-y paragraph.</title>
</head>
<body>
<p>
 <table>
  <tr>
   <td>Hello</td>
  </tr>
 </table>
</p>
</body>
</html>

#41 @Mte90
4 years ago

So reading the spec ​https://www.w3.org/TR/html4/struct/text.html#h-9.3 (I think that is the same as HTML5 the W3C validator infact report it as error) paragraph cannot contain a table element.

Reading seems that we open a paragraph, we add a table and the close of the table tag close the paragraph.

This means that we need to fix wpautop and add some tests for that case.

#42 in reply to: ↑ 39 @azaozz
4 years ago

  • Milestone Future Release
  • Resolution → wontfix
  • Status reopened → closed

Replying to audrasjb:

it looks like there is still an issue: <p> element can't contain <table>...

Yes, generally wpautop doesn't support tables. There is a workaround: no line breaks between any of the table tags (tr, td, etc.). Then the tables mostly work properly.

This ticket hasn't been fixed for 19 years because similar changes to wpautop tend to introduce bugs and other/new edge cases. Another reason is that some of the incorrect behavior may be expected by other code and fixing it would break that code.

Re-closing as "wontfix" seems the right thing to do here.

Note: See TracTickets for help on using tickets.