WordPress.org

Make WordPress Core

Opened 10 months ago

Closed 8 months ago

#39307 closed defect (bug) (fixed)

</p> tag inserted in <figure>

Reported by: sujag Owned by: azaozz
Milestone: 4.7.3 Priority: normal
Severity: normal Version: 4.7
Component: Formatting Keywords: needs-testing fixed-major
Focuses: Cc:

Description

since wordpress 4.7 HTML entered in "text" mode results in invalid result. Stray </p> get's inserted into <figure> after image-Element.

post content

<figure><img src="/wp-content/uploads/example.jpg" alt="something" /><figcaption>Caption</figcaption></figure>

generated markup note wrong </p> after <img>

<figure><img src="/wp-content/uploads/example.jpg" alt="something" /></p>
<figcaption>Caption</figcaption>
</figure>

Up to 4.6 this could be avoided by writing the whole figure-Element in one line, this workaround doesn't help anymore in 4.7

Attachments (3)

39307.patch (1.3 KB) - added by azaozz 10 months ago.
Autop-extra-p-in-figure.php.patch (941 bytes) - added by pbearne 10 months ago.
39307.2.patch (1.6 KB) - added by azaozz 10 months ago.

Download all attachments as: .zip

Change History (26)

#1 @azaozz
10 months ago

Hi @sujag, thanks for the bug report.

Confirmed. Caused by [38592], see #4857. Without that change it inserts a <br> which is still a bug but at least doesn't visually alter the output (single closing </p> is converted to empty <p></p> in the browsers).

#2 @pento
10 months ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 4.7.1

I think this needs to be fixed with an exception in wpautop(), similar to object tags - the assumption is that flow elements inside of block elements (in this case, <img> inside <figure>), need to be wrapped in a <p>, but that's not the normal use case for <figure>, which is usually just an <img> and <figcaption> (in either order).

Things get a little weird with spacing inside of block elements (see #38656), the tests for this should ensure that fun combinations of spacing and ordering don't make it go haywire.

#3 @azaozz
10 months ago

Yeah, think we need an exception here too.

Perhaps can have another "group" of tags that should not be wrapped in <p> and require specific parent tags. Candidates for there would be figcaption, track, source, param.

@azaozz
10 months ago

#4 @azaozz
10 months ago

In 39307.patch: Collapse line breaks inside <figure>, and before and after <figcaption>.

This will effectively remove any <p> inside of <figure>, above or below <figcaption> (which are allowed but very rarely used). These weren't working properly before 4.7, so don't expect it to cause any regressions. The behavior is identical to <object> containing <embed> and/or <param> elements.

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

#5 @azaozz
10 months ago

  • Keywords needs-patch removed

#6 @pbearne
10 months ago

I am seeing similar problem in this ticket https://core.trac.wordpress.org/ticket/39377

#7 @pbearne
10 months ago

  • Keywords needs-unit-tests removed

I have added a test for this while I was creating one the other ticket

#8 @pbearne
10 months ago

  • Keywords has-unit-tests has-patch added

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


10 months ago

@azaozz
10 months ago

#11 @azaozz
10 months ago

In 39307.2.patch: similar to 39307.patch but does not remove line breaks from directly inside <figure> tags. This is closer to the previous behavior.

Still needs testing with older posts containing these tags to confirm there are no regressions.

#12 @azaozz
10 months ago

  • Milestone changed from 4.7.1 to 4.7.2

Seems better to try to test this more before adding in a dot release.

#13 @azaozz
9 months ago

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

In 39912:

Formatting: fix wpautop() to stop adding paragraph tags around <figcaption>.

Fixes #39307 for trunk.

#14 @azaozz
9 months ago

  • Keywords needs-testing added; has-unit-tests has-patch removed

Reopen for 4.7.2 consideration.

#15 @azaozz
9 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#16 follow-up: @pbearne
9 months ago

@azaozz do you need new unit tests for this?

#17 in reply to: ↑ 16 @azaozz
9 months ago

Replying to pbearne:
Was looking at "unifying" the tests for figcaption, track, source, param, as they are almost identical. But probably better to keep it simple and easier to read :)

#18 @azaozz
9 months ago

In 39914:

Tests: wpautop() should not add extra </p> before <figcaption>.

Props pbearne.
See #39307.

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


9 months ago

#20 @jbpaul17
9 months ago

  • Keywords fixed-major added

Per @jnylen0 in today's bug scrub in #core: Adding fixed-major keyword as this ticket has a couple of commits already.

#21 follow-up: @dd32
8 months ago

@azaozz Is this clear for 4.7.x? or is extra work and/or testing needed?

#22 in reply to: ↑ 21 @azaozz
8 months ago

Replying to dd32:
Yes, should be good for 4.7.x. There are still couple of very rare cases introduced by [38592], but they are much harder to fix.

#23 @dd32
8 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 40091:

Formatting: fix wpautop() to stop adding paragraph tags around &lt;figcaption&gt;.

Props azaozz, pbearne for tests.
Merges [39912], [39914] to the 4.7 branch.
Fixes #39307 for 4.7.

Note: See TracTickets for help on using tickets.