WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 6 months ago

Last modified 5 months ago

#14674 closed defect (bug) (fixed)

HR destroys HTML

Reported by: thomask Owned by: pento
Milestone: 5.3 Priority: normal
Severity: normal Version: 3.0
Component: Formatting Keywords: has-patch wpautop has-unit-tests
Focuses: Cc:
PR Number:

Description

if you add <hr /> to the post (via HTML editor, or via enhanced TinyMCE editor) followed by normal text id do not add the <p> for that paragraph, but add the finishing </p>

so it than looks like

<p>some text</p>
<hr>some other text</p>

it can be solved by adding extra line break after <hr> but after reediting the post, the linebreak disapears and must be added again.

Attachments (6)

14674.diff (1.4 KB) - added by solarissmoke 9 years ago.
HR's are a special case of block level element because they self-close
14674.2.diff (960 bytes) - added by solarissmoke 7 years ago.
Refreshed
14674.3.diff (684 bytes) - added by MattyRob 3 years ago.
14674.4.diff (910 bytes) - added by MattyRob 3 years ago.
14674.5.diff (1.2 KB) - added by MattyRob 3 years ago.
14674.6.diff (1.3 KB) - added by pento 6 months ago.

Download all attachments as: .zip

Change History (26)

#1 @dd32
9 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

@solarissmoke
9 years ago

HR's are a special case of block level element because they self-close

#2 @solarissmoke
9 years ago

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

I think the problem was the wpautop was expecting to find a closing tag for the hr element, like it does with all other block-level elements. This patch fixes it for me but needs testing to be sure it doesn't break anything else.

#3 @WraithKenny
8 years ago

  • Keywords wpautop added

#4 @MikeHansenMe
7 years ago

  • Keywords needs-refresh needs-patch added; has-patch needs-testing removed

@solarissmoke
7 years ago

Refreshed

#5 @solarissmoke
7 years ago

  • Keywords has-patch added; needs-refresh needs-patch removed

#6 @MikeHansenMe
7 years ago

  • Keywords close added

not sure why I asked for refresh. This problem seems to be fixed in the current version 3.5-alpha-21751. Proposing we close the ticket.

#7 follow-up: @SergeyBiryukov
7 years ago

  • Keywords close removed

I can still reproduce in trunk.

  1. Copy this into Text editor:
    <p>some text</p> <hr />some other text
    
  2. Preview the post.
  3. View HTML source of the page:
    <p>some text</p>
    <hr />some other text</p>
    

#8 in reply to: ↑ 7 @chriscct7
5 years ago

  • Keywords wpautop has-patch removed
  • Milestone Future Release deleted
  • Resolution set to worksforme
  • Status changed from new to closed

Replying to SergeyBiryukov:

I can still reproduce in trunk.

  1. Copy this into Text editor:
    <p>some text</p> <hr />some other text
    
  2. Preview the post.
  3. View HTML source of the page:
    <p>some text</p>
    <hr />some other text</p>
    

This now works on 4.2. Closing as works for me.

#9 @SergeyBiryukov
5 years ago

  • Keywords wpautop has-patch added
  • Milestone set to Future Release
  • Resolution worksforme deleted
  • Status changed from closed to reopened

I can still reproduce comment:7 in 4.2 exactly as described.

#10 @chriscct7
5 years ago

This is wierd. I think I know why me and @MikeHansonMe can't replicate this.

Depending on the browser used, the browsers try to fix the error I think.

This is what I get in Chrome:

<p>some text</p>
<hr>some other text<p></p>

but this is what I get in FireFox:

<p>some text</p>
<hr></hr>some other text<p></p>

What browser are you using?

#11 follow-up: @SergeyBiryukov
5 years ago

Firefox 37.0.2.

Make sure to actually view the page source, not via Inspect Element. I think both Firebug and Chrome Developer Tools try to normalize invalid HTML.

#12 in reply to: ↑ 11 @chriscct7
5 years ago

  • Keywords needs-testing added; wpautop removed
  • Version changed from 3.0.1 to 3.0

Replying to SergeyBiryukov:

Firefox 37.0.2.

Make sure to actually view the page source, not via Inspect Element. I think both Firebug and Chrome Developer Tools try to normalize invalid HTML.

Ah I see now. Yep, that needs to be fixed.

Tagging patch as needs testing.

#13 @chriscct7
5 years ago

  • Keywords wpautop added

#14 @MattyRob
3 years ago

The current patch doesn't take into account the HTML <hr> tag as well XHTML <hr /> tag. Updated patch takes this into account using a preg_replace() call and also demonstrates using two consecutive str_replace calls (I'm not sure which will be faster).

In my testing it works fine. (Mac OS Browser as follows Safari 9.1.2, FF 48.0 and Chrome 58)

Last edited 3 years ago by MattyRob (previous) (diff)

@MattyRob
3 years ago

#15 @MattyRob
3 years ago

I coincidently found that the above patch (14674.3.diff) fails PHPUnit tests but I believe that this is because the unit tests are not accurate.

The test fails expecting <hr>foo</hr> but getting <hr>\n<p>foo</hr> however there should not be any content within <hr> tags as it's self closing.

Can the <hr> test simply be removed from the unit tests?

@MattyRob
3 years ago

#16 @dd32
3 years ago

Can the <hr> test simply be removed from the unit tests?

If the test is wrong, yes. If possible, it should be ensured another unit test shows the expected behaviour.

@MattyRob
3 years ago

#17 @MattyRob
3 years ago

@dd32

Update patch attached for the unit tests. Needs another set of eyes though as this is my first ever PHPUnit test! The test is passing but is it a valid test?

@pento
6 months ago

#18 @pento
6 months ago

  • Keywords has-unit-tests added; needs-testing removed
  • Milestone set to 5.3
  • Owner set to pento
  • Status changed from reopened to assigned

14674.6.diff Refreshes the patch to apply cleanly, and tweaks the behaviour to account for changes in how wpautop() behaves since this ticket was last active.

#19 @pento
6 months ago

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

In 45574:

Formatting: Add correct <p> tags near <hr> tags.

It can be tricky to know when wpautop() should add <p> tags, but one thing we can be certain about is that they really shouldn't be anywhere near <hr> tags.

Now they aren't.

Props solarissmoke, MattyRob, pento.
Fixes #14674.

#20 @pento
5 months ago

#28762 was marked as a duplicate.

Note: See TracTickets for help on using tickets.