WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

#32078 closed defect (bug) (fixed)

TinyMCE View HTML Content in Shortcode

Reported by: bduclos Owned by: azaozz
Milestone: 4.2.3 Priority: normal
Severity: normal Version: 4.2
Component: TinyMCE Keywords: fixed-major has-patch
Focuses: javascript Cc:

Description

If the shortcode content contains HTML code, the TinyMCE View no longer works while it was working in 4.1.2

Attachments (4)

32078.patch (4.0 KB) - added by azaozz 3 years ago.
32078.2.patch (4.7 KB) - added by azaozz 3 years ago.
32078.3.patch (4.9 KB) - added by iseulde 3 years ago.
32078.4.patch (5.8 KB) - added by azaozz 3 years ago.

Download all attachments as: .zip

Change History (29)

#1 @iseulde
3 years ago

  • Keywords reporter-feedback added

Can you give us an example?

#2 @chrisvanpatten
3 years ago

I don't have an example on hand (this isn't my ticket, just saw it on Slack!) but I know this has been raised as an issue for Shortcake/Shortcode UI as well.

According to that comment, this is the commit that broke things.

Edit: here's the non-GitHub changeset link.

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

#3 @iseulde
3 years ago

Can someone give steps to reproduce, please? :)

#4 @bduclos
3 years ago

For example, the following shortcode contains p tags inside its inner content which in visual mode breaks the TinyMCE view

[toggle title="Toggle Title"]<p>Lorem ipsum</p>[/toggle]

while this one without HTML tags works:

[toggle title="Toggle Title"]Title Lorem ipsum[/toggle]
Last edited 3 years ago by iseulde (previous) (diff)

#5 @iseulde
3 years ago

  • Focuses javascript added
  • Keywords needs-patch added; reporter-feedback removed
  • Milestone changed from Awaiting Review to Future Release
  • Type changed from enhancement to defect (bug)

Confirmed. But this didn't really work well in 4.1 either. The matched content is [toggle title="Toggle Title"]↵<p>Lorem ipsum</p>↵<p>[/toggle] on the first switch, and [toggle title="Toggle Title"]Lorem ipsum</p>↵<p>[/toggle] the second time.

Now the markers are not placed correctly:

<p data-wpview-marker="%5Btoggle%20title%3D%22Toggle%20Title%22%5D%3C%2Fp%3E%0A%3Cp%3ELoremNaNpsum%3C%2Fp%3E%0A%3Cp%3E%5B%2Ftoggle%5D">[toggle title="Toggle Title"]</p><p>Lorem ipsum</p><p>[/toggle]</p>

For now I'd recommend that you use an attribute to store the content.

#6 @bduclos
3 years ago

"For now I'd recommend that you use an attribute to store the content." how would you do it?
If I try the following code the TinyMCE View is broken, it seems there's no way to use HTML in a shortcode.

[toggle title="Toggle Title" content="<h1>Title Lorem ipsum</h1>"]


There is also an issue if the shortcode is written on multiple lines
e.g if in text mode your shortcode is :

[audio mp3="https://wordpress.org/news/files/2014/04/AintMisbehavin.mp3"]
[/audio]

instead of

[audio mp3="https://wordpress.org/news/files/2014/04/AintMisbehavin.mp3"][/audio]

when you switch to Visual Mode, the TinyMCE View is broken while it was working in 4.1.

#7 @iseulde
3 years ago

Encode it?

#8 @bduclos
3 years ago

You mean with urlencode?

[toggle title="Toggle Title" content="%3Ch1%3ETitle+Lorem+ipsum%3C%2Fh1%3E"]

#9 @iseulde
3 years ago

Yes, that should work.

#10 @bduclos
3 years ago

Yes it works but it becomes a bit messy when you switch to Text mode.

#11 @iseulde
3 years ago

What does the shortcode do? Didn't you have problems with autop before?

#12 @bduclos
3 years ago

The shortcode and its TinyMCE view works fine but it's not very readable for a user

@azaozz
3 years ago

#13 @azaozz
3 years ago

  • Keywords has-patch needs-testing added; needs-patch removed
  • Milestone changed from Future Release to 4.2.3

32078.patch fixes it here. Also fixes a typo in /media/views/frame/video-details.js and prevents nested <p> tags on inserting the markers.

This ticket was mentioned in Slack in #core-editor by azaozz. View the logs.


3 years ago

#15 @iseulde
3 years ago

@azaozz Using this.shortcode assumes the text is a shortcode though?

#16 @azaozz
3 years ago

Yes, there we are checking if the user changed the "view text" while waiting for the view data to be fetched with ajax. However shortcodes get placeholders, the view text is not exposed so the user cannot change it. Actually most views would get a placeholder, can probably test for that instead of excluding shortcodes.

@azaozz
3 years ago

#17 @iseulde
3 years ago

In the patch above there are two functions with the same name. If you fix it, there will be problems with the undo levels.

Also not sure about return content.replace( /<p>\s*<p data-wpview-marker=/g, '<p data-wpview-marker=' ).replace( /<\/p>\s*<\/p>/g, '</p>' );. This should only be an issue if the user adds the markup? If we remove the correction in the other patch with improvements, we shouldn't add it back here.

@iseulde
3 years ago

#18 @iseulde
3 years ago

Should be fixed now. Let's use the same code for undo levels and getting the content.

#19 @iseulde
3 years ago

Quick summary

We always leave the shortcode in the content, so TinyMCE will parse it and generate the DOM. This is bad as invalid markup in the shortcode will be removed and the markers can be cut off when TinyMCE wraps HTML in P nodes. The solution is to replace the marker early for views that have a placeholder (default). Views that don't have a loader (rare) need to make sure the text is a string. There's also a second problem with entities when converting back. We replace the view with the text in the DOM, so any HTML will be escaped. The solution is to replace in the string, not the DOM.

#20 @azaozz
3 years ago

Looking at 32078.3.patch:

  • Thinking it's good to have the nested <p> removal regex when inserting the markers. This is currently fixed in the 'wordpress' MCE plugin: https://core.trac.wordpress.org/browser/branches/4.2/src/wp-includes/js/tinymce/plugins/wordpress/plugin.js#L321 but why should we be inserting invalid HTML in the first place.
  • We should keep emptying the view nodes at the beginning of the TinyMCE DOM cleanup. This makes it faster and fixes some edge cases where different "filters" on the content are triggered by the view nodes or serialized HTML, sometimes resulting in errors.
Last edited 3 years ago by azaozz (previous) (diff)

@azaozz
3 years ago

#21 @azaozz
3 years ago

In 32078.4.patch: rename the functions to be more descriptive, fix the regex that removes the views HTML before adding undo level so it removes emptied views too.

These patches exposed another error: on show MCE adds another undo level with the current editor content. As we have emptied the view nodes on hide (to stop video/audio players, etc.), the DOM in the hidden editor contains only the view wrappers. This seems like a good way to keep the encoded view text (so any invalid HTML is not removed) and let MCE make a useful undo level that can be used to undo any changes made in the Text editor.

#22 @azaozz
2 years ago

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

In 32678:

TinyMCE wpView: preserve HTML wrapped in shortcodes.
Fixes #32078.

#23 @azaozz
2 years ago

  • Keywords fixed-major added; needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopen for 4.2.

#24 @azaozz
2 years ago

#32214 was marked as a duplicate.

#25 @azaozz
2 years ago

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

In 33316:

TinyMCE wpView: preserve HTML wrapped in shortcodes.
Props iseulde, azaozz. Fixes #32078 for 4.2.

Note: See TracTickets for help on using tickets.