Opened 5 years ago

Closed 5 years ago

#11544 closed Bug (fixed)

Placeholders should not be upcasted in parents not accepting spans

Reported by: Marcus Bointon Owned by: Marek Lewandowski
Priority: Normal Milestone: CKEditor 4.3.4
Component: UI : Widgets Version: 4.3
Keywords: Cc:

Description (last modified by Piotrek Koszuliński)

New description

  1. Download and open 11544.html.
  2. Switch between modes.
  3. See that there's a mess in <title> tag.

Important note: While working on this issue other scenarios like placeholder used in <textarea> or <video> should be checked. Perhaps more parent tags should be excluded. The rule may be that placeholder may only be created in if, according to DTD, text node's parent accepts span.

Original description (incorrect)

This is related to #11223 and this forum post

I'm using the placeholder plugin in CKEditor 4.3.2, which generally works apart from two issues:

  • It doesn't allow it to be used in the root of the head tag (so for example you can't use placeholders to generate meta tags); placeholder get moved into the body tag.
  • Placeholder items in the title tag (and possibly elsewhere) get escaped incorrectly, and then fail to be unescaped on save, or when switching to/from source mode.

For example, starting by pasting this document into the source view:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
<html>
<head>
	<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
	<title>[[mytag1]]</title>
[[mytag2]]
</head>
<body>[[mytag3]]</body>
</html>

Switching to HTML view then back to source results in:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
<html>
<head>
	<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
	<title>&lt;span tabindex=&quot;-1&quot; contenteditable=&quot;false&quot; data-cke-widget-wrapper=&quot;1&quot; data-cke-filter=&quot;off&quot; class=&quot;cke_widget_wrapper cke_widget_new cke_widget_inline&quot; data-cke-display-name=&quot;placeholder&quot;&gt;&lt;span class=&quot;cke_placeholder&quot; data-cke-widget-keep-attr=&quot;0&quot; data-widget=&quot;placeholder&quot;&gt;[[mytag1]]&lt;/span&gt;&lt;/span&gt;</title>
</head>
<body>[[mytag2]][[mytag3]]</body>
</html>

You can see an escaping wrapper has been applied in the title tag, and the [[mytag]] element in the head tag has been moved into the body.

Doing the same switch again results in:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
<html>
<head>
	<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
	<title>&lt;span tabindex=&quot;-1&quot; contenteditable=&quot;false&quot; data-cke-widget-wrapper=&quot;1&quot; data-cke-filter=&quot;off&quot; class=&quot;cke_widget_wrapper cke_widget_new cke_widget_inline&quot; data-cke-display-name=&quot;placeholder&quot;&gt;&lt;span class=&quot;cke_placeholder&quot; data-cke-widget-keep-attr=&quot;0&quot; data-widget=&quot;placeholder&quot;&gt;&lt;span tabindex=&quot;-1&quot; contenteditable=&quot;false&quot; data-cke-widget-wrapper=&quot;1&quot; data-cke-filter=&quot;off&quot; class=&quot;cke_widget_wrapper cke_widget_new cke_widget_inline&quot; data-cke-display-name=&quot;placeholder&quot;&gt;&lt;span class=&quot;cke_placeholder&quot; data-cke-widget-keep-attr=&quot;0&quot; data-widget=&quot;placeholder&quot;&gt;[[mytag1]]&lt;/span&gt;&lt;/span&gt;&lt;/span&gt;&lt;/span&gt;</title>
</head>
<body>[[mytag2]][[mytag3]]</body>
</html>

It's now double-escaped, and this repeats each time you switch views or save.

For me this is a complete showstopper; I can't use CKEditor if it causes content explosions.

Attachments (1)

11544.html (888 bytes) - added by Piotrek Koszuliński 5 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 5 years ago by Piotrek Koszuliński

Keywords: placeholder source removed
Milestone: CKEditor 4.3.4
Status: newconfirmed
Version: 4.3.24.3

Placeholders cannot be used in <head>. You have to load correct HTML and <head> cannot contain text content. You could solve this with config.protectedSource but you'll need to use different pattern than for normal placeholders.

So the broken scenario is placeholder's format used in the <title>. See 11544.html. I also used there protectedSource to check if it correctly works in the <head> tag and everything looks fine. What's not fine is a mess in <title>.

While working on this issue other scenarios like placeholder used in <textarea> or <video> should be checked. Perhaps more parent tags should be excluded.

Last edited 5 years ago by Piotrek Koszuliński (previous) (diff)

Changed 5 years ago by Piotrek Koszuliński

Attachment: 11544.html added

comment:2 Changed 5 years ago by Marcus Bointon

That example HTML had a couple of problems, but I turned it into a minimal jsfiddle that reproduces the problem; just toggle the source button.

Note that I disabled protectedSource - placeholder alone triggers the issue.

comment:3 Changed 5 years ago by Piotrek Koszuliński

Description: modified (diff)
Summary: Wrong escaping in title tag, moving tags out of head with placeholdersPlaceholders should not be upcasted in parents not accepting spans

I know that placeholder alone triggers this issue. I wanted to show in 11544.html that if you want to use non-HTML strings in the <head> section you need to use protectedSource. Otherwise those strings will be pulled out to the <body>, which is a correct behaviour.

comment:4 Changed 5 years ago by Piotrek Koszuliński

Description: modified (diff)

comment:5 Changed 5 years ago by Marcus Bointon

Yes, thanks for pointing that out - I will try to think of a way to pre and post-filter my content in <head> to stop it being moved about. Does the jsfiddle work for you?

comment:6 Changed 5 years ago by Piotrek Koszuliński

Yes, your jsffidle clearly shows the issue.

comment:7 Changed 5 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

comment:8 Changed 5 years ago by Marek Lewandowski

Status: assignedreview

Pushed to t/11544 at dev and t/11544 at tests.

comment:9 Changed 5 years ago by Piotrek Koszuliński

Status: reviewreview_passed

Force pushed branches because:

  1. They were based on major instead of master.
  2. If non standard editor was used in generic tests its good to initialize it in async:init, what won't require changes when other setups will be needed.
  3. Fixed the case with custom elements.

comment:10 Changed 5 years ago by Marek Lewandowski

Resolution: fixed
Status: review_passedclosed

Fixed with git:8b8b2af2bd (merged to master) at dev and 677dc1a1bc (merged to master) at tests.

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy