365746 - clicking dock icon when modal dialog is showing shouldn't create new browser window.

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Sean Murphy]

Attachment: patch

It is expected behavior that clicking on Camino's application icon in the dock will open up a new browser window if one is not already present.  When a modal alert dialog is showing and the dock is clicked, however, we probably should suppress displaying the new browser window.

To reproduce the behavior of the current situation:

1) In Camino's preferences, enable "Load home page when: opening new window"
2) Close all open browser windows
3) Get Camino to display a modal alert (choose Empty Cache)
4) Click Camino's icon in the dock

A new window is opened, but not yet accessible, behind the dialog.  Furthermore, the home page of the new window is unable to actually load (maybe since the main thread is blocked).

Fixing this will also prevent any unexpected behavior if a modal dialog ends up being used in bug 358689.

Since this bug is hard to describe, here's a movie demonstrating the issues I mentioned above: <http://seanmurph.com/camino/handleReopen.mov>.  The first dialog is the session manager kicking in after a crash and is currently still in patch form (so don't be confused when you see it).

A possible fix could also simplify the error checking procedures of applicationShouldHandleReopen:hasVisibleWindows:, specifically the prevention of a modal loop with PreferenceManager.  For example:

  // we might be sitting there with the "there is another copy of camino running" dialog up
  // (which means we're in a modal loop in [PreferenceManager init]). So if we haven't
  // finished initting prefs yet, just bail.
  if (![PreferenceManager sharedInstanceDontCreate])
    return NO;

can be removed, and would become…

  // Don't open a new browser window if we're displaying a modal alert dialog
  if ([NSApp modalWindow])
    return NO;

…which, in addition to preventing that same modal loop, will avoid opening a new browser window if anything modal is currently displayed (not just the "camino is already running" dialog).

If everyone thinks this is worth fixing, a patch using the above technique is attached.

Comment 1

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

Will this help with the Dock icon case of bug 354643 at all?

Comment 2

[Sean Murphy]

(In reply to comment #1)
> Will this help with the Dock icon case of bug 354643 at all?

That bug did come to mind when I was fixing this one.  A fix here wouldn't necessarily seem to solve that problem, but I have been trying to fall victim to the behavior of bug 354643 and have to say it does WFM now (thanks for the detailed instructions to reproduce, btw).  I was previously able to cause the problem to occur there.  This patch's main purpose is to suppress displaying a new browser window behind an already present modal dialog when the dock icon is clicked.  The default browser dialog appears after a browser window is already visible.  I'll look into this some more, and try to determine more about what's going on here after today's meeting.

An aside: Is there a problem with the "check default browser on startup" preference being remembered?  I tried to re-enable this option and it would not stick after closing the preferences window, switching to a different pane, or most importantly re-launching the app.  I'll submit a report and fix that problem if it turns out not to be something wrong with just my local copy.

Comment 3

[Stuart Morgan]

Comment on attachment 250279 [details] [diff] [review]
patch

Now that we have the "init is complete" method, let's use that as the test here, since that's what we really want to have prevent window creation.

Comment 4

[Sean Murphy]

(In reply to comment #3)
> (From update of attachment 250279 [details] [diff] [review])
> Now that we have the "init is complete" method, let's use that as the test
> here, since that's what we really want to have prevent window creation.

Relying solely on |isInitialized| doesn't take into account the situation I mentioned as the STR in comment #0: clicking on the dock icon with the empty cache dialog visible.

Opening up a new browser window in this situation is definitely not as problematic as doing so while waiting for an answer from the 'resume session from crash' dialog, but the loading of the home page is paused and the user is unable to interact with the window.

Would you rather just worry about initialization for now, or consider both scenarios and look for a modal dialog as well?

Comment 5

[Stuart Morgan]

Whoops, forgot about this bug. That's a good point; lets actually check both, since we really want both to be true. (It may not be feasible to hit the non init case at the moment, but if it is/becomes so, we may as well be prepared for it)

Comment 6

[Sean Murphy]

Attachment: Patch v2 (New Approach)

Comment 7

[Stuart Morgan]

Comment on attachment 297118 [details] [diff] [review]
Patch v2 (New Approach)

r=me

Comment 8

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

Landed on the trunk and MOZILLA_1_8_BRANCH.