Last Comment Bug 574229 - [Mac] Choosing "New Window" from Firefox's dock menu does not open the new window in the active Space
: [Mac] Choosing "New Window" from Firefox's dock menu does not open the new wi...
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: Shell Integration (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal with 4 votes (vote)
: Firefox 17
Assigned To: Josh Aas
:
Mentors:
Depends on: 415463
Blocks: 389614
  Show dependency treegraph
 
Reported: 2010-06-23 18:17 PDT by Tom Dyas
Modified: 2012-10-05 11:12 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
affected
verified
verified


Attachments
non-working quick fix (733 bytes, patch)
2010-11-15 19:22 PST, Tom Dyas
no flags Details | Diff | Splinter Review
another non-working fix (1.09 KB, patch)
2010-11-22 10:45 PST, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
fix v0.8 (858 bytes, patch)
2012-08-03 18:49 PDT, Josh Aas
no flags Details | Diff | Splinter Review
fix v1.0 (1.08 KB, patch)
2012-08-03 20:34 PDT, Josh Aas
no flags Details | Diff | Splinter Review
fix v1.1 (1.39 KB, patch)
2012-08-07 12:24 PDT, Josh Aas
gavin.sharp: review+
gavin.sharp: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Tom Dyas 2010-06-23 18:17:44 PDT
In bug 415463 comment 115, a user reported that, when "New Window" from Firefox's dock menu is selected, Firefox does not open the new window in the active Space, but rather in the Space where the last active Firefox window is.  I have confirmed this behavior.

Safari's dock menu also includes a "New Window" item and will open the new window in the active Space.  Firefox should match Safari's behavior.

Here is a link to one person's pure Cocoa solution: http://osdir.com/ml/cocoa-dev/2009-11/msg01284.html  I am investigating whether a similar solution will work for Gecko.  (The fix may end up being in Widget:Cocoa.)
Comment 1 harsun 2010-11-15 19:16:19 PST
(In reply to comment #0)
> In bug 415463 comment 115, a user reported that, when "New Window" from
> Firefox's dock menu is selected, Firefox does not open the new window in the
> active Space, but rather in the Space where the last active Firefox window is. 
> I have confirmed this behavior.
> 
> Safari's dock menu also includes a "New Window" item and will open the new
> window in the active Space.  Firefox should match Safari's behavior.
> 
> Here is a link to one person's pure Cocoa solution:
> http://osdir.com/ml/cocoa-dev/2009-11/msg01284.html  I am investigating whether
> a similar solution will work for Gecko.  (The fix may end up being in
> Widget:Cocoa.)

I have a number of people here in my office, who want to know the fix of this one, the previous bug has a quick workaround, however I do think the person who raised "Bug 415463 - Dock menu support for Mac OS X (add "New Window" menu item)", was hoping for Bug 574229 fix as well.
thx.
Comment 2 Tom Dyas 2010-11-15 19:22:40 PST
Created attachment 490784 [details] [diff] [review]
non-working quick fix

The attached patch is what I used to try and fix the bug.  I no longer have the time any more to work on this bug, including determining why OS X didn't respect the flag set in the patch.
Comment 3 Markus Stange [:mstange] 2010-11-22 10:45:30 PST
Created attachment 492373 [details] [diff] [review]
another non-working fix

I think the trick in the solution you referenced was to change the collection behavior around the call to makeKeyAndOrderFront. This patch does that, and it sometimes works, but most of the time the new window and the window on the other space immediately swap places...
Comment 4 Alek Slater 2012-04-15 19:57:31 PDT
This bug is still present, and not fixed after 2 years?
Is it not just using a key like NSWindowCollectionBehaviorMoveToActiveSpace, or something related to that (all in the NSWindow documentation).
Comment 5 Josh Aas 2012-08-03 18:15:48 PDT
Taking, I've made some progress on this bug.
Comment 6 Josh Aas 2012-08-03 18:49:48 PDT
Created attachment 648928 [details] [diff] [review]
fix v0.8

It took me a surprisingly long time to come up with this simple patch. The discussion earlier in this bug threw me off the correct trail for a bit.

The problem isn't how we open a new window or what its collection behavior is. The problem is simply that we active the app from our dock code, which causes a spaces switch before the new window appears. Removing this activation is the basic fix, but we still need to figure out how to activate the window after it is opened. I should be able to figure that out in the next day or two.
Comment 7 Josh Aas 2012-08-03 20:34:46 PDT
Created attachment 648939 [details] [diff] [review]
fix v1.0

I need to do some more testing and I'm not 100% sure this can't leak via the observer, but it seems to work pretty well in minimal testing.

OpenBrowserWindow() seems to be effectively asynchronous, so simply activating the application after calling OpenBrowserWindow() doesn't work. We still end up switching to the other space before the new window opens. My solution in this patch is to observe document-shown and activate when we receive the notification.

Try run here:

https://tbpl.mozilla.org/?tree=Try&rev=2e3d6d55b1d7
Comment 8 Josh Aas 2012-08-04 11:54:24 PDT
Comment on attachment 648939 [details] [diff] [review]
fix v1.0

Questions for reviewer: I'm not really sure that observing "document-shown" is the best idea, I just don't know of a better one. If that didn't fire for some reason we would misbehave and possibly leak something. Also, we might be able to execute that code sooner after the window has actually been opened.
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-07 11:54:43 PDT
Comment on attachment 648939 [details] [diff] [review]
fix v1.0

Can you just use a load listener on the window instead? Something like:

let win = OpenBrowserWindow();
win.addEventListener("load", function listener() {
  win.removeEventListener("load", listener);
  dockSupport.activateApplication(true);
});
Comment 10 Josh Aas 2012-08-07 12:24:37 PDT
Created attachment 649745 [details] [diff] [review]
fix v1.1

I like it, that is better. Thanks.
Comment 11 Josh Aas 2012-08-07 12:29:20 PDT
pushed to mozilla-inbound

http://hg.mozilla.org/integration/mozilla-inbound/rev/257580e4ccef
Comment 12 Josh Aas 2012-08-07 14:10:27 PDT
Comment on attachment 649745 [details] [diff] [review]
fix v1.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Dock integration
User impact if declined: This is a big deal for anyone who uses spaces on OS X and the patch is pretty safe. Hoping to uplift to Firefox 16.
Testing completed (on m-c, etc.): Landed on m-i today.
Risk to taking this patch (and alternatives if risky): Not very risky, easy to back out.
String or UUID changes made by this patch: None
Comment 13 Josh Aas 2012-08-07 15:41:08 PDT
pushed to mozilla-aurora

http://hg.mozilla.org/releases/mozilla-aurora/rev/0a856c99ad3b
Comment 14 Ed Morley [:emorley] 2012-08-08 09:34:00 PDT
https://hg.mozilla.org/mozilla-central/rev/257580e4ccef
Comment 15 Mihaela Velimiroviciu (:mihaelav) 2012-09-25 02:46:01 PDT
I've verified this on:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:16.0) Gecko/20100101 Firefox/16.0 beta 4

A new window is opened in the active space.

Setting the flag to Verified on Firefox 16.
Comment 16 Federico Padua (fedepad) 2012-10-05 11:08:41 PDT
I tested on :

Firefox 17.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:17.0) Gecko/17.0 Firefox/17.0
Build ID : 20121005042010

The new window is opened in the active desktop, it works fine.
Comment 17 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-05 11:12:59 PDT
Thank you Federico.

Note You need to log in before you can comment on or make changes to this bug.