Last Comment Bug 699885 - ALT+TAB/CTRL+TAB while entering DOM full-screen should exit full-screen
: ALT+TAB/CTRL+TAB while entering DOM full-screen should exit full-screen
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Chris Pearce (:cpearce)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 725466 718939 723046 723204
Blocks: 545812 695935 700151
  Show dependency treegraph
 
Reported: 2011-11-04 12:14 PDT by Chris Pearce (:cpearce)
Modified: 2012-02-08 14:05 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch 1 v1: Add nsWindow::sJustGotKillFocus (3.91 KB, patch)
2011-11-04 16:59 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 2 v1: Exit full-screen if window deactivated during transition to full-screen (3.63 KB, patch)
2011-11-04 17:13 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 1 v2: DispatchFocusToTopLevelWindow on WM_KILLFOCUS !wParam (1.22 KB, patch)
2011-11-07 02:22 PST, Chris Pearce (:cpearce)
roc: review+
Details | Diff | Splinter Review
Patch 2 v2: Detect change tab and lose focus during request full-screen (2.85 KB, patch)
2011-11-07 02:33 PST, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 3: Test changes (6.43 KB, patch)
2011-11-07 02:35 PST, Chris Pearce (:cpearce)
roc: review+
Details | Diff | Splinter Review
Patch 2 v3: Detect change tab and lose focus during request full-screen (2.32 KB, patch)
2011-11-07 15:31 PST, Chris Pearce (:cpearce)
dao+bmo: review+
Details | Diff | Splinter Review

Description Chris Pearce (:cpearce) 2011-11-04 12:14:14 PDT
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...
Comment 1 Chris Pearce (:cpearce) 2011-11-04 16:25:33 PDT
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
Comment 2 Chris Pearce (:cpearce) 2011-11-04 16:59:02 PDT
Created attachment 572128 [details] [diff] [review]
Patch 1 v1: Add nsWindow::sJustGotKillFocus

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.
Comment 3 Chris Pearce (:cpearce) 2011-11-04 17:13:31 PDT
Created attachment 572131 [details] [diff] [review]
Patch 2 v1: Exit full-screen if window deactivated during transition to full-screen

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...
Comment 4 Chris Pearce (:cpearce) 2011-11-07 02:22:16 PST
Created attachment 572413 [details] [diff] [review]
Patch 1 v2: DispatchFocusToTopLevelWindow on WM_KILLFOCUS !wParam

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.
Comment 5 Chris Pearce (:cpearce) 2011-11-07 02:33:25 PST
Created attachment 572414 [details] [diff] [review]
Patch 2 v2: Detect change tab and lose focus during request full-screen

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.
Comment 6 Chris Pearce (:cpearce) 2011-11-07 02:35:15 PST
Created attachment 572416 [details] [diff] [review]
Patch 3: Test changes

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 7 Dão Gottwald [:dao] 2011-11-07 04:06:08 PST
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 8 Chris Pearce (:cpearce) 2011-11-07 10:29:54 PST
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 9 Dão Gottwald [:dao] 2011-11-07 15:18:38 PST
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?
Comment 10 Chris Pearce (:cpearce) 2011-11-07 15:26:17 PST
Yeah, looks like it. Thanks!
Comment 11 Chris Pearce (:cpearce) 2011-11-07 15:31:49 PST
Created attachment 572644 [details] [diff] [review]
Patch 2 v3: Detect change tab and lose focus during request full-screen
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-07 15:40:24 PST
Comment on attachment 572413 [details] [diff] [review]
Patch 1 v2: DispatchFocusToTopLevelWindow on WM_KILLFOCUS !wParam

nice
Comment 15 Chris Pearce (:cpearce) 2012-02-08 14:05:20 PST
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.

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