Opened 10 years ago
Closed 10 years ago
#32078 closed defect (bug) (fixed)
TinyMCE View HTML Content in Shortcode
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (29)
#2
@
10 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.
#4
@
10 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]
#5
@
10 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
@
10 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.
#8
@
10 years ago
You mean with urlencode?
[toggle title="Toggle Title" content="%3Ch1%3ETitle+Lorem+ipsum%3C%2Fh1%3E"]
#12
@
10 years ago
The shortcode and its TinyMCE view works fine but it's not very readable for a user
#13
@
10 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.
10 years ago
#16
@
10 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.
#17
@
10 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.
#18
@
10 years ago
Should be fixed now. Let's use the same code for undo levels and getting the content.
#19
@
10 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
@
10 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.
#21
@
10 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
@
10 years ago
- Owner set to azaozz
- Resolution set to fixed
- Status changed from new to closed
In 32678:
Can you give us an example?