459254 - shift-command-clicking on a link opens new window in foreground instead of background

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[edjusted]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en; rv:1.9.0.4pre) Gecko/2008100800 Camino/2.0a1pre (like Firefox/3.0.4pre)
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en; rv:1.9.0.4pre) Gecko/2008100800 Camino/2.0a1pre (like Firefox/3.0.4pre)

In previous builds of Camino, command-clicking on a link opens the link in a new foreground window, while shift-command-clicking on a link opens the link in a new *background* window. In the new 2.0a1 build, shift-command-clicking on a link opens the link the the *foreground* instead of the *background*.

Reproducible: Always

Steps to Reproduce:
1.shift-command-click on a link in any web page
2.
3.
Actual Results:  
link opens in a new foreground window

Expected Results:  
link should open in a new background window instead

Comment 1

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

This broke between 2008-09-19-00 and 2008-09-20-00, which basically means the tab loop!?

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=Camino&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-09-19+00%3A00&maxdate=2008-09-20+00%3A00&cvsroot=%2Fcvsroot

Comment 2

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

cl had bug 395423 that day, too, not that it seems any more likely.

Comment 3

[Chris Lawson (gone)]

Yeah, I don't see how any of the tab loop stuff (or my bug) could possibly have broken this, but I don't see anything else even by expanding that regression window 24 hours on either end.

Comment 4

[Chris Lawson (gone)]

Even expanding the regression window to 15-23 September, I don't see anything likely:

http://bonsai.mozilla.org/cvsquery.cgi?date=explicit&mindate=2008-09-15&maxdate=2008-09-23

but I can indeed confirm Smokey's regression window. The 15 Sep build works; the 23 Sep build doesn't.

Comment 5

[Sean Murphy]

I'm not sure why the keyboard loop fixes I introduced seem to cause this, but the basic gist of what's going on:

When showing the new window, we call setSuppressMakeKeyFront:YES, order the window behind the current one, then immediately call setSuppressMakeKeyFront:NO.  When Gecko sends the SetFocus() callback, which happens during loading start and finish, we'll check the suppressMakeKeyFront and find it to be no and then order the window front, since the flag was immediately switched back when finished showing the window.

It's not immediately clear to me why this worked before, but I'd fix it by removing the setSuppressMakeKeyFront:NO call when showing the browser window.  As soon as the window becomes key again, by the user, setSuppressMakeKeyFront:NO will actually be called during that process.

Comment 6

[Sean Murphy]

Attachment: Patch

Fix using the approach mentioned above.

Comment 7

[Chris Lawson (gone)]

(In reply to comment #5)

> It's not immediately clear to me why this worked before, but I'd fix it by
> removing the setSuppressMakeKeyFront:NO call when showing the browser window. 
> As soon as the window becomes key again, by the user,
> setSuppressMakeKeyFront:NO will actually be called during that process.

Does this mean we should remove *all* the instances of |setSuppressMakeKeyFront:NO| outside of stuff that would be getting hit when a window becomes key? I see two other occurrences of that call in the code, one or both of which might be responsible for bug 461403 (which is very similar but isn't helped by the existing patch here).

Comment 8

[Chris Lawson (gone)]

For that matter, is there a reason Gecko is allowed to steal focus *ever* when it's not in the key window? I'm thinking we need to change the |setGeckoActive:| method in BWC so that it only calls |setSuppressMakeKeyFront:NO| if |setGeckoActive:|'s in-parameter is YES. Isn't Gecko's focus-setting part of, well, Gecko being "active"? Seems to me like we ought to fully "inactivate" Gecko in a method call of |setGeckoActive:NO| :-p

Comment 9

[Stuart Morgan]

(In reply to comment #8)
> For that matter, is there a reason Gecko is allowed to steal focus *ever* when
> it's not in the key window?

window.focus()

Comment 10

[Sean Murphy]

Comment on attachment 343434 [details] [diff] [review]
Patch

Clearing the review request, and obsoleting the patch, because rather than working around the problem, I found the root cause and this will be fixed by bug 461403.

Comment 11

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

FIXED by the landing of bug 461403; the fix will be in tomorrow's Cm2-M1.9 nightly.

Comment 12

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

Grr.  /kick bug 452888.