956286 - nightly started from taskbar icon doesn't get startup focus

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Shell, defect, P2)

Description

[Lars Viklund]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140102030203

Steps to reproduce:

Install Nightly (2014-01-02) on Windows 8.1.
Click the icon pinned to taskbar to launch the Desktop flavor of Nightly.


Actual results:

The Nightly window that appears does not have window focus, this includes the Safe Mode box if you force it with the Shift key.

The bug only occurs when CommandExecuteHandler.exe is used as the process parent and launching from the taskbar. Running firefox.exe directly doesn't demonstrate the bug, nor does running Nightly from the Start Screen.


Expected results:

The Nightly window that appears should gain focus the same way that any other application gains it when they start.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

I presume this isn't actually about Metro but it's caused by recent Metro changes.

Comment 2

[Jim Mathies [:jimm]]

I was only able to reproduce this once for the first launch after a switch. After that launch the browser had foreground status. Can you confirm the same behavior?

Comment 3

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #2)
> I was only able to reproduce this once for the first launch after a switch.
> After that launch the browser had foreground status. Can you confirm the
> same behavior?

Oh, nm, having another window on the desktop reproduces every launch.

Comment 4

[Ioana (away)]

Reproducible on the 01/07 Firefox 28.0a2.

The window is not in focus when launching Firefox from the taskbar icon, but it is in focus when launching it from the Metro interface.

Comment 5

[Jim Mathies [:jimm]]

In some ways this is desired behavior, for example if you tap the find bar edit to bring up the osk, then hit the windows button twice to flip to the start screen and back, the osk will slide back up when fx opens (with the focus set to the find bar edit).

So I think the fix here is to hide the find bar when a flyout is displayed. We do this for the tab strip and the nav bar already.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

Jim, I don't entirely understand what you're saying. This bug is about Desktop (not Metro) Nightly not getting focus on being started, which can never be desired behavior? It's filed in Metro because it's most likely caused by Metro changes.

Comment 7

[Jim Mathies [:jimm]]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #6)
> Jim, I don't entirely understand what you're saying. This bug is about
> Desktop (not Metro) Nightly not getting focus on being started, which can
> never be desired behavior? It's filed in Metro because it's most likely
> caused by Metro changes.

sorry that comment was supposed to go in bug 952989.

Comment 8

[Jim Mathies [:jimm]]

Attachment: patch v.1

Comment 9

[Jim Mathies [:jimm]]

Attachment: patch v.1

Comment 10

[Jim Mathies [:jimm]]

Attachment: patch v.1

The fix plus some cleanup. The issue here was that the ceh had foreground rights, which we need to hand off to the browser we launch. Switched to CreateProcess so I could specify ShowWindow params and have quick access to the process id.

Note I first tried this with shell execute which can also return the process handle, but it didn't work. Not sure why but CreateProcess did so I'm using that.

Comment 11

[Brian R. Bondy [:bbondy]]

Comment on attachment 8359393 [details] [diff] [review]
patch v.1

Review of attachment 8359393 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp
@@ +605,5 @@
> +  startInfo.cb = sizeof(STARTUPINFO);
> +  startInfo.dwFlags = STARTF_USESHOWWINDOW;
> +  startInfo.wShowWindow = SW_SHOWNORMAL;
> +
> +  BOOL result = 

nit: trailing whitespace

@@ +606,5 @@
> +  startInfo.dwFlags = STARTF_USESHOWWINDOW;
> +  startInfo.wShowWindow = SW_SHOWNORMAL;
> +
> +  BOOL result = 
> +    CreateProcessW(aBrowserPath, const_cast<LPWSTR>(params.GetBuffer()),

nit: Looks like you're trying to use const_cast to remove constness from params.GetBuffer() but I think that's already a non const pointer.

@@ +615,5 @@
> +  }
> +  // Hand off foreground/focus rights to the browser we create. If we don't
> +  // do this the ceh will keep ownership causing desktop firefox to launch
> +  // deactivated.
> +  AllowSetForegroundWindow(procInfo.dwProcessId);

Let's log a warning when this fails

Comment 12

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #11)
> @@ +606,5 @@
> > +  startInfo.dwFlags = STARTF_USESHOWWINDOW;
> > +  startInfo.wShowWindow = SW_SHOWNORMAL;
> > +
> > +  BOOL result = 
> > +    CreateProcessW(aBrowserPath, const_cast<LPWSTR>(params.GetBuffer()),
> 
> nit: Looks like you're trying to use const_cast to remove constness from
> params.GetBuffer() but I think that's already a non const pointer.

that should have been static_cast. updated.

Comment 13

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/fx-team/rev/2fb50e84768b

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2fb50e84768b

Comment 15

[Manuela Muntean [Away]]

For iteration #22, verified as fixed with latest Nightly on 8.1 64-bit: after clicking the icon pinned to taskbar to launch Nightly in Desktop mode, the Nightly window that appears has focus.

Comment 16

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b3056af995be

Comment 17

[u279076]

Kamil, please verify this is fixed in the latest Aurora build.

Comment 18

[Kamil Jozwiak [:kjozwiak]]

Went through the following verification without any issues. Used the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-02-05-00-40-01-mozilla-aurora/

- Ensured that Firefox has focus when launching from the pinned icon on the task bar
- Ensured that Firefox has focus when launching from the desktop shortcut
- Ensured that Firefox has focus when launching from the firefox.exe located in the installation folder
- Ensured that Firefox has focus when switching from metrofx to firefox desktop
- Went through all of the above test cases while I had several applications opened on the desktop
- Went through all of the above test cases using different variations of snapped view