419338 - window.focus() doesn't bring the window to the front

Status: VERIFIED FIXED

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

Description

[Ed Lee :Mardak]

Pressing cmd-j for the download manager while it's open doesn't bring the window to the front. But it does move focus away from the current window.

This might have regressed from bug 417124. (Some time between 02/17 and 02/20.)

Comment 1

[Josh Aas]

Attachment: fix v1.0

We might want to do this. It is getting late and I haven't had the time for proper contemplation and testing, will request review later.

Comment 2

[Håkan Waara]

The top of the stack in my debugging showed that it is our very own WindowDelegate method windowBecameKey: that invokes code which expects our ::Show(PR_TRUE) to focus a window.

It's unfortunate because there's a lot of weird hacks going on inside ::Show() that should probably only be run on a freshly created window (if ever).

A similar approach to this would be to set a windowIsAlreadyShowing boolean which lets us skip everything but the most necessary in the case where we should only bring a window to front and focus it.

Is there any work to get a widget test-suite, or focus test-suite going so we can work on real fixes for these issues and remove the hacks? I think something like that is critical for the maintainability of this code.

Comment 3

[Josh Aas]

I'm not sure what your comment about the window delegate means. Explain more?

I don't want to do any more monkeying around with this code. We already did a little bit of cleanup and we ended up with this bug and a sibling sheets regression.  We can do more cleanup in mozilla 2.

Comment 4

[Håkan Waara]

This bug is not due to the cleanup, it's due to the returning early, which was the fix for redrawing regression bug 417124.

I agree that this code is no fun to touch though, hence the last paragraph in comment 2... It was not a rhetorical question; I am genuinely interested in what post-1.9 work will be in this area.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

> Is there any work to get a widget test-suite, or focus test-suite
> going

Just now I've been doing some work on testing focus bugs.  But there
are many problems, and it's unlikely that there will be useful results
anytime soon.  See bug 419466.

Comment 6

[Josh Aas]

Post-1.9 there will probably be some work to remove those hacks, and almost certainly we'll be working on tests of some sort. I don't know much about that in particular right now though. There are some bugs on file like 407955, but I can't really pay attention to that stuff right now.

Comment 7

[Håkan Waara]

Comment on attachment 305439 [details] [diff] [review]
fix v1.0

Please add a comment saying why we can return early sometimes, but not always.

Comment 8

[Håkan Waara]

One thing to check too: I hope there is no code that expects us to be "symmetric".  Before in ::Show() we always re-executed all the stuff in there for both bState PR_FALSE/PR_TRUE, but now we'll only do it only for true. 

Comment 9

[Josh Aas]

The only thing I know of that is symmetric is the sheet code, but that is protected already.

Comment 10

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

So we're going to rerun Show() every time we call Show(PR_TRUE) on a window that's already showing? won't that regress 417124?

Comment 11

[Josh Aas]

No, because the "[mWindow display]" call is in the ::Show(FALSE) case.

Comment 12

[Josh Aas]

landed on trunk

Comment 15

[Dave Miller [:justdave]]

I can confirm that this appears to have fixed the specific situation in bug 419094.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b4pre)
Gecko/2008022704 Minefield/3.0b4pre

Comment 16

[Marcia Knous [:marcia]]

Verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b4pre) Gecko/2008022704 Minefield/3.0b4pre and a 10.4 nightly.