If a change tab r ALT+TAB comes in after full-screen is requested, but before the request is actioned, we should cancel full-screen, as otherwise the window/document still goes full-screen, and in the change-tab case the document with the full-screen styles applied is in a background tab. Detecting change tab is easy, we can do this by checking whether the documents which receive mozfullscreenchange are descendent from the currently focused tab in browser.js. Detecting ALT+TAB should be easy, we can just track whether the window has been deactivated in browser.js, and exit full-screen if the tab's window has been deactivated in mozfullscreenchange. Unfortunately this only works on Mac & Linux, as there's a bug in Windows event handling code which means we don't always send a deactivate event on ALT+TAB while changing to full-screen, so I'm going to have to fix that too...

So, the problem is on Windows we're not receiving a "deactivate" event on the chrome window if we ALT+TAB (or switch app with mouse) while we transition to full-screen. This is caused by a problem win32 event handling in widget/src/windows/nsWindow.cpp. When our window loses focus for whatever reason, we receive two Win32 events, WM_ACTIVATE [1] and WM_KILLFOCUS [2]. Unfortunately the order of these events varies depending on the way we lost focus, but we always try to call DispatchFocusToTopLevelWindow(NS_DEACTIVATE) on the second of these two events. The three cases are which cause our top-level window to lose focus: 1. Switch app (with mouse or ALT+TAB) We receive WM_ACTIVATE msg, with WA_INACTIVE flag and HIWORD(wParam)=0. We set nsWindow::sJustGotDeactivate=true. Then we get WM_KILLFOCUS, and see that sJustGotDeactivate==true, so we call DispatchFocusToTopLevelWindow(NS_DEACTIVATE). 2. Minimize top-level window: We receive WM_KILLFOCUS msg. Next we receive a WM_ACTIVATE msg with WA_INACTIVE flag. It has HIWORD(lParam)==32, so we know the window has been minimized, and so we call DispatchFocusToTopLevelWindow(NS_DEACTIVATE). 3. Switch app while going full-screen case: We receive WM_KILLFOCUS first. Then we get WM_ACTIVATE with WA_INACTIVE flag, but with HIWORD(lParam)==0 (since the window isn't minimized at this stage), so we *don't* call DispatchFocusToTopLevelWindow(NS_DEACTIVATE). But we need to, otherwise the chrome window won't get a "deactivate" message dispatched, chrome won't know we've lost focus, and we won't know that we need to exit from full-screen. So... I suggest that we add a new static flag similar to nsWindow::sJustGotDeactivate called nsWindow::sJustGotKillFocus, which we set to true when we get WM_KILLFOCUS, and set to false when we get WM_SETFOCUS. When we receive a WM_ACTIVATE+WA_INACTIVE message, we call DispatchFocusToTopLevelWindow(NS_DEACTIVATE) if |(HIWORD(wParam) || sJustGotKillFocus)| (as opposed to if |(HIWORD(wParam))| as currently [3]). Then if we ever get a de-activate notification after a kill-focus notification, we'll always call DispatchFocusToTopLevelWindow(NS_DEACTIVATE). This could even make nsWindow::sJustGotDeactivate redundant, but I'm not quite brave enough to seriously suggest doing that this close to an Aurora uplift... [1] http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#5198 [2] http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#5273 [3] http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#5212

Patch 1 of 2. Add nsWindow::sJustGotKillFocus to ensure we call DispatchFocusToTopLevelWindow(NS_DEACTIVATE) when we lose focus while transitioning to full-screen mode. I'll wait for my try push results to come back before requesting review.

Track activate and deactivate events on the top-level window, and exit full-screen if the top-level window lost focus on the transition to full-screen. Note we can't merge the {de}activate listener I'm adding here with the existing FullScreen.exitDomFullScreen "deactivate" listener, since receive "activate" and "deactivate" events during the transition to full-screen while document.mozFullScreen==true and window.fullScreen==true (on Windows at least), making it impossible to distinguish between the entering-full-screen-and-focus-is-toggling case and the in-full-screen-and-lost-focus case. Will request review when try results come back...

My previous proposal of adding nsWindow::sJustGotKillFocus broke dom/test/mochitest/chrome/test_focus.xul and doesn't cover all cases, sometimes we can still receive a WM_KILLFOCUS without receiving a WM_ACTIVATE with WA_INACTIVE flag before/after that. So just instead just notify the focus manager when we receive a WM_KILLFOCUS message with !wParam, as that means another app has taken focus from us.

Detect if the mozfullscreenchange event for entering into full-screen is targeted at a document which is not descendent from the selected tab, and exit full-screen if so. This detects if the user changes to another tab before the asynchronous full-screen request has been processed, and exits full-screen if they have. Also detect if the active window isn't the chrome window. This detects if the user changes to another app before the asynchronous full-screen request has been processed, and exits full-screen if they have.

The tests are much more sensitive to focus now, so I had to alter the tests to use SimpleTest.waitForFocus() to prevent orange on Linux. Try push with these 3 patches applied: https://tbpl.mozilla.org/?tree=Try&rev=3e9c88c6988e

Comment on attachment 572414 [details] [diff] [review] Patch 2 v2: Detect change tab and lose focus during request full-screen >+ let targetDoc = event.target.ownerDocument ? event.target.ownerDocument : event.target; >+ if (targetDoc != document) { >+ // However, if we receive a "mozfullscreenchange" event for a document >+ // which is not a subdocument of the currently selected tab, we know that >+ // we've switched tabs since the request to enter full-screen was made, >+ // so we should exit full-screen since the "full-screen document" isn't >+ // acutally visible. >+ if (!this.isDocInCurrentTab(targetDoc)) { >+ document.mozCancelFullScreen(); >+ } Why is this needed, given that we already exit DOM FS mode when switching tabs? This code isn't e10s-safe, as it assumes direct access to the content document.

Comment on attachment 572414 [details] [diff] [review] Patch 2 v2: Detect change tab and lose focus during request full-screen This is needed because mozRequestFullScreen() was made asynchronous in bug 695935, so if you switch tab right after the request is posted but before the request is processed the existing code can't detect the change. We want this to ship in Firefox 10. e10s isn't shipping until after Firefox 10, so I will make this work in e10s after Firefox 10.

Comment on attachment 572414 [details] [diff] [review] Patch 2 v2: Detect change tab and lose focus during request full-screen >+ // Returns true of document d is a subdocument of the currently selected tab. >+ isDocInCurrentTab: function(d) { >+ var rootContentDoc = gBrowser.selectedBrowser.contentDocument; >+ var doc = d; >+ while (doc) { >+ if (doc == rootContentDoc) { >+ return true; >+ } >+ var parent = doc.defaultView.parent.document; >+ if (parent == doc) { >+ // Document's window's parent is itself; this is the chrome document. >+ return false; >+ } >+ doc = parent; >+ } >+ return false; >+ }, >+ // However, if we receive a "mozfullscreenchange" event for a document >+ // which is not a subdocument of the currently selected tab, we know that >+ // we've switched tabs since the request to enter full-screen was made, >+ // so we should exit full-screen since the "full-screen document" isn't >+ // acutally visible. >+ if (!this.isDocInCurrentTab(targetDoc)) { This can be simplified as 'if (targetDoc.defaultView.top != gBrowser.contentWindow)', can't it?

Yeah, looks like it. Thanks!

Comment on attachment 572413 [details] [diff] [review] Patch 1 v2: DispatchFocusToTopLevelWindow on WM_KILLFOCUS !wParam nice

https://hg.mozilla.org/integration/mozilla-inbound/rev/bea7ecf9084e https://hg.mozilla.org/integration/mozilla-inbound/rev/de9859b56d6e https://hg.mozilla.org/integration/mozilla-inbound/rev/afca83588ca2

https://hg.mozilla.org/mozilla-central/rev/bea7ecf9084e https://hg.mozilla.org/mozilla-central/rev/de9859b56d6e https://hg.mozilla.org/mozilla-central/rev/afca83588ca2

Note I backed out bea7ecf9084e from all branches, and it went into a chemspill release (10.0.1, and esr 10.0.1) because it was causing bug 718939. Filed bug 725466 to fix the backout.