IE: Comments at the start of the document get lost

[fredck]

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

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

[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.

[tobiasz.cudnik]

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).

[garry.yao]

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.

[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

[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 ?

[tobiasz.cudnik]

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

[tobiasz.cudnik]

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.

[arczi]

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;

[tobiasz.cudnik]

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.

[garry.yao]

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.

[tobiasz.cudnik]

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

TC is included.

[arczi]

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 ).

[tobiasz.cudnik]

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

[garry.yao]

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

[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).

[fredck]

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.

[tobiasz.cudnik]

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.

[tobiasz.cudnik]

Fixed with [3831].