Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#3001 closed Bug (fixed)

IE : Automatically save the selection when loosing focus

Reported by: Frederico Caldeira Knabben Owned by: Frederico Caldeira Knabben
Priority: Must have (possibly next milestone) Milestone: CKEditor 3.0
Component: General Version:
Keywords: Confirmed Review+ Cc:

Description

IE is not able to manage multiple selection, even cross frame. Because of this, the current selection gets lost when leaving the editing area.

The current selection should be automatically saved when the editing area looses the focus in IE and then reselected when the focus is moved back to it.

By doing that, we can remove all selection hacks we are doing now for dialogs, floating panes, etc.

Attachments (3)

3001.patch (23.7 KB) - added by Frederico Caldeira Knabben 9 years ago.
3001_2.patch (23.2 KB) - added by Martin Kou 9 years ago.
3001_3.patch (46.8 KB) - added by Frederico Caldeira Knabben 9 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 9 years ago by Frederico Caldeira Knabben

Owner: set to Frederico Caldeira Knabben
Priority: NormalHigh
Status: newassigned

Changed 9 years ago by Frederico Caldeira Knabben

Attachment: 3001.patch added

comment:2 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review? added

Changed 9 years ago by Martin Kou

Attachment: 3001_2.patch added

comment:3 Changed 9 years ago by Martin Kou

I've fixed some coding mistakes in the dialog plugin, and also updated the patch to account for changes in [3316].

comment:4 Changed 9 years ago by Frederico Caldeira Knabben

Martin, the new patch is ok for me... if you give it R+ I'll proceed on committing it.

comment:5 Changed 9 years ago by Garry Yao

Keywords: Review- added; Review? removed

Some issues have been found after the patch is applied:

  1. L513 and L670 of _source\plugins\dialog\plugin.js should dedicate to IE.
  2. Certain plugins( e.g Link/Image/Form ) require the selection been restored within 'onOk', which fired even before 'onHide' event of dialog, so the selection range is still not correct for them, reproduce with:

Reproducing Procedures

  1. Open the replace by class example page;
  2. Select the word 'you';
  3. Open 'image' dialog to insert a image;
    • Actual Result: The image has been inserted at the front.

Changed 9 years ago by Frederico Caldeira Knabben

Attachment: 3001_3.patch added

comment:6 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review? added; Review- removed

This new patch makes the selection issues much more self contained, so we can completely avoid selection hacks elsewhere in the code, specially in dialog definitions.

comment:7 Changed 9 years ago by Garry Yao

Keywords: Review+ added; Review? removed

It's worthy to takes a long time to validate all those changes introduced by the new patch, it really eliminate all those previous hacks everywhere, so R+.
Though, I found the following minor issues:

  1. It's not necessary to have L132 of _source\plugins\selection\plugin.js, since a saving will be fired by 'selectchange' event at the moment;
  2. Within selectElement method L766, maybe the cached type should be 'CKEDITOR.SELECTION_ELEMENT' instead.
  3. Within selectRanges method L813, we can't leave 'selectedElement' to null, otherwise later it will be retrieved from native selection, which is error prone, also the 'type' and 'startElement' is not accurately calculated.

comment:8 Changed 9 years ago by Garry Yao

And another point, not sure if we can also utilize the selection locking system in focus/blur selection management for the same purpose, which means lock on 'blur' event and unlock on 'focusin' event instead of performing the save on every single action.

comment:9 Changed 9 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: assignedclosed

Fixed with [3329].

comment:10 in reply to:  7 Changed 9 years ago by Frederico Caldeira Knabben

Replying to garry.yao:

  1. It's not necessary to have L132 of _source\plugins\selection\plugin.js, since a saving will be fired by 'selectchange' event at the moment;

It's needed. I've found some cases where selectionchange don't get fired, or it's fired before the focus event, making the saving be missed.

  1. Within selectElement method L766, maybe the cached type should be 'CKEDITOR.SELECTION_ELEMENT' instead.

Ok... I've thought about <span> elements and so on, but it makes sense to have the type as element. I've changed it when committing.

  1. Within selectRanges method L813, we can't leave 'selectedElement' to null, otherwise later it will be retrieved from native selection, which is error prone.

No problem to use "null". The cache checking is done using "!== undefined", which is "true" for "null.

also the 'type' and 'startElement' is not accurately calculated.

Not a big issue for now. It would just make our code larger with not real benefits.

comment:11 in reply to:  8 Changed 9 years ago by Frederico Caldeira Knabben

Replying to garry.yao:

And another point, not sure if we can also utilize the selection locking system in focus/blur selection management for the same purpose, which means lock on 'blur' event and unlock on 'focusin' event instead of performing the save on every single action.

That's was my very first try, which looked obvious and much simpler. But, our dear IE looses the selection before the blur event (or focusout) get fired (in some cases), so we can't save it. It took me two days to come out with the final solution.

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