WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 2 years 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 3 years ago.
42531.2.diff (519 bytes) - added by pento 3 years ago.

Download all attachments as: .zip

Change History (25)

#1 @ocean90
3 years ago

@biskobe Is this something you can look into?

#2 @adamsilverstein
3 years ago

Introduced in r41783 according to git bisect.

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


3 years ago

#4 @afercia
3 years 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
3 years ago

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

#6 @adamsilverstein
3 years 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
3 years 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
3 years 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.


3 years ago

#10 @peterwilsoncc
3 years ago

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

@pento
3 years ago

#11 @pento
3 years 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.


3 years ago

#13 @westonruter
3 years ago

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

#14 @peterwilsoncc
3 years ago

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

No paragraph is added switching between tabs.

#15 @peterwilsoncc
3 years ago

  • Keywords needs-testing removed

#16 follow-up: @dd32
3 years 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
3 years ago

  • Keywords needs-testing removed

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

#18 @westonruter
3 years ago

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

#19 @peterwilsoncc
3 years 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
3 years 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
3 years 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.


3 years ago

#23 @aduth
2 years 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.