Ticket #8732 (closed Bug: fixed)

Opened 3 years ago

Last modified 3 years ago

CKEditor creating hidden spans which can result in hidden content

Reported by: duncansimey Owned by: fredck
Priority: Normal Milestone: CKEditor 3.6.3
Component: Core : Styles Version: 3.5.1
Keywords: IE Cc:

Description

One of our customers reported instances of text that looked OK when they typed it in to CKEditor, but was missing when the edited HTML was later included in reports generated by our application. Investigation showed their text was inside spans with style display:none We've worked out how to reproduce the problem with Firefox and IE (only checked IE7 onwards), see below.

The text inside the display:none spans is only displayed inside CKEditor on some browsers (e.g. IE9 in IE7 document mode). If the person entering the text has one of these browsers then there is no indication that there is a problem. The problem can be reproduced on 3.6.2 Demo and the Nightly Build demo.

To reproduce, do the following.... If using IE, turn on JavaScript Alerts using Advanced Internet Options. Paste the following into the editor while in Source mode.

<!-- Start of Text -->
<p>Testing</p>
<p>One</p>
<p>Two</p>
<p>Three</p>
<!-- End of Text -->

Go back to WYSIWYG mode. Select the word Testing and use the toolbar to set a background colour; in IE you get a JavaScript error (note the location of the problem). Select the word Two and use the toolbar to set a background colour; it works fine. Select the word Three and use the toolbar to set a background colour; in IE you get a JavaScript error (the error details indicate a different location to the first error because it is a different problem). Note: The problem with Three does not occur with Firefox.

Inspect the HTML in Source mode; it now contains weird hidden spans (see below). Clicking after the end of the word Testing and typing some text adds the text into the hidden span. Depending on the exact browser version, the typed text either gets displayed or hidden or sometimes does not go into the hidden span.

<!-- Start of Text -->
<p><span id="cke_bm_74S" style="display: none">&nbsp;</span>Testing<span id="cke_bm_74E" style="display: none">&nbsp;</span></p>
<p>One</p>
<p><span style="background-color: #ffff00">Two</span></p>
<p><span id="cke_bm_77S" style="display: none">&nbsp;</span>Three<span id="cke_bm_77E" style="display: none">&nbsp;</span></p>
<!-- End of Text -->

The example above used Background Colour but can be reproduced with any toolbar button which works on selected text (Bold, Font, etc). The HTML comments in the source are what is triggering the problem. During testing we have also observed other ways of producing JavaScript errors and have not yet worked out reproducible test cases. These include an error during paste and an error while changing the selection. Therefore there are at least two more causes of JavaScript errors.

The problem is being caused by CKEditor badly handling JavaScript errors. During command execution the source HTML is marked up with hidden spans indicating the start and end of the selected area. These are being added at the start of the command, but are not being tidied up because the script is being aborted by the error. If this was a simple bit of code, then I'd suggest adding a try/catch block where the exception handler cleans up the hidden spans. Whatever solution is proposed, it needs to be generic enough to cope with a range of problems including ones we have not yet worked out how to repeatably demonstrate!

There is a related ticket #8232 (Inline styles create empty garbage "cke_bm_" spans). This fixed one specific cause of JavaScript error. It did not fix the problem described on that ticket by fastballweb in comment 2. Nor did it fix the general problem of tidying up after exceptions.

Attachments

8732.patch (658 bytes) - added by fredck 3 years ago.

Change History

comment:1 Changed 3 years ago by j.swiderski

  • Keywords IE Firefox added
  • Status changed from new to confirmed
  • Version changed from 3.6.2 to 3.5.1

First Problem: JavaScript errors caused by comments.

Reproducible in IE6-9 and Firefox3.6-10 from CKEditor 3.5.1
Message: sibling.is is not a function
URI: /3.6.2/ckeditor/_source/core/dom/range.js
Line: 1032

Reproducible in IE6-9 only from CKEditor 3.5.1 rev [6391] Message: Object doesn't support property or method "is"
URI: /3.6.2/ckeditor/_source/core/dom/range.js
Line: 1192


Second problem: As a result of errors invisble spans are inserted

comment:2 Changed 3 years ago by j.swiderski

Similar problem: #8569

Other related to problems with HTML comments: #8640, #8216, #8129, #6709

Version 0, edited 3 years ago by j.swiderski (next)

comment:3 Changed 3 years ago by j.swiderski

#8687 and #8758 were marked as dulicates.

Last edited 3 years ago by j.swiderski (previous) (diff)

comment:4 Changed 3 years ago by j.swiderski

Another TC from #8687
Paste <p><!-- BUG -->loren ipsum</p>, go back to WYSIWYG, press ctrl+A, ctrl+B (for bold)

A quick fix propoesed in #8687: URI: core/dom/range.js,
method: enlarge()

if ( ( sibling.$.offsetWidth > 0 || excludeBrs && sibling.is && sibling.is( 'br' ) ) && !sibling.data( 'cke-bookmark' ) )

comment:5 Changed 3 years ago by fredck

  • Status changed from confirmed to closed
  • Resolution set to worksforme

I'm not able to reproduce this issue any more with the current trunk.

comment:6 follow-up: ↓ 8 Changed 3 years ago by j.swiderski

  • Status changed from closed to reopened
  • Keywords Firefox removed
  • Resolution worksforme deleted

This issue is still reproducible in IE

  1. Paste the below
    <!-- Start of Text -->
    <p>Testing</p>
    <p>One</p>
    <p>Two</p>
    <p>Three</p>
    <!-- End of Text -->
    
  2. Change background color of word Testing and then try to change background color of word Three

You will get:
Reproducible in IE6-9 only from CKEditor 3.5.1 rev [6391] Message: Object doesn't support property or method "is"
URI: /trunk/ckeditor/_source/core/dom/range.js
Line: 1198

comment:7 Changed 3 years ago by j.swiderski

  • Status changed from reopened to confirmed

comment:8 in reply to: ↑ 6 Changed 3 years ago by fredck

Replying to j.swiderski:

This issue is still reproducible in IE

Confirmed, but note that the tc you have reported is different than the original ticket tc and than its simplified tc.

Changed 3 years ago by fredck

comment:9 Changed 3 years ago by fredck

  • Owner set to fredck
  • Status changed from confirmed to review
  • Component changed from General to Core : Styles

comment:10 follow-up: ↓ 11 Changed 3 years ago by duncansimey

Fred - There are lots of ways to reproduce javascript problems. I noted we had spotted others I'd failed to reproduce reliably. Which is why I asked for a general try / catch type solution instead of quick fixes. I still think better exception handling is the way forward.

Last edited 3 years ago by duncansimey (previous) (diff)

comment:11 in reply to: ↑ 10 Changed 3 years ago by fredck

Replying to duncansimey:

Fred - There are lots of ways to reproduce javascript problems. I noted we had spotted others I'd failed to reproduce reliably. Which is why I asked for a general try / catch type solution instead of quick fixes. I still think better exception handling is the way forward.

I got your point, but the problem here is that, if really doable, this would involve a strong refactoring on good part of our code. We would have to create a "job-based" system that would revert the job in case of issues, reporting the error to the user (and eventually to us). That's all cool, but is something to consider in a dedicated ticket (#8825).

In this ticket instead, we have a specific issue identified. The system we just described would still not help here. It would just avoid the js error, but the user would still not be able to do what s/he was intended to do. So, a specific patch is needed.

comment:12 Changed 3 years ago by duncansimey

Thanks Fred. We will work out steps to create some of the other problems we have seen, so they can be fixed too.

Last edited 3 years ago by duncansimey (previous) (diff)

comment:13 Changed 3 years ago by garry.yao

  • Status changed from review to review_passed

comment:14 Changed 3 years ago by fredck

  • Status changed from review_passed to closed
  • Resolution set to fixed
  • Milestone set to CKEditor 3.6.3

Fixed with [7406].

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