WordPress.org

Make WordPress Core

Opened 19 months ago

Closed 19 months ago

Last modified 18 months ago

#42531 closed defect (bug) (fixed)

Text is splitted into two paragraphs when switching editor tabs

Reported by: ocean90 Owned by: westonruter
Milestone: 4.9 Priority: normal
Severity: major Version: 4.9
Component: Editor Keywords: has-patch commit dev-reviewed
Focuses: javascript Cc:

Description

Probably introduced by #41962/#42029.

Steps to reproduce:

  • Go to the edit screen of a post
  • Set cursor in the middle of a sentence
  • Switch to text tab
  • Switch back to visual tab

Video: https://cloudup.com/cVoCok2lmNx

Expected behaviour: No additional paragraph is added.

Attachments (2)

42531.diff (9.4 KB) - added by adamsilverstein 19 months ago.
42531.2.diff (519 bytes) - added by pento 19 months ago.

Download all attachments as: .zip

Change History (25)

#1 @ocean90
19 months ago

@biskobe Is this something you can look into?

#2 @adamsilverstein
19 months ago

Introduced in r41783 according to git bisect.

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


19 months ago

#4 @afercia
19 months ago

Given also #42530 was introduced by the "keep selection" changes, and today it's the day before the 4.9 release, I'd be +1 for reverting the changes. They are just not tested enough and this is a lesson we should learn. FWIW, I wouldn't be so comfortable with shipping WordPress 4.9 with these 2 very evident regressions in a so critical area like the editor.

#5 @jbpaul17
19 months ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

#6 @adamsilverstein
19 months ago

42531.diff reverts r41783 and fixes the issue in my testing.

Given the size/scope of this change, and its recently discovered unintended consequences, and the timing of the 4.9 release, reverting probably makes the most sense here.

#7 @aduth
19 months ago

The issue appears to occur during the switch from Text to Visual mode, when parsing the textarea value, specifically here:

https://github.com/tinymce/tinymce/blob/7d7a8db05d55bd5078849c23a342993588ce9995/src/core/src/main/js/html/DomParser.js#L351-L352

With the changes introduced in r41783, a span with attribute data-mce-type="bookmark" is inserted into the textarea value when switching from Text to Visual modes, with intention of preserving the caret position.

https://github.com/WordPress/wordpress-develop/blob/f7dabb40c1293cb6490d4fc16e6c2b14cfbbc0e8/src/wp-admin/js/editor.js#L528

When parsing, TinyMCE considers three nodes: the text node before and after the span bookmark, and the span bookmark itself. With the logic of the DOMParser, it ends the first root block when encountering the span assigned a data-mce-type, thereby resulting in the creation of two root nodes (the two paragraphs from the report).

In brief testing, omitting the behavior to add the data-mce-type attribute when creating the cursor marker resolves the issue of two paragraphs being created.

diff --git src/wp-admin/js/editor.js src/wp-admin/js/editor.js
index ed77d3c755..140b258b6f 100644
--- src/wp-admin/js/editor.js
+++ src/wp-admin/js/editor.js
@@ -525,7 +525,7 @@ window.wp = window.wp || {};
                                mode = htmlModeCursorStartPosition !== htmlModeCursorEndPosition ? 'range' : 'single',
 
                                selectedText = null,
-                               cursorMarkerSkeleton = getCursorMarkerSpan( $$, '' ).attr( 'data-mce-type','bookmark' );
+                               cursorMarkerSkeleton = getCursorMarkerSpan( $$, '' );
 
                        if ( mode === 'range' ) {
                                var markedText = textArea.value.slice( htmlModeCursorStartPosition, htmlModeCursorEndPosition ),

#8 @afercia
19 months ago

Playing with text, switching multiple times, re-unifying the two split lines in just one line, then switching again visual/text etc. I also randomly get a span in the content. Using Chrome and Safari:
This i<span style="font-size: 1rem;">s a sentence</span>

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


19 months ago

#10 @peterwilsoncc
19 months ago

I'm +1 on reverting with 42531.diff too.

@pento
19 months ago

#11 @pento
19 months ago

  • Keywords has-patch added; needs-patch removed

42531.2.diff fixes this bug. autop() needed to wrap single line posts in <p> tags, so that, as @aduth noted, TinyMCE doesn't try to wrap the text nodes in <p> tags as it parses the post DOM.

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


19 months ago

#13 @westonruter
19 months ago

  • Keywords needs-testing added
  • Owner changed from adamsilverstein to peterwilsoncc
  • Status changed from assigned to reviewing

#14 @peterwilsoncc
19 months ago

42531.2.diff is doing what it says on the pack.

No paragraph is added switching between tabs.

#15 @peterwilsoncc
19 months ago

  • Keywords needs-testing removed

#16 follow-up: @dd32
19 months ago

  • Keywords needs-testing added

It looks like the code in 42531.2.diff is really only an optimisation, to skip processing text when not needed (as TinyMCE will wrap it in a paragraph tag anyway).
Removing that shouldn't cause any issues from what I can see, +1 from me.

#17 @dd32
19 months ago

  • Keywords needs-testing removed

Yay for trac showing warnings off screen :) (removing tag)

#18 @westonruter
19 months ago

  • Keywords commit dev-reviewed added
  • Owner changed from peterwilsoncc to westonruter
  • Status changed from reviewing to accepted

#19 @peterwilsoncc
19 months ago

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

In 42177:

Editor: Cease adding paragraphs when switching editor tabs.

Fixes an error since [41783] in which extra paragraphs could be added to content when switching editor tabs.

Props pento, adamsilverstein, aduth.
Fixes #42531.

#20 @westonruter
19 months ago

In 42178:

Editor: Cease adding paragraphs when switching editor tabs.

Fixes an error since [41783] in which extra paragraphs could be added to content when switching editor tabs.

Props pento, adamsilverstein, aduth.
Fixes #42531 for 4.9.

#21 in reply to: ↑ 16 @aduth
19 months ago

Replying to dd32:

It looks like the code in 42531.2.diff is really only an optimisation, to skip processing text when not needed (as TinyMCE will wrap it in a paragraph tag anyway).
Removing that shouldn't cause any issues from what I can see, +1 from me.

Noting that there is a change in behavior after 42531.2.diff

Before:

switchEditors.wpautop( 'Foo' );
// "Foo"

After:

switchEditors.wpautop( 'Foo' );
// "<p>Foo</p>\n"

However, the "After" case now more closely mimics the PHP behavior (which has the same result). Still worth noting that there is a difference with potential impact.

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


19 months ago

#23 @aduth
18 months ago

Another effect of changes introduced in r42178:

Before:

switchEditors.wpautop( '' );
// ""

After:

switchEditors.wpautop( '' );
// "<br />\n"

In this instance, I'd consider it a regression, since the "Before" behavior matches that found in PHP's wpautop.

Note: See TracTickets for help on using tickets.