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

VERIFIED DUPLICATE of bug 93732

Status

SeaMonkey
Composer
VERIFIED DUPLICATE of bug 93732
17 years ago
13 years ago

People

(Reporter: TucsonTester2, Assigned: Charles Manske)

Tracking

Trunk
mozilla0.9.5

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: EDITORBASE (1/2 day))

Attachments

(1 attachment)

(Reporter)

Description

17 years ago
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

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

Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

17 years ago
-->cmanske
Assignee: brade → cmanske
(Assignee)

Comment 3

17 years ago
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!
Assignee: cmanske → waterson
Component: Editor → XP Miscellany
Keywords: regression
QA Contact: sujay → brendan
Summary: Characters and symbols menu wont open → Closing non-modal dialog with "X" doesn't call the assigned "onCancel" method.
(Reporter)

Comment 4

17 years ago
Reproduce issue on win 95 4.00.950b ie 5.5 using build 2001082109
(Assignee)

Comment 5

17 years ago
This needs some attention please! 
Doesn't seem like Chris should own this, imho.
Severity: normal → major
Target Milestone: --- → mozilla0.9.4

Comment 6

17 years ago
I think cmanske meant XPToolkit/Widgets.
Assignee: waterson → trudelle
Component: XP Miscellany → XP Toolkit/Widgets
QA Contact: brendan → jrgm
(Assignee)

Comment 7

17 years ago
Thanks! Sorry about that.

Comment 8

17 years ago
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().)
Assignee: trudelle → cmanske
Component: XP Toolkit/Widgets → Editor
QA Contact: jrgm → sujay
(Assignee)

Comment 9

17 years ago
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!

(Assignee)

Comment 10

17 years ago
Created attachment 47465 [details] [diff] [review]
Fix: Add "Unload()" handler to do dialog cleanup tasks
(Assignee)

Comment 11

17 years ago
ready to review
Status: NEW → ASSIGNED
Keywords: patch, review
Whiteboard: FIX IN HAND need r=, sr=

Comment 12

17 years ago
r=jrgm

Comment 13

17 years ago
... 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 ...
(Assignee)

Comment 14

17 years ago
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

17 years ago
please keep this bug open after you checkin to track the remaining dialog issues.
Whiteboard: FIX IN HAND need r=, sr= → FIX IN HAND need sr=
(Assignee)

Comment 16

17 years ago
sr=kin (reviewed at Charley's machine)
(Assignee)

Updated

17 years ago
Whiteboard: FIX IN HAND need sr= → FIX IN HAND reviewed
(Assignee)

Updated

17 years ago
Keywords: nsbranch+

Comment 17

17 years ago
a=asa on behalf of drivers
(Assignee)

Comment 18

17 years ago
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.

Severity: major → normal
Keywords: nsbranch+, patch, regression, review
Summary: Closing non-modal dialog with "X" doesn't call the assigned "onCancel" method. → Closing dialog with "X" doesn't call the assigned "onCancel" method.
Whiteboard: FIX IN HAND reviewed
Target Milestone: mozilla0.9.4 → mozilla0.9.5

Comment 19

17 years ago
spam composer change
Component: Editor: Core → Editor: Composer
(Assignee)

Updated

17 years ago
Whiteboard: EDITORBASE (1/2 day)
(Assignee)

Comment 20

16 years ago
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 ***
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → DUPLICATE

Comment 21

16 years ago
verified.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.