Make WordPress Core

Opened 13 years ago

Closed 12 years ago

#18534 closed defect (bug) (fixed)

Synchronize block tags between PHP and JS versions of wpautop()

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by: nacin's profile nacin
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.2
Component: Editor Keywords: has-patch 3.5-early commit
Focuses: Cc:

Description (last modified by SergeyBiryukov)

As reported by WraithKenny on #11947:

Try in HTML Tab:

<select>
	<option>Value</option>
	<option>Value</option>   
	<option>Value</option>
	<option>Value</option> 
	<option>Value</option> 
</select>

Then switch to Visual and Back. The result is:

<select> <option>Value</option></select>
<select> <option>Value</option></select>
<select> <option>Value</option></select>
<select> <option>Value</option></select>
<select> <option>Value</option></select>

The patch synchronizes block tags between formatting.php and editor.dev.js, which fixes the issue.

Attachments (2)

18534.patch (2.0 KB) - added by SergeyBiryukov 13 years ago.
18534.2.patch (1.9 KB) - added by SergeyBiryukov 12 years ago.

Download all attachments as: .zip

Change History (22)

#1 @scribu
13 years ago

Related: #18514

Last edited 13 years ago by scribu (previous) (diff)

#3 @ryan
12 years ago

  • Milestone changed from 3.3 to Future Release

Seems good, but we really need some unit tests here. Punting.

#4 @SergeyBiryukov
12 years ago

  • Keywords needs-unit-tests added

#5 @nacin
12 years ago

  • Version changed from 3.3 to 3.2

#6 @SergeyBiryukov
12 years ago

Closed #19630 as a duplicate.

#7 @WraithKenny
12 years ago

  • Cc Ken@… added

#8 @nacin
12 years ago

  • Keywords needs-refresh added

Patch needs refresh as input is no longer considered a block element in wpautop.

#9 @SergeyBiryukov
12 years ago

  • Keywords needs-refresh removed

#10 @SergeyBiryukov
12 years ago

  • Description modified (diff)

#11 @nacin
12 years ago

  • Keywords 3.5-early added; needs-unit-tests removed

To be clear, the JS is missing option, map, area, style, figure, and figcaption.

The PHP was missing samp and noscript.

Writing tests for this isn't really feasible, so removing that keyword.

#12 follow-up: @azaozz
12 years ago

This should probably be done after updating to the next version of TinyMCE as it would come with HTML 5.0 support.

#13 @bananastalktome
12 years ago

  • Cc bananastalktome@… added

#14 in reply to: ↑ 12 @SergeyBiryukov
12 years ago

  • Milestone changed from Future Release to 3.5

Replying to azaozz:

This should probably be done after updating to the next version of TinyMCE as it would come with HTML 5.0 support.

Related: [21228] (for #21173)

#15 follow-ups: @MarcusPope
12 years ago

Just wanted to point out that the blocklist/$allblocks variables are not the only differences between the php and javascript wpautop functions. The javascript version includes regex transforms that are not included in the php version and vice versa. I highly suggest that you make an ajax request to the php version on tab switch instead of failing to maintain two versions. This would reduce the application complexity, and make the logic DRY.

#16 in reply to: ↑ 15 @SergeyBiryukov
12 years ago

Replying to MarcusPope:

I highly suggest that you make an ajax request to the php version

Related: #18514

#17 in reply to: ↑ 15 ; follow-up: @azaozz
12 years ago

Replying to MarcusPope:

...I highly suggest that you make an ajax request to the php version on tab switch instead of failing to maintain two versions. This would reduce the application complexity, and make the logic DRY.

Yes, there are differences between the PHP and JS versions of autop, but they serve different purpose.

Apart form making the Visual -> Text -> Visual editor switching a lot faster, the JS autop works together with TinyMCE's and browser's cleanup on loading the html code.

Was actually looking at removing all </p> from the JS autop. That would simplify it a lot and may remove the need for blocklist.

#18 in reply to: ↑ 17 @MarcusPope
12 years ago

Replying to azaozz:

Yes, there are differences between the PHP and JS versions of autop, but they serve different purpose.

Hmm, that's a low down dirty shame because if you save a post in HTML mode you sometimes get a different result than if you switch to Visual mode as a preview and then click save. And that different result has historically been invalid markup.

Apart form making the Visual -> Text -> Visual editor switching a lot faster, the JS autop works together with TinyMCE's and browser's cleanup on loading the html code.

Slightly faster but inconsistent vs slightly slower and consistent doesn't seem like a difficult choice to me. Though I can't convey strongly enough how hopeless the task of cleaning up html with regex expresssions really is.

But if the delay of a single ajax request is too daunting perhaps you could serve up an array of the same regex rules and the $allblocks variable via the wp_localize_script construct. With only a cursory glance between the two it seems that most of them could share the exact same regex and replacement strings... the client side could then use them to create new RegExp js objects. Doesn't work for functional replacements but it would reduce the duplicate logic and improve subsequent maintenance greatly.

Was actually looking at removing all </p> from the JS autop. That would simplify it a lot and may remove the need for blocklist.

You might want to consider disabling both versions of wpautop altogether. We've done that on all of our wp installations for the last 5 client projects. Letting TinyMCE do the work with its default paragraph insertion behavior has resulted in a lot less frustration with markup corruption. Maybe it's time to reconsider starting from scratch given TinyMCE's improvements in recent years.

As a proof of concept keep an eye out for version 1.2 of this plugin: http://wordpress.org/extend/plugins/preserved-html-editor-markup/

It should be ready by the end of the week. Not only will it preserve whitespace between the two tabs, it also disables autop entirely and gives users the option to use br newlines or use paragraph tags or both (where one return key press inserts a br tag and two return key presses inserts a new paragraph.)

Anyway, good luck to you all on this one (and its related tickets.)

#19 @nacin
12 years ago

  • Keywords commit added

Marking commit for 18534.2.patch.

#20 @nacin
12 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In [21888]:

Synchronize block-level elements between the JS and PHP versions of wpautop. props SergeyBiryukov. fixes #18534.

Note: See TracTickets for help on using tickets.