Closed Bug 831489 Opened 9 years ago Closed 9 years ago

Facebook Messenger For Firefox Steals Focus

Categories

(Firefox Graveyard :: SocialAPI, defect)

21 Branch
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 22

People

(Reporter: unique.ek, Assigned: markh)

References

Details

Attachments

(1 file, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130116 Firefox/21.0
Build ID: 20130116040201

Steps to reproduce:

Enabled Facebook Messenger For Firefox


Actual results:

Whenever i recieve a new message a new chat windows appears and steals focus.New chat windows also steal focus from the previous chat windows even if you are typing something on them.
This is quite disturbing when you are trying to write something.


Expected results:

The new chat windows should wait silently till i want to interact with them.
Component: Untriaged → SocialAPI
I agree we should not be doing this.  Even if another chat is currently focused we should not focus the new chat.  It seems that about the only time we should programatically focus a chatbar is (a) when the keyboard shortcut for this purpose is used and (b) when we minimize/collapse/close a chat that currently has focus.

Gavin/Shane - what do you think?
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Chat windows opened from the worker (ie. not from user initiated events) should not take focus.
(In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> Chat windows opened from the worker (ie. not from user initiated events)
> should not take focus.

Note that if the sidebar is visible it seems to open incoming chats (as the new chat window appears non-minimized).  Thus, I'm not sure we can simply say "by the worker"
Shortcut idea sounds fine for me.But i think instead of focusing to the next chat window when the current has been closed we should focus on the previous focused object.The object which had the focus before we opened the chat window.That way user can browse a website or whatever they are doing then reply to chat and after closing it the user can continue their usage.
(In reply to Mark Hammond (:markh) from comment #3)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> > Chat windows opened from the worker (ie. not from user initiated events)
> > should not take focus.
> 
> Note that if the sidebar is visible it seems to open incoming chats (as the
> new chat window appears non-minimized).  Thus, I'm not sure we can simply
> say "by the worker"

I guess my point was more along the line that if it is opened by user action it should be focused, otherwise current focus should not change.  (ie. user clicks on friend in sidebar to open chat, it gets focused.)
nsIDOMWindowUtils.isHandlingUserInput() appears to be perfect for this.  I'm coming up with a patch + tests that will also solve bug 830623 without the workaround in that bug.
Assignee: nobody → mhammond
This patch:
* 1/2 decouples focus from selection.  It is possible to "select" a chat without focusing it (1/2 decoupled, as it's not possible to focus a non-selected chat).  Chats are not focused by default (even new chats, which are selected by default).

* Only focuses a chat if the chat request is as a direct result of user input.  This has the side-effect of fixing bug 830623 - chats opened by the worker are never classified as "direct user input".

* Uses nsIFocusManager::MoveFocus instead of advanceFocusIntoSubtree, as the latter doesn't do the right thing if the chat has no selectable children.

* Tests most scenarios related to this.

There is some kind of bug-karma here too:

* Facebook actually opens chats via the sidebar as a result of a message from the worker (ie, clicking on a person in the sidebar seems to send and receive a worker message before the chat is requested).  This means that no Facebook chats are considered "direct user input" and thus would never be focused (which would be bad) - but...

* Facebook also calls window.focus() after the chat is created.  As a result of bug 604289 (or a bug very very closely related), this window.focus() call works when clicking on the sidebar.  Thus, chats opened by the Facebook sidebar actually do focus as expected.  However, this means that Facebook will need to tweak their sidebar code else fixing bug 604289 will prevent their chat windows ever getting focus (specifically, they will need to open the chat directly in the "onclick" handler rather than in a worker message handler).
Attachment #715119 - Flags: feedback?(mixedpuppy)
Attachment #715119 - Flags: feedback?(gavin.sharp)
Comment on attachment 715119 [details] [diff] [review]
Decouple chat selection from focus

># HG changeset patch
># Parent 081cf5b0121e8e8c0133c68a85ff4fda9e2b5c63
># User Mark Hammond <mhammond@skippinet.com.au>
>Bug 831489 - prevent social chats from stealing focus.  r=???
>
>diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js
>--- a/browser/base/content/browser-social.js
>+++ b/browser/base/content/browser-social.js
>@@ -333,19 +333,19 @@ let SocialChatBar = {
>   // mode chats are available, but not shown.
>   get isAvailable() {
>     return SocialUI.enabled && Social.haveLoggedInUser();
>   },
>   // Does this chatbar have any chats (whether minimized, collapsed or normal)
>   get hasChats() {
>     return !!this.chatbar.firstElementChild;
>   },
>-  openChat: function(aProvider, aURL, aCallback, aMode) {
>+  openChat: function(aProvider, aURL, aCallback, aMode, aFocus) {
>     if (this.isAvailable)
>-      this.chatbar.openChat(aProvider, aURL, aCallback, aMode);
>+      this.chatbar.openChat(aProvider, aURL, aCallback, aMode, aFocus);
>   },

Rather than checking for isHandlingUserInput in MozSocialAPI, couldn't we check that in chatbar.openChat? 

>+      <method name="_isChatFocused">
>+        <parameter name="aChatbox"/>
>+        <body><![CDATA[
>+          let fm = Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager);
>+          let fe = fm.focusedElement;
>+          if (!fe)
>+            return false;

given my comment below, we could just use commandDispatcher.focusedElement.

>+          // If there are no focusable elements, the iframe itself will have
>+          // focus, otherwise an elt in the iframe.
>+          // XXX - Is there a better way?
>+          return fe == aChatbox.iframe ||
>+                 fe.ownerDocument == aChatbox.iframe.contentDocument;

IMO if the document has no focusable elements it is ok for the iframe to get focus.

>         ]]></body>
>       </method>
> 
>       <property name="selectedChat">
>         <getter><![CDATA[
>           return this._selectedChat;
>         ]]></getter>
>         <setter><![CDATA[
>@@ -163,17 +179,16 @@
>           // * need to handle either current or new being null.
>           if (this._selectedChat != val) {
>             if (this._selectedChat) {
>               this._selectedChat.removeAttribute("selected");
>             }
>             this._selectedChat = val;
>             if (val) {
>               this._selectedChat.setAttribute("selected", "true");
>-              this.focus();
>             }
>           }
>           if (val) {
>             this._selectedChat.removeAttribute("activity");
>           }
>         ]]></setter>
>       </property>
> 
>@@ -226,19 +241,22 @@
>         <body><![CDATA[
>           // Select a different chat (as the currently selected one is no
>           // longer suitable as the selection - maybe it is being minimized or
>           // closed.)  We only select non-minimized and non-collapsed chats,
>           // and if none are found, set the selectedChat to null.
>           // It's possible in the future we will track most-recently-selected
>           // chats or similar to find the "best" candidate - for now though
>           // the choice is somewhat arbitrary.
>+          let moveFocus = this.selectedChat && this._isChatFocused(this.selectedChat);
>           for (let other of this.children) {
>             if (other != this.selectedChat && !other.minimized && !other.collapsed) {
>               this.selectedChat = other;
>+              if (moveFocus)
>+                this.focus();
>               return;
>             }
>           }
>           // can't find another - so set no chat as selected.
>           this.selectedChat = null;
>         ]]></body>
>       </method>
> 
>@@ -424,22 +442,25 @@
>           aChatBox.setAttribute("src", aURL);
>         ]]></body>
>       </method>
>       <method name="openChat">
>         <parameter name="aProvider"/>
>         <parameter name="aURL"/>
>         <parameter name="aCallback"/>
>         <parameter name="aMode"/>
>+        <parameter name="aFocus"/>

per my first comment, can we just check for user input within, rather than passing it in.

>new file mode 100644
>--- /dev/null
>+++ b/browser/base/content/test/social/browser_social_chatwindowfocus.js


Have you tested using tab/shift+tab to move focus between chat windows?
Attachment #715119 - Flags: feedback?(mixedpuppy) → feedback+
Attached patch updated, more tests (obsolete) — Splinter Review
(In reply to Shane Caraveo (:mixedpuppy) from comment #8)

> Rather than checking for isHandlingUserInput in MozSocialAPI, couldn't we
> check that in chatbar.openChat? 

This new version checks for that in browser-social's openChat function - it makes sense to me to keep the policy for focus external to the chat itself and also has the added advantage of not needing the "check if we should focus" in both the "opening a new chat" and "opening an existing chat" branches.  I'm obviously not too bothered by this though, so if someone *really* wants it in socialchat.xml, I'll oblige :)

> given my comment below, we could just use commandDispatcher.focusedElement.

I'd prefer to stick with nsIFocusManager seeing as that is how we are setting focus - however, _isChatFocused() is now quite a bit more complicated.  Our chat HTML has an embedded <iframe> which causes some complications - once that iframe gets focus, it is a different document than the chat's iframe.

Now we walk up the parent chain using nsIDocShellTreeItem which crosses document boundaries.  Fortunately this method is only rarely used, so I don't think performance will be an issue, but if anyone knows a better way to implement this function I'd love to hear it.  Ideally nsIFocusManager should be able to tell us this, but doesn't seem able to.

It would even be possible to drop this method, but it would mean we lose the feature of auto-focusing a new chat when the focused chat is minimized - this seems like a nice feature to keep though.

> IMO if the document has no focusable elements it is ok for the iframe to get
> focus.

Agreed - and that should happen - although it is worth noting that the tests don't exercise that (as our test chat window has focusable elements).  I'm not too bothered by this lack of testing though, as I can't imagine a real chat window not having focusable elements...

> per my first comment, can we just check for user input within, rather than
> passing it in.

See above - we no longer pass it in, but browser-social is what checks isHandlingUserInput and explicitly calls .focus()

> Have you tested using tab/shift+tab to move focus between chat windows?

I've added that test (which is awesome, as that is what uncovered a problem with the previous patch when the chat has an embedded iframe.)  FWIW though, TAB doesn't work with Facebook's chats, either with or without this patch.

Apart from the ugliness of _isChatFocused(), I'm happy with this version so jumping directly to a review request from Gavin (but a drive-by from Shane is also welcome!)
Attachment #715119 - Attachment is obsolete: true
Attachment #715119 - Flags: feedback?(gavin.sharp)
Attachment #715336 - Flags: review?(gavin.sharp)
(In reply to Mark Hammond (:markh) from comment #9)
> Created attachment 715336 [details] [diff] [review]
> updated, more tests

> I'd prefer to stick with nsIFocusManager seeing as that is how we are
> setting focus - however, _isChatFocused() is now quite a bit more
> complicated.  Our chat HTML has an embedded <iframe> which causes some
> complications - once that iframe gets focus, it is a different document than
> the chat's iframe.

I could be wrong, but I thought commandDispatcher would point into the frame handling commands, so it should point into any embedded iframes as well.

> Now we walk up the parent chain using nsIDocShellTreeItem which crosses
> document boundaries.  Fortunately this method is only rarely used, so I
> don't think performance will be an issue, but if anyone knows a better way
> to implement this function I'd love to hear it.  Ideally nsIFocusManager
> should be able to tell us this, but doesn't seem able to.

You shouldn't need to walk like that.  Just get the containing browser and compare it with the iframe.

    var containingBrowser = window.QueryInterface(Ci.nsIInterfaceRequestor)
                                  .getInterface(Ci.nsIWebNavigation)
                                  .QueryInterface(Ci.nsIDocShell)
                                  .chromeEventHandler;

I feel like there was a way to easily see if commandDispatcher.focusedElement was a child of iframe, crossing embedded frames, but I cannot find that right now.


> It would even be possible to drop this method, but it would mean we lose the
> feature of auto-focusing a new chat when the focused chat is minimized -
> this seems like a nice feature to keep though.
> 
> > IMO if the document has no focusable elements it is ok for the iframe to get
> > focus.
> 
> Agreed - and that should happen - although it is worth noting that the tests
> don't exercise that (as our test chat window has focusable elements).  I'm
> not too bothered by this lack of testing though, as I can't imagine a real
> chat window not having focusable elements...

webrtc video-only frames?


> > Have you tested using tab/shift+tab to move focus between chat windows?
> 
> I've added that test (which is awesome, as that is what uncovered a problem
> with the previous patch when the chat has an embedded iframe.)  FWIW though,
> TAB doesn't work with Facebook's chats, either with or without this patch.

Yeah, I believe they are handling keypresses and canceling the event.

> Apart from the ugliness of _isChatFocused(), I'm happy with this version so
> jumping directly to a review request from Gavin (but a drive-by from Shane
> is also welcome!)


I think that can/should be simplified.
(In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> (In reply to Mark Hammond (:markh) from comment #9)
> > Created attachment 715336 [details] [diff] [review]
> > updated, more tests
> 
> > I'd prefer to stick with nsIFocusManager seeing as that is how we are
> > setting focus - however, _isChatFocused() is now quite a bit more
> > complicated.  Our chat HTML has an embedded <iframe> which causes some
> > complications - once that iframe gets focus, it is a different document than
> > the chat's iframe.
> 
> I could be wrong, but I thought commandDispatcher would point into the frame
> handling commands, so it should point into any embedded iframes as well.

commandDispatch and nsIFocusManager always report identical objects for both focusedElement and focusedWindow.  ie, best I can tell, there is zero difference between them (even though the command dispatcher doesn't seem to be a simple delegate to nsIFocusManager)

> > Now we walk up the parent chain using nsIDocShellTreeItem which crosses
> > document boundaries.  Fortunately this method is only rarely used, so I
> > don't think performance will be an issue, but if anyone knows a better way
> > to implement this function I'd love to hear it.  Ideally nsIFocusManager
> > should be able to tell us this, but doesn't seem able to.
> 
> You shouldn't need to walk like that.  Just get the containing browser and
> compare it with the iframe.
> 
>     var containingBrowser = window.QueryInterface(Ci.nsIInterfaceRequestor)
>                                   .getInterface(Ci.nsIWebNavigation)
>                                   .QueryInterface(Ci.nsIDocShell)
>                                   .chromeEventHandler;

Ahh, awesome.  For some reason I expected that to return the top-level chrome window.  New patch has this instead.
Attachment #715336 - Attachment is obsolete: true
Attachment #715336 - Flags: review?(gavin.sharp)
Attachment #715731 - Flags: review?(gavin.sharp)
Comment on attachment 715731 [details] [diff] [review]
Updated with better "is iframe focused" check

Hmm, is MOVEFOCUS_ROOT really correct? "move focus to the root element in the document" doesn't really seem like what we want. Don't we want focus to go to the first focusable element (i.e. chat input), rather than to the root element?

You should be able to use Services.focus instead of "fm" in socialchat.xml (and also in the test).

In general focus is best left untouched unless you really really need to mess with it. From my naive understanding of the current state of affairs, we want to focus a chat when:

- it is opened (and we are handling user input)
- when it is "unminimized" (which should only be possible in response to user input)
- when you close another focused chat

Assuming those are complete you calls to focus() seem correct, and decoupling focus setting from the selectedChat setter seems virtuous.

Test coverage looks good!

r=me with the moveFocus flag issue sorted out, sorry for the delay.
Attachment #715731 - Flags: review?(gavin.sharp) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6ea0dbf2b63b for the same failure your try run shows on Win7 opt, though on inbound (at least so far, for the stuff that's finished) it seems more fond of happening on OS X.
Something in this push caused frequent Linux64 debug mochitest-browser-chrome failures. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e067a8b6b4b1

https://tbpl.mozilla.org/php/getParsedLog.php?id=20224982&tree=Mozilla-Inbound

07:51:34     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_chatwindowfocus.js | exactly 1 chat open
07:51:34     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_chatwindowfocus.js | chat is minimized
07:51:34     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_chatwindowfocus.js | tab should be focused
07:51:34     INFO -  ++DOMWINDOW == 149 (0x4c97508) [serial = 2236] [outer = 0x3cbc8d8]
07:51:34     INFO -  ++DOCSHELL 0x6a1bd40 == 50 [id = 808]
07:51:34     INFO -  ++DOMWINDOW == 150 (0x5c66a68) [serial = 2237] [outer = (nil)]
07:51:34     INFO -  WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && subjPrincipal) failed: file ../../../docshell/base/nsDocShell.cpp, line 8396
07:51:34  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_chatwindowfocus.js | uncaught exception - TypeError: navigator.mozSocial is undefined at https://example.com/browser/browser/base/content/test/social/social_chat.html?id=1:10
07:51:34  WARNING -  This is a harness error.
07:51:34     INFO -  Stack trace:
07:51:34     INFO -      JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: simpletestOnerror :: line 1105
07:51:34     INFO -      native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
07:51:34     INFO -  JavaScript error: https://example.com/browser/browser/base/content/test/social/social_chat.html?id=1, line 10: navigator.mozSocial is undefined
07:51:34     INFO -  WARNING: Unable to test style tree integrity -- no content node: file ../../../layout/base/nsCSSFrameConstructor.cpp, line 8403
07:51:34     INFO -  ++DOMWINDOW == 151 (0x46bc568) [serial = 2238] [outer = 0x5c66a68]
07:51:34     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_chatwindowfocus.js | Console message: [JavaScript Error: "TypeError: navigator.mozSocial is undefined" {file: "https://example.com/browser/browser/base/content/test/social/social_chat.html?id=1" line: 10}]
07:51:34     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_chatwindowfocus.js | still 1 chat open
07:51:34     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_chatwindowfocus.js | chat no longer minimized
07:51:34     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_chatwindowfocus.js | tab should still be focused
07:51:34     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_chatwindowfocus.js | sub-test testNoFocusOnAutoRestore complete
07:51:34     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_chatwindowfocus.js | sub-test testFocusOnExplicitRestore starting
07:51:34     INFO -  ++DOCSHELL 0x7a5a730 == 51 [id = 809]
07:51:34     INFO -  ++DOMWINDOW == 152 (0x6efd808) [serial = 2239] [outer = (nil)]
07:51:34     INFO -  ++DOMWINDOW == 153 (0x7fd4498) [serial = 2240] [outer = 0x6efd808]
07:51:34     INFO -  ++DOMWINDOW == 154 (0x565ab58) [serial = 2241] [outer = 0x6efd808]
07:51:34     INFO -  ++DOCSHELL 0x6e97440 == 52 [id = 810]
07:51:34     INFO -  ++DOMWINDOW == 155 (0x78573b8) [serial = 2242] [outer = (nil)]
07:51:34     INFO -  WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && subjPrincipal) failed: file ../../../docshell/base/nsDocShell.cpp, line 8396
07:51:34     INFO -  WARNING: Unable to test style tree integrity -- no content node: file ../../../layout/base/nsCSSFrameConstructor.cpp, line 8403
07:51:34     INFO -  ++DOMWINDOW == 156 (0x5992e68) [serial = 2243] [outer = 0x78573b8]
07:51:34     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_chatwindowfocus.js | got chatbox message
07:51:34     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_chatwindowfocus.js | tab should still be focused
https://hg.mozilla.org/mozilla-central/rev/0546579b0c06
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Depends on: 847094
@unique.ek can you please verify that this is now fixed for you in the latest Firefox 22 Nightly?
Testing now.If something comes up in a few days i will let you know if its fixed or not ;)
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #19)
> @unique.ek can you please verify that this is now fixed for you in the
> latest Firefox 22 Nightly?

It looks ok here.Haven't experienced that since the fix.
Seems ok now.Haven't experienced this issue since the fix
Thanks unique.ek
Status: RESOLVED → VERIFIED
Depends on: 864683
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.