Ticket #1990 (closed Bug: fixed)

Opened 2 months ago

Last modified 2 months ago

InsertHtml() ignores selection from floating dialog in IE7

Reported by: MarkWB Assigned to: fredck
Priority: Normal Milestone: FCKeditor 2.6
Component: UI : Dialogs Version: SVN
Keywords: Confirmed Review+ Cc:

Description

"FCK.InsertHtml()" function ignores selection/position when executed from a floating dialog instanciated with "FCKDialogCommand", and the web browser is IE7. The inserted text always gets inserted at the beginning. This does NOT happen when using Firefox 2.0.0.12. Furthermore, it does not happen if executed from an IE popup using window.showModalDialog().

Attached is a very simple plugin called 'insertbug' that adds the command 'InsertText' for demonstrating the problem. Firefox has no problem with selected text getting replaced or inserted at the insertion carot position. With IE, "Inserted Text" is always inserted at the beginning.

Tested 2.6.beta and nightly build 18335 (03/09/2008).

Attachments

insertbug.zip (1.4 kB) - added by MarkWB on 03/09/08 16:25:02.
plugin for duplication
1990.patch (20.4 kB) - added by fredck on 03/22/08 16:45:07.
1990_2.patch (20.5 kB) - added by fredck on 03/22/08 18:47:00.

Change History

03/09/08 16:25:02 changed by MarkWB

  • attachment insertbug.zip added.

plugin for duplication

03/11/08 07:50:44 changed by martinkou

  • owner set to martinkou.
  • status changed from new to assigned.

03/11/08 08:52:46 changed by martinkou

  • keywords set to Discussion.

We've actually got an API added in v2.6 to deal with the problem. It is a known problem that IE cannot keep multiple selections across iframes, and so we've been saving the last known selection position inside the editor iframe just before the dialog opened. You can restore the last selection position for InsertHtml() by calling window.parent.Selection.EnsureSelection() before InsertHtml(), which is what we're doing in the 2.6 dialogs.

Your ticket leads to a possible improvement upon the current implementation however - plugin developers probably don't care or don't know about the IE bug I've mentioned and the EnsureSelection() workaround. Maybe we should call the EnsureSelection() workaround in InsertHtml() and other content manipulating functions in our API to make it transparent to plugin developers.

03/14/08 03:26:59 changed by MarkWB

I did find the API you speak of after I submitted this, but it took some digging. It would be nice if cross-browser consistency is maintained for the 'documented' user API. However, documentation could also work, while such an API could hide what developers will ultimately be needing to know anyway for other reasons. If 'other reasons' could possibly involve development of plugins, I think I'd go with documentation. Keep up the good work. It looks to me like you'll end up making a sensible decision.

03/19/08 12:01:37 changed by fredck

  • keywords changed from Discussion to Confirmed.

Martin, you proposal for it makes a lot of sense. Let's make it transparent, doing any kind of fix in this sense inside the API.

We could have the FCK.SaveSelection() and FCK.EnsureSelection() functions (empty for non-IE), which would then be used in the FCKDialog and the API, for those functions that are usually called by dialogs, and that depend on the selection, like InsertHtml.

At this point, EnsureSelection should be removed from fckdialog.html, as well as any unnecessary calls to it.

03/22/08 15:30:48 changed by fredck

  • owner changed from martinkou to fredck.
  • status changed from assigned to new.

03/22/08 15:30:55 changed by fredck

  • status changed from new to assigned.

03/22/08 16:45:07 changed by fredck

  • attachment 1990.patch added.

03/22/08 16:50:21 changed by fredck

  • keywords changed from Confirmed to Confirmed Review?.

The provided patch brings a huge change to the code, mainly for the points dealing with selection. Basically now, using FCKSelection is a safe way to be sure the selection will be in the right place for IE. It should bring backward compatibility with legacy plugins.

Another thing introduced is "FCK.IsSelectionChangeLocked", making it possible to stop firing OnSelectionChange. This was necessary because I was having infinite loops when restoring the selection. It should also bring some performance benefits, opening the possibility of using it in another place, like the Enter Key handler, as already proposed in another ticket.

03/22/08 18:47:00 changed by fredck

  • attachment 1990_2.patch added.

03/22/08 18:47:39 changed by fredck

I've attached a new patch, following Alfonso's findings reported on IRC.

03/25/08 10:50:10 changed by martinkou

  • keywords changed from Confirmed Review? to Confirmed Review+.

A very nice patch. Passes my regression tests (e.g. #2024, #1801, and one more on control selection which I forgot the ticket #..) with flying colors as well. Review+.

03/25/08 11:24:13 changed by fredck

  • status changed from assigned to closed.
  • resolution set to fixed.

Fixed with [1795]. Click here for more info about our SVN system.