#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)
Change History (26)
comment:1 Changed 15 years ago by
Owner: | set to Tobiasz Cudnik |
---|---|
Status: | new → assigned |
comment:2 Changed 15 years ago by
Changed 15 years ago by
Attachment: | 3801.patch added |
---|
comment:3 Changed 15 years ago by
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 15 years ago by
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 15 years ago by
comment:6 Changed 15 years ago by
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:
- This part of htmldataprocessor plugin doesn't use this method at all right now.
- 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 15 years ago by
I've gave wrong example, instead of extendBlockForDisplay and blockNeedsExtension it should be protectElementsNames or protectSelfClosingElements.
Changed 15 years ago by
Attachment: | 3801_2.patch added |
---|
comment:8 Changed 15 years ago by
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: 18 Changed 15 years ago by
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 15 years ago by
Attachment: | 3801_3.patch added |
---|
comment:10 Changed 15 years ago by
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 15 years ago by
Keywords: | Review- added; Review? removed |
---|
It's great to see the desired way for fixing this, but the patch could be further simplified:
- A function creation could be avoid by merging the protection logic of comments with protectStyleTags, and same thing may happen to protectStyleTagsRegex;
- A similar fix to [3416] should be used to strip xml namespace to avoid changing the parser codes.
Changed 15 years ago by
Attachment: | 3801_4.patch added |
---|
comment:12 Changed 15 years ago by
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 15 years ago by
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 15 years ago by
Attachment: | 3801_5.patch added |
---|
comment:14 Changed 15 years ago by
Keywords: | Review? added; Review- removed |
---|
Thanks for pointing out the details, i've fixed them.
comment:15 Changed 15 years ago by
Keywords: | Review- added; Review? removed |
---|
Running the TC entry test_innerHtmlComments_ticket_3801_1 is making browser running into infinite loop.
comment:16 Changed 15 years ago by
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 15 years ago by
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 15 years ago by
Attachment: | 3801_6.patch added |
---|
comment:18 Changed 15 years ago by
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 15 years ago by
Keywords: | Review+ added; Review? removed |
---|
The reason for this is filtering data via innerHTML
Result of above code will be
Such filtering takes place in htmldataprocessor plugin.