Opened 9 years ago

Closed 9 years ago

#1934 closed Bug (fixed)

Selection.EnsureSelection is broken for object selections

Reported by: Frederico Caldeira Knabben Owned by: Martin Kou
Priority: Normal Milestone: FCKeditor 2.6
Component: UI : Dialogs Version: FCKeditor 2.6 Beta
Keywords: Confirmed IE Review+ Cc:


Confirmed with IE7. Ok with FF.

Steps to Reproduce

  1. Insert an image (like a smiley).
  2. Remove the selection from the image and select it again.
  3. Open the "Insert Special Character" dialog.
  4. Insert any char.

A JavaScript error is thrown in Selection.EnsureSelection. There is no way to close the dialog after it. The page is now blocked.

Note that the desired action was correctly completed though.

Attachments (2)

1934.patch (976 bytes) - added by Martin Kou 9 years ago.
1934_2.patch (443 bytes) - added by Martin Kou 9 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 9 years ago by Martin Kou

Owner: set to Martin Kou
Status: newassigned

comment:2 Changed 9 years ago by Martin Kou

Two things are wrong here:

  1. After the special characters dialog has inserted a character, the saved selection should be changed so that FCKeditor wouldn't try to re-selected something that has been replaced.
  2. EnsureSelection() shouldn't give out JavaScript errors even if the saved selection data is invalid.

Changed 9 years ago by Martin Kou

Attachment: 1934.patch added

comment:3 Changed 9 years ago by Martin Kou

Keywords: Review? added

comment:4 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

This problem happens not only with the Special Char dialog. It happens with all dialogs that replace the current content. So, in step 3, if you insert a table, form field or even another smiley, you will have the same error. So, the fix on fck_specialchar.html exclusively is not enough.

Also, that fix in fck_specialchar.html looks like a dirty hack. It is not up to the inner dialog to do selection fixes at that point. There should be a tool function in dialog.html (like Selection.ResetSelectionData) that should be called by all dialogs that replace the selection.

comment:5 Changed 9 years ago by Martin Kou

Keywords: Discussion added; Review- removed

I've done some more tests but now it seems the fix in fck_specialchar.html doesn't make sense at all, nor is any Selection.ResetSelectionData() needed. It seems adding an error check to EnsureSelection() is already sufficient.

If you look at the sequence of events that happens when the special character dialog is being closed, you'll see why the fix doesn't make sense:

  1. User chooses a special character.
  2. EnsureSelection() is called to restore the selection in the editor window. (note that the dialog layout is not closed at this time)
  3. FCK.InsertHtml() is called to insert a special character, replacing any selected content, and move the caret after the inserted character.
  4. Cancel() in fckdialog.html is called.
  5. CloseDialog() in fckdialog.html is called.
  6. CloseDialog() calls Selection.EnsureSelection() to restore the selection in the editor window.

The current issue is, step 6 may fail and prevent the dialog from being closed. What happens if we just override the error (with a try{...} catch(e){}) at step 6? There will be no error, and the selection position after dialog closure would still be correct because step 3 has already moved the caret to the correct position. Adding a reset selection step (which saves the new selection position for the next EnsureSelection() call) just before step 6 has no real effect because the caret position is already at the correct place, and it assumes the caret is already at the correct place - so why bother restoring something that's already correct?

I've tested with just the error override fix in EnsureSelection() with other dialogs (e.g. button dialog, table dialog, image dialog, anchor dialog, etc.) and they all worked perfectly without ResetSelectionData(). The only real use I can think of for ResetSelectionData() is when we have a dialog which would do multiple modification to the editor document and wishes to save one of the resulting selections for IE - but I don't think there's such a case in our existing dialogs?

Changed 9 years ago by Martin Kou

Attachment: 1934_2.patch added

comment:6 Changed 9 years ago by Martin Kou

Keywords: Review? added; Discussion removed

No comments? I upload an updated patch for review then.

comment:7 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

It makes sense, and it works as all those dialogs are actually taking care about the selection. So, the proposed fix is ok for the current dialogs.

Please include the changelog for it.

comment:8 Changed 9 years ago by Martin Kou

Resolution: fixed
Status: assignedclosed

Fixed with [1688].

Click here for more info about our SVN system.

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