729872 - Window deactivate while in HTML5 fullscreen causes text fields to not show cursor or selection

Status: VERIFIED FIXED

(Core :: General, defect)

Description

[Paul Silaghi, QA [:pauly]]

Steps to reproduce:
1. Enable html5 http://www.youtube.com/html5
2. Open http://www.youtube.com/watch?v=R8Or0qxb0-k
3. Enter youtube fullscreen
4. Press ALT+TAB

Actual results:
Youtube leaves fullscreen mode. Location bar, search bar become unresponsive

Expected results:
Location bar, search bar should work properly

Comment 1

[Alice0775 White]

Bisect with  full-screen-api.enabled=true;

Regression window(m-c),
Cannot reproduce:
http://hg.mozilla.org/mozilla-central/rev/e6893e6c883f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111104 Firefox/10.0a1 ID:20111104020439
Can reproduce:
http://hg.mozilla.org/mozilla-central/rev/5ebd59b5a94a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111104 Firefox/10.0a1 ID:20111104112939
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e6893e6c883f&tochange=5ebd59b5a94a


Regression window(m-i),
Cannot reproduce:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4f57f89653df
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111103 Firefox/10.0a1 ID:20111103223340
Can reproduce:
http://hg.mozilla.org/integration/mozilla-inbound/rev/e7e0950aec83
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111103 Firefox/10.0a1 ID:20111104003338
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4f57f89653df&tochange=e7e0950aec83
Suspect: Bug 685402

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Text fields still work, we just don't show the caret or highlight the text selection. Toggling F11 fullscreen mode makes text fields behave as expected again.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

So there's two problems here:

1. nsFocusManager dispatches "deactivate" synchronously, and then does some more stuff related to blurring the focused window. The "deactivate" listener in browser.js exits fullscreen, which lowers and raises the focused window, which messes up the blur logic, and is what's causing the text widgets to not work upon focus after deactivate. Making the "deactivate" listener exit fullscreen in a timeout works around this issue, and text inputs work after deactivate in fullscreen. This also fixes bug 726474.

2. nsGlobalWindow::SetFullScreen() (on Windows at least) causes the top level browser window to be lowered and then raised. So after the top-level window has been blurred from alt+tab, we'll call SetFullScreen(false) to exit fullscreen mode, which will lower and then raise the window, causing the window to be drawn as if it's been raised (focused, and the window manager's active window), even though it's not the window manager's active window.

I'll spin off problem 2 into another bug.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch

Fix as I describe in comment 1, exit fullscreen on deactivate asynchronously, so as to not confuse the focus manager's blur code.

Comment 5

[Dão Gottwald [:dao]]

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

Please implement FullScreen.handleEvent and call window.addEventListener("deactivate", this)

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 601740 [details] [diff] [review]
Patch v2

Much nicer, thanks.

>   exitDomFullScreen : function(e) {
>     document.mozCancelFullScreen();
>   },

The 'e' argument is obsolete now.

>+  handleEvent: function (event) {
>+    switch (event.type) {
>+      case "deactivate":
>+        // We must call exitDomFullScreen asynchronously, since "deactivate" is
>+        // dispatched in the middle of the focus manager's window lowering code,
>+        // and the focus manager gets confused if we exit fullscreen mode in the
>+        // middle of window lowering. See bug 729872.
>+        setTimeout(this.exitDomFullScreen, 0);

Please make this this.exitDomFullScreen.bind(this) even though it doesn't currently matter given how exitDomFullScreen is implemented.

>+      window.addEventListener("deactivate", this, true);

Any reason why this isn't just window.addEventListener("deactivate", this)? I.e. why is this a capturing listener?

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Dão Gottwald [:dao] from comment #7)
> Comment on attachment 601740 [details] [diff] [review]
> >+      window.addEventListener("deactivate", this, true);
> 
> Any reason why this isn't just window.addEventListener("deactivate", this)?
> I.e. why is this a capturing listener?

Hmmm, I don't think it needs to be, I'll make it non-capturing.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b65b4ac15cd

Comment 10

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/9b65b4ac15cd

Comment 11

[Paul Silaghi, QA [:pauly]]

Verified fixed on FF13:
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20120312 Firefox/13.0a1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120304 Firefox/13.0a1
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20120311 Firefox/13.0a1

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 601740 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: Exiting full-screen by ALT+TABing to another window will cause text fields in Firefox UI to appear to be unfocused, even though they are; so the user may not realise they can type in the Awesome/search bars.
Testing completed (on m-c, etc.): Been on m-c for a while.
Risk to taking this patch (and alternatives if risky): Pretty low risk, just makes an event listener async.
String changes made by this patch: None.

Once the Mar 13 merge is complete, this fix will already be on Aurora, so I'm only requesting approval for beta.

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 601740 [details] [diff] [review]
Patch v2

[Triage Comment]
Issue with a recent new feature and early enough in Beta 12 to take a fix like this.

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6828271ffa35

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

It would be good to fix this bug on ESR10 as well.

Comment 16

[Alex Keybl [:akeybl]]

We don't expect this to be a major pain point for ESR users, and it's not a critical security/stability fix - let's let this ride the train and get fixed in ESR17.

Comment 17

[Paul Silaghi, QA [:pauly]]

Still an issue on FF 12b2: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0

Comment 18

[Paul Silaghi, QA [:pauly]]

Verified fixed on FF 12b3 on Win 7/64, Ubuntu 11.10 and Mac OS X 10.6