416455 - App-focus wierdness after right-clicking on backgrounded browser window

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[Steven Michaud [:smichaud] (Retired)]

Strange app-focus problems happen when you right-click on a browser
window that belongs to a backgrounded instance of Minefield, and then
cause another window to be opened.

This problem is a regression caused (or at least triggered) by the
patch for bug 396186.  It doesn't happen when you roll back that bug's
patch (attachment 297284 [details] [diff] [review]).

STR

1) Run Minefield, and make sure at least one browser window is open.

2) Click on the desktop (or another app) to put Minefield into the
   background, but leave its browser window visible.

3) Right-click on that browser window and (from the resulting context
   menu) choose "View Page Source" or "View Page Info".

   A new window will appear (containing either the page source or the
   page info).  Minefield will still be in the background, and the
   main menu will remain as before (i.e. it will not switch to the
   Minefield menu).  This is the correct behavior.

   But left-clicking on any of the browser's windows (the browser
   window, the page source window or the page info window) will not
   switch the app focus to Minefield -- the main menu will not switch
   to the Minefield menu, and it won't be possible to place the
   keyboard focus into any of its windows.  This is not correct.

   Correct behavior can be restored by clicking	on the desktop (or
   another app) and then clicking once more on one of Minefield's
   visible windows.

The easiest way to fix this bug is to roll back the patch for bug
396186.  Given sufficient time, I may be able to find another way to
fix it ... but we don't have a lot of time before the Firefox 3
release.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Attachment: Back out patch for bug 396186

I'm not going to have time to look into this further before beta4.  So
here's a patch that backs out the patch for bug 396186 and (in my
tests) fixes this bug (bug 416455).  Beyond testing that it does fix
this bug, I haven't yet done much with it, so I'm not yet requesting
review.

A tryserver build will follow in an hour or two.

Neal, can you think of specific objections to this patch (beyond the
fact that it's ugly, which I already know)?  Can you think of things
that it might break, and that I should test?

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with this patch (attachment 304576 [details] [diff] [review]):

https://build.mozilla.org/tryserver-builds/2008-02-20_12:[e-mail]-bugzilla416455/[e-mail]

Comment 3

[Steven Michaud [:smichaud] (Retired)]

> I'm not going to have time to look into this further before beta4.

Actually, while updating my patch for the current tree I stumbled upon
a much simpler (and better) fix.  New patch (and tryserver build)
coming up shortly.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Attachment: Patch rev1 (simplify)

It isn't necessary to back out the patch for bug 396186.  Instead we
should fix an error in that patch -- it dropped a needed call to
maybeInitContextMenuTracking.

I've now tested this patch on both OS X 10.4.11 and 10.5.2, popping up
and using all kinds of context menus.  I didn't see any problems.

Here's a tryserver build made with this patch:

https://build.mozilla.org/tryserver-builds/2008-02-25_12:[e-mail]-bugzilla416455-rev1/[e-mail]

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 305588 [details] [diff] [review]
Patch rev1 (simplify)

+    if (!NS_SUCCEEDED(rv) || !useNativeContextMenus)

NS_FAILED(rv)

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 305588 [details] [diff] [review]
Patch rev1 (simplify)

This appears not to have landed yet.

I'm seeking approval for my patch plus the change recommended in
comment #5.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

This patch's risk is small but non-zero.

So I think it makes sense to include it in beta4 to give it wider
testing.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 305588 [details] [diff] [review]
Patch rev1 (simplify)

a1.9b4=beltzner

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Landed on trunk:

Checking in widget/src/cocoa/nsChildView.mm;
/cvsroot/mozilla/widget/src/cocoa/nsChildView.mm,v  <--  nsChildView.mm
new revision: 1.311; previous revision: 1.310
done

But (oops) forgot to make the change roc suggested in comment #5.
Since this change was just cosmetic, I'll leave things the way they
are for now.  I'll fix this once the tree is unfrozen after the beta4
release.