WordPress.org

Make WordPress Core

Opened 7 years ago

Last modified 8 months ago

#14674 reopened defect (bug)

HR destroys HTML

Reported by: thomask Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Formatting Keywords: has-patch needs-testing wpautop
Focuses: Cc:

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 (5)

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

Download all attachments as: .zip

Change History (22)

#1 @dd32
7 years ago

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

@solarissmoke
7 years ago

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

#2 @solarissmoke
7 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
5 years ago

  • Keywords wpautop added

#4 @MikeHansenMe
5 years ago

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

@solarissmoke
5 years ago

Refreshed

#5 @solarissmoke
5 years ago

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

#6 @MikeHansenMe
5 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
5 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
2 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
2 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
2 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
2 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
2 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
2 years ago

  • Keywords wpautop added

#14 @MattyRob
12 months 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 12 months ago by MattyRob (previous) (diff)

@MattyRob
12 months ago

#15 @MattyRob
8 months 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
8 months ago

#16 @dd32
8 months 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
8 months ago

#17 @MattyRob
8 months 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?

Note: See TracTickets for help on using tickets.