Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#1990 closed Bug (fixed)

InsertHtml() ignores selection from floating dialog in IE7

Reported by: MarkWB Owned by: fredck
Priority: Normal Milestone: FCKeditor 2.6
Component: UI : Dialogs Version: SVN (FCKeditor) - Retired
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 (3)

insertbug.zip (1.4 KB) - added by MarkWB 9 years ago.
plugin for duplication
1990.patch (20.4 KB) - added by fredck 9 years ago.
1990_2.patch (20.5 KB) - added by fredck 9 years ago.

Download all attachments as: .zip

Change History (14)

Changed 9 years ago by MarkWB

plugin for duplication

comment:1 Changed 9 years ago by martinkou

  • Owner set to martinkou
  • Status changed from new to assigned

comment:2 Changed 9 years ago by martinkou

  • Keywords Discussion added

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.

comment:3 Changed 9 years ago 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.

comment:4 Changed 9 years ago by fredck

  • Keywords Confirmed added; Discussion removed

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.

comment:5 Changed 9 years ago by fredck

  • Owner changed from martinkou to fredck
  • Status changed from assigned to new

comment:6 Changed 9 years ago by fredck

  • Status changed from new to assigned

Changed 9 years ago by fredck

comment:7 Changed 9 years ago by fredck

  • Keywords Review? added

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.

Changed 9 years ago by fredck

comment:8 Changed 9 years ago by fredck

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

comment:9 Changed 9 years ago by martinkou

  • Keywords Review+ added; Review? removed

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

comment:10 Changed 9 years ago by fredck

  • Resolution set to fixed
  • Status changed from assigned to closed

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

comment:11 Changed 8 years ago by alfonsoml

This patch has caused #2495

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