WordPress.org

Make WordPress Core

Opened 8 months ago

Last modified 6 months ago

#39377 reviewing defect (bug)

wpautop adds a extra </p>

Reported by: pbearne Owned by: azaozz
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.7
Component: Formatting Keywords: dev-feedback
Focuses: Cc:

Description

I am seeing similar problem this code

<?php
wpautop( '<div><span></span><div></div></div>' );

renders

 "<div><span></span></p>
<div></div>
</div>
"

Attachments (4)

Autop-extra-p.php.patch (782 bytes) - added by pbearne 8 months ago.
Autop-extra-p.php.2.patch (780 bytes) - added by pbearne 8 months ago.
fixed the uses comment
formatting.php.patch (1.0 KB) - added by pbearne 8 months ago.
fixes it
autop.patch (1.5 KB) - added by pbearne 8 months ago.
patch and test

Download all attachments as: .zip

Change History (14)

@pbearne
8 months ago

fixed the uses comment

@pbearne
8 months ago

fixes it

#1 @pbearne
8 months ago

  • Keywords has-patch has-unit-tests dev-feedback added

Added a patch that does fix it but the fix is to add span to the block items which does feel right

This was caused by https://core.trac.wordpress.org/changeset/38592

#2 @azaozz
8 months ago

  • Keywords has-patch removed

Looking at formatting.php.patch, we cannot add span to the list of "block tags", it is not a block :) That would introduce numerous failures with spans in paragraphs.

Not even sure if this edge case can be fixed without removing the extra \n from here. We can try adding exceptions for the more common inline tags that are before a block tag where both are inside another block tag, but that will be quite slow and complex, and will have additional edge cases.

This looks quite similar to the nested block tags problems in autop. More cases are tracked in #38656. Perhaps better to close it as duplicate.

#3 @pbearne
8 months ago

I suspected that adding span to the blocks wouldn't be the best fix

Can we keep the unit test?

@pbearne
8 months ago

patch and test

#4 follow-up: @pbearne
8 months ago

  • Keywords has-patch added
  • Summary changed from wpautop add a extra </p> to wpautop adds a extra </p>

I had a quick look at the function again and saw that where are handling <option> to fix the same problem so I have been able to add similar code to strip line breaks before and after <span> elements and this works

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


8 months ago

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


6 months ago

#7 @pbearne
6 months ago

@dd32 can push into the next 4.7.x as well as #39307

#8 @dd32
6 months ago

  • Owner set to azaozz
  • Status changed from new to reviewing

#9 in reply to: ↑ 4 @azaozz
6 months ago

  • Keywords has-unit-tests has-patch removed

Replying to pbearne:

I had a quick look at the function again and saw that where are handling <option> to fix the same problem so I have been able to add similar code to strip line breaks before and after <span> elements

As I said above this will introduce numerous regressions. All spans that are not immediately inside a block element will be affected. Also all <br> around spans will be gone. Couple of cases:
1 <span>2</span> 3 will render as 123 instead of 1 2 3

1 <span>2</span>
3

will render as 123 instead of

1 2
3

I still don't see a good way to fix this apart from reverting [38592].

I had a quick look at the function again and saw that where are handling <option> to fix the same problem

Yes, that works for <option> as it is only allowed in <select> and there can't be any text around it.

Last edited 6 months ago by azaozz (previous) (diff)

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


6 months ago

Note: See TracTickets for help on using tickets.