Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#3801 closed Bug (fixed)

IE: Comments at the start of the document get lost

Reported by: Frederico Caldeira Knabben Owned by: Tobiasz Cudnik
Priority: Normal Milestone: CKEditor 3.0
Component: General Version: SVN (CKEditor) - OLD
Keywords: Confirmed IE Review+ Cc:

Description

When loading the following HTML in IE, the comments get lost:

<!-- Test -->
<p>Contents</p>

Attachments (6)

3801.patch (1.4 KB) - added by Tobiasz Cudnik 8 years ago.
3801_2.patch (1.7 KB) - added by Tobiasz Cudnik 8 years ago.
3801_3.patch (3.4 KB) - added by Tobiasz Cudnik 8 years ago.
3801_4.patch (4.4 KB) - added by Tobiasz Cudnik 8 years ago.
3801_5.patch (4.3 KB) - added by Tobiasz Cudnik 8 years ago.
3801_6.patch (3.2 KB) - added by Tobiasz Cudnik 8 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 8 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik
Status: newassigned

comment:2 Changed 8 years ago by Tobiasz Cudnik

The reason for this is filtering data via innerHTML

var div = document.createElement( 'div' );
div.innerHTML = "<!-- foo --><p>bar</p>";
console.log( div.innerHTML );

Result of above code will be

<P>bar</P>

Such filtering takes place in htmldataprocessor plugin.

Changed 8 years ago by Tobiasz Cudnik

Attachment: 3801.patch added

comment:3 Changed 8 years ago by Tobiasz Cudnik

Keywords: Review? added

This may be not the most beautiful solution, but it works and i think that there may not be any other (besides dropping use of innerHTML of course).

comment:4 Changed 8 years ago by Garry Yao

Keywords: Review- added; Review? removed

It's a nice fix, but please consider head whitespace and I think the CKEDITOR.dom.element::setHtml should be a better place for the fix to happen.

comment:5 Changed 8 years ago by Garry Yao

Another tiny problem with the descriptional comment:
IE: Comments at the start of the document get lost.(#3801)
in favor of
#3801 IE: Comments at the start of the document get lost

comment:6 Changed 8 years ago by Tobiasz Cudnik

No problem with the comment, but i don't understand which "head whitespace" you had on mind. We can't use whitespace instead of "a" (or other char) because it doesn't have same result. If there are whitespaces before the HTML comment, they don't get lost (should they?).

Besides i'm not convinced that CKEDITOR.dom.element::setHtml is better place because of 2 reasons:

  1. This part of htmldataprocessor plugin doesn't use this method at all right now.
  2. Is HTML comment support really needed in CKEDITOR.dom.element::setHtml ?

What i see as a good move for next patch is moving this code to separate function like others, eg: extendBlockForDisplay or blockNeedsExtension. Also applying the fix only when HTML comment is present at the beginning of data could have sense, but i'm not sure that it's really needed (would have impact on performance).

What do you think ?

comment:7 Changed 8 years ago by Tobiasz Cudnik

I've gave wrong example, instead of extendBlockForDisplay and blockNeedsExtension it should be protectElementsNames or protectSelfClosingElements.

Changed 8 years ago by Tobiasz Cudnik

Attachment: 3801_2.patch added

comment:8 Changed 8 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

I've moved fix into separate function, which made things cleaner.

I'm afraid that moving it to CKEDITOR.dom.element::setHtml is impossible, because fix is acceptable only when we're getting content back right away. We can't modify content set by setHtml and such is needed by the fix.

comment:9 Changed 8 years ago by Artur Formella

Keywords: Review- added; Review? removed

This bug is bigger.

Please try the following source:

<p><!-- foo -->a</p>

result: foo is missing

but second "foo" works:

<p><!-- foo -->a</p>
<p><!-- foo -->a</p>

I think the reason is the same as in htmldataprocessor plugin - I mean:

div.innerHTML = data; 
data = div.innerHTML;

Changed 8 years ago by Tobiasz Cudnik

Attachment: 3801_3.patch added

comment:10 Changed 8 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

Good point Artur. I've took different approach in third patch, which conforms with other protect functions and is more elegant. There is also fixForBody addition, which i think should be placed in DTD, but since it's automatically generated, for now i've placed that as additional condition.

I will provide a TC for this issue soon.

comment:11 Changed 8 years ago by Garry Yao

Keywords: Review- added; Review? removed

It's great to see the desired way for fixing this, but the patch could be further simplified:

  1. A function creation could be avoid by merging the protection logic of comments with protectStyleTags, and same thing may happen to protectStyleTagsRegex;
  2. A similar fix to [3416] should be used to strip xml namespace to avoid changing the parser codes.

Changed 8 years ago by Tobiasz Cudnik

Attachment: 3801_4.patch added

comment:12 Changed 8 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

This patch reduces code amount by merging existing functions and reusing dom.element object.

TC is included.

comment:13 Changed 8 years ago by Artur Formella

Keywords: Review- added; Review? removed

Almost perfect.

Unrelated line:

//                      var div = document.createElement( 'div' );

I would also replace line

// IE remvoes style tags an comments from innerHTML. (#3710 & ...).

with

// IE removes style tags an comments from innerHTML. (#3710 & #3801 ).

Changed 8 years ago by Tobiasz Cudnik

Attachment: 3801_5.patch added

comment:14 Changed 8 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

Thanks for pointing out the details, i've fixed them.

comment:15 Changed 8 years ago by Garry Yao

Keywords: Review- added; Review? removed

Running the TC entry test_innerHtmlComments_ticket_3801_1 is making browser running into infinite loop.

comment:16 Changed 8 years ago by Tobiasz Cudnik

Can you please tell a bit more about this infinitive loop ? Browser and stack trace would be very helpful. Mostly because after running this test in all browsers (including IE quirks modes) i haven't noticed any issue (besides that other tests are failing sometime).

comment:17 Changed 8 years ago by Frederico Caldeira Knabben

I still find the very first patch the best approach for it. It's simple and performs well. Actually, we could even avoid making any kind of checks, just applying the "a" addition for all browsers with any content. After all substr(1) should perform well.

Changed 8 years ago by Tobiasz Cudnik

Attachment: 3801_6.patch added

comment:18 in reply to:  9 Changed 8 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

I'm attaching a patch with first solution applied for all browsers. As it turns out it's not affected by nested comment issue mentioned by Artur.

I've also added one new TC entry.

comment:19 Changed 8 years ago by Garry Yao

Keywords: Review+ added; Review? removed

comment:20 Changed 8 years ago by Tobiasz Cudnik

Resolution: fixed
Status: assignedclosed

Fixed with [3831].

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