370096 - onbeforeunload doesn't work when closing tab/window

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Mats Palmgren (inactive)]

onbeforeunload doesn't work

STEPS TO REPRODUCE
1. load URL (testcase from bug 284596)
2. type Cmd+W or Cmd+Q

ACTUAL RESULT
Window is closed without any popup dialog appearing

EXPECTED RESULT
popup dialog appears giving the user a chance to veto the close/quit.

PLATFORMS AND BUILDS TESTED
Camino trunk debug build, a few days old, on MacOSX 10.4.8

Comment 1

[Stuart Morgan]

Fixing summary. It works if you navigate away from the page, just not if you close the tab/window. Are there embedding hooks we can to tie into to give handlers a chance to run?

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Happens on branch, too.

Comment 4

[Stuart Morgan]

Attachment: fix

Adds a CHBV method to check onunload events, and sets up the chrome to call it whenever closing tabs or windows. I also renamed BW's confusingly-named |windowClosed| method, which has always just really meant that the browser view was closed, since that drove me crazy while figuring out how to wire this up.

Comment 5

[Stuart Morgan]

Er, s/onunload/onbeforeunload/.

Comment 6

[Mike Pinkerton (not reading bugmail)]

+  // Check all the winodws to see if any have tabs that shouldn't be closed.

fix typo.

+  NSArray* openWindows = [[[NSApp orderedWindows] copy] autorelease];

why copy? i don't see anything in here that changes the list while we're iterating.

+  int numTabs = [self numberOfTabViewItems];

const int?

looks good otherwise.

Comment 7

[Stuart Morgan]

Attachment: v2

Comments addressed. Also fixes an early return I missed in applicationShouldTerminate that would skip onbeforeunloads if the general quit warning pref were off. It looks like a lot more change in MainController than it really is, since it's just reparenting a lot of existing code into an |if ([prefManager getBooleanPref:"camino.warn_when_closing" withSuccess:NULL])|.

Comment 8

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 278205 [details] [diff] [review]
v2

sr=pink

Comment 9

[Stuart Morgan]

Landed on trunk and MOZILLA_1_8_BRANCH.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Since this is a web compat issue, we should consider this for 1.5.2 (if it can reasonably be backported).

Comment 11

[Stuart Morgan]

I'd like to see this bake for a little while, just to be safe, so it depends when we do 1.5.2

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Stuart, are you satisfied with the baking now?  (Unfortunately, the hunk in MainController doesn't apply on the 1_5 branch; it'll need a little massaging because of the exceptions change.)

Comment 13

[Stuart Morgan]

Yep, sounds good to me.

Comment 14

[Stuart Morgan]

Landed on CAMINO_1_5_BRANCH.