Selection.EnsureSelection is broken for object selections

[Frederico Caldeira Knabben]

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.

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

[Frederico Caldeira Knabben]

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.

[Martin Kou]

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?

[Martin Kou]

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

[Frederico Caldeira Knabben]

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.

[Martin Kou]

Fixed with [1688].

Click here for more info about our SVN system.