Opened 8 years ago

Closed 8 years ago

#5618 closed Bug (fixed)

Dialog error when remove editor instance

Reported by: Garry Yao Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.3
Component: UI : Dialogs Version:
Keywords: Confirmed Review+ Cc:

Description

The following regression is seen after [5412]:

Reproducing Procedures

  1. Load the 'Ajax' sample page and click 'Create Editor' button;
  2. Open any of the the dialog;
  3. Execute the following lines of JavaScript in global scope:
    removeEditor();
    
  4. Click once again 'Create Editor' and open the same dialog;
    • Actual Result: Dialog opened with no viewport cover.

Attachments (3)

4797_4.patch (6.1 KB) - added by Garry Yao 8 years ago.
5618.patch (495 bytes) - added by Alfonso Martínez de Lizarrondo 8 years ago.
Simple patch
5618_2.patch (5.8 KB) - added by Garry Yao 8 years ago.

Download all attachments as: .zip

Change History (16)

Changed 8 years ago by Garry Yao

Attachment: 4797_4.patch added

comment:1 Changed 8 years ago by Garry Yao

Keywords: Review? added
Owner: set to Garry Yao
Status: newassigned

comment:2 Changed 8 years ago by Garry Yao

Priority: NormalHigh

comment:3 Changed 8 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review- added; Review? removed

Is it really expected for the people to destroy the editor while a dialog is open?

I can see that now there's an error when an instance is destroyed if no dialog was open, and can be fixed by a little patch, but all those changes to take care of the proposed situation seem a little overkill to me.

Changed 8 years ago by Alfonso Martínez de Lizarrondo

Attachment: 5618.patch added

Simple patch

comment:4 Changed 8 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review? added; Review- removed

This patch fixes a regression that must be fixed ASAP, and then we can keep talking about the previous patch and if that's a situation that should be supported.

comment:5 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed
Priority: HighNormal

I'm ok with Alfonso's patch. It can be committed leaving this ticket opened. Please remove the review keyword after that.

comment:6 in reply to:  3 Changed 8 years ago by Frederico Caldeira Knabben

Replying to alfonsoml:

Is it really expected for the people to destroy the editor while a dialog is open?

We can't say no for that, but it may happen.

I can see that now there's an error when an instance is destroyed if no dialog was open, and can be fixed by a little patch, but all those changes to take care of the proposed situation seem a little overkill to me.

Garry's proposal is not bad. It doesn't bring too many changes to the code, mainly changing the logic on the cover management, which is ok.

comment:7 Changed 8 years ago by Frederico Caldeira Knabben

@Garry... I've reviewed your patch, and there is a problem with it, which is related to the z-index changes you've made.

If you open the table cell dialog, you can then open the color dialog for the cell background. Note that, after the patch, the second dialog cover is not there.

comment:8 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review+ removed

I've committed Alfonso's patch with [5448] because things are pretty broken without it.

comment:9 Changed 8 years ago by Garry Yao

Keywords: Review? added

If you open the table cell dialog, you can then open the color dialog for the cell background. Note that, after the patch, the second dialog cover is not there.

Previously we made all opened dialogs model (by overlaying the latest opened upon the old one), while I'm proposing here to remove this feature, with the advantages of being able to edit all opened dialog simultaneously, does it make sense?

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

Keywords: Review- added; Review? removed

Replying to garry.yao:

If you open the table cell dialog, you can then open the color dialog for the cell background. Note that, after the patch, the second dialog cover is not there.

Previously we made all opened dialogs model (by overlaying the latest opened upon the old one), while I'm proposing here to remove this feature, with the advantages of being able to edit all opened dialog simultaneously, does it make sense?

That's not good, because it takes the use focus out of the "current task", causing confusion. Let's have the current behavior preserved.

Changed 8 years ago by Garry Yao

Attachment: 5618_2.patch added

comment:11 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review- removed

comment:12 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:13 Changed 8 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [5476].

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