Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#3801 closed Bug (fixed)

IE: Comments at the start of the document get lost

Reported by: fredck 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 7 years ago.
3801_2.patch (1.7 KB) - added by tobiasz.cudnik 7 years ago.
3801_3.patch (3.4 KB) - added by tobiasz.cudnik 7 years ago.
3801_4.patch (4.4 KB) - added by tobiasz.cudnik 7 years ago.
3801_5.patch (4.3 KB) - added by tobiasz.cudnik 7 years ago.
3801_6.patch (3.2 KB) - added by tobiasz.cudnik 7 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 7 years ago by tobiasz.cudnik

  • Owner set to tobiasz.cudnik
  • Status changed from new to assigned

comment:2 Changed 7 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 7 years ago by tobiasz.cudnik

comment:3 Changed 7 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 7 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 7 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 7 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 7 years ago by tobiasz.cudnik

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

Changed 7 years ago by tobiasz.cudnik

comment:8 Changed 7 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 follow-up: Changed 7 years ago by arczi

  • 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 7 years ago by tobiasz.cudnik

comment:10 Changed 7 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 7 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 7 years ago by tobiasz.cudnik

comment:12 Changed 7 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 7 years ago by arczi

  • 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 7 years ago by tobiasz.cudnik

comment:14 Changed 7 years ago by tobiasz.cudnik

  • Keywords Review? added; Review- removed

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

comment:15 Changed 7 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 7 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 7 years ago by 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.

Changed 7 years ago by tobiasz.cudnik

comment:18 in reply to: ↑ 9 Changed 7 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 7 years ago by garry.yao

  • Keywords Review+ added; Review? removed

comment:20 Changed 7 years ago by tobiasz.cudnik

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

Fixed with [3831].

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