96891 - Closing dialog with "X" doesn't call the assigned "onCancel" method.

Status: VERIFIED DUPLICATE of bug 93732

(SeaMonkey :: Composer, defect)

Description

[TucsonTester2]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 6.0b; Windows NT 5.1)
BuildID:    20010821

If you open the characters and symbols menu and click on the close button to 
close it, it can be reopened.  However, if you opwn the menu and use the X at 
the top right hand corner to close it then you cannot reopen it without 
restarting composer.

Reproducible: Always
Steps to Reproduce:
1. Open a new composer page
2. Click on insert then choose Characters and Symbols
3. Click on the close button
4. Reopen the Characters and symbols menu
5. Click on the 'X' at the top to close it
6. Try to reopen the menu

Actual Results:  Menu does not open

Expected Results:  menu should open

Composer has to be restarted to open menu

Comment 1

[shrirang khanzode]

confirmed,  error in console:
" Warning prev sibling is not in our list!!! "

Comment 2

[Kathleen :Brade]

-->cmanske

Comment 3

[Charles Manske]

This worked fine in 6.1 and we haven't changed any Composer code.
I know there has been recent changes in the XPFE dialog code.
The Insert Character dialog is non-modal, so the parent window keeps track of
an existing child Insert Character window with the member variable: 
"InsertCharWindow". 
What is broke is that using the "X" to close a window doesn't call the usual
"Cancel" method. We set this method as we usually do with:
  doSetOKCancel(onOK, Cancel);
in the dialog startup code. This is the Cancel method in EdInsertChars.js:
function Cancel()
{
  window.opener.InsertCharWindow = null;
  SaveWindowLocation();
  window.close();
}

So something is broken in XP code. If this "Cancel" method isn't called,
the reference to the now-destroyed "InsertCharWindow" is not cleared, so 
the command to open the dialog tries to find that window, but it doesn't exist.
I'd like this to be examined for 0.9.4, as the bug completely breaks the 
Insert Character feature. I think there must be some other bad side effects 
as well!

Comment 4

[TucsonTester2]

Reproduce issue on win 95 4.00.950b ie 5.5 using build 2001082109

Comment 5

[Charles Manske]

This needs some attention please! 
Doesn't seem like Chris should own this, imho.

Comment 6

[Chris Waterson]

I think cmanske meant XPToolkit/Widgets.

Comment 7

[Charles Manske]

Thanks! Sorry about that.

Comment 8

[John Morrison]

Er, well, this didn't work in 6.1 (I just reproduced this defect in that 
build).

At any rate, there is no implicit hookup of the "Cancel" button with any code 
that will be run when the window is destroyed. (Maybe there should be, but that
is the way it has been thus far, and should be a separate bug from this one
if filed). 

If there is something that must be handled no matter how a window is destroyed, 
then it should be done in the unload handler of the window, e.g.:

@@ -33,6 +33,7 @@
     xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
     xmlns:html="http://www.w3.org/1999/xhtml"
     onload = "Startup()"
+    onunload = "window.opener.InsertCharWindow = null;"
     orient="vertical">

[You might also consider moving 'SaveWindowLocation();' into the unload 
handler, but I don't know; I haven't looked at what it does].

(There is also an 'onclose' handler, that will allow you to entirely abort
the closing of the window, but that handler is not triggered in the case of
of a direct call to window.close().)

Comment 9

[Charles Manske]

Darn! I thought I testing closing the window with "X" when I first implemented 
this, but you're right, the problem does exist in 6.1!
Thanks for the help - what you suggest works great!

Comment 10

[Charles Manske]

Attachment: Fix: Add "Unload()" handler to do dialog cleanup tasks

Comment 11

[Charles Manske]

ready to review

Comment 12

[John Morrison]

r=jrgm

Comment 13

[John Morrison]

... although, noting that there is a generic onCancel() in EdDialogCommon.js,
I'm wondering if there might be other places that are assuming that certain
cleanup actions are taken whenever/however a window is closed ...

Comment 14

[Charles Manske]

You are correct: most of Composer's dialogs use the generic "onCancel" method.
What is missed by using the "X" is just "SaveWindowLocation();", so we can 
certainly live with not having that for the short term.
Note that we do hit that "onCancel" when the "Esc" key is used to cancel a 
dialog. 

Comment 15

[Kathleen :Brade]

please keep this bug open after you checkin to track the remaining dialog issues.

Comment 16

[Charles Manske]

sr=kin (reviewed at Charley's machine)

Comment 17

[Asa Dotzler [:asa]]

a=asa on behalf of drivers

Comment 18

[Charles Manske]

Patch on 08/28/01 checked in, so the more important issue for the non-Modal
"Insert Character" dialog is fixed.

Keeping bug open to cover similar issue in all Composer dialogs:
We need to handle what we do in the common 'onCancel()' method for all dialogs
that use this common close method by adding appropriate "onunload" handlers.

Comment 19

[Syd Logan]

spam composer change

Comment 20

[Charles Manske]

I'm trying to simplify my dialog bugs by coalescing all the very simple tasks
that touch multiple dialogs into one bug. This problem is not covered by
bug 93732.

*** This bug has been marked as a duplicate of 93732 ***

Comment 21

[sujay]

verified.