Closed Bug 874566 Opened 6 years ago Closed 6 years ago
Chat boxes should be opened in the currently active window (Linux/Mac)
See also bug 835111.
Thanks Marco, just carrying over the fact that I cannot reproduce any bad behaviour here. Chat windows always open in the currently active window for me. Please provide more context as to what exactly is happening in your case.
Status: NEW → UNCONFIRMED
Ever confirmed: false
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #1) > Thanks Marco, just carrying over the fact that I cannot reproduce any bad > behaviour here. Chat windows always open in the currently active window for > me. Please provide more context as to what exactly is happening in your case. With two windows open, the chat boxes are always opened in the last opened window and not in the currently active window. I have these three addons installed: Adblock Plus 2.2.4 DOM Inspector 2.0.14 HTTPS-Everywhere 3.2.1
Do these problems reproduce without those add-ons installed?
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #3) > Do these problems reproduce without those add-ons installed? Yes. I've tried with a clean profile, but in that case I'm in the bug 872413 situation.
Yeah, so I think we need to figure out and resolve bug 872413 before we can move forward with *this* bug. I don't think it's reasonable to try debugging this until that issue is resolved.
I can reproduce this on Linux. Works fine on Windows (and presumably also on Mac, but I haven't tried that recently). Note that if the sidebar is open, the chat opens in *all* windows currently - but this is a "feature" of facebook rather than the social api - so to test this specific case, ensure the sidebar is closed as an incoming message arrives.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Thanks Mark, can you please clarify your steps to reproduce?
STR: * Open 2 top-level windows, ensure FB social activated with sidebar closed. * Bring one of the windows to the foreground. * Send message from another computer to the FB user logged in to the machine under test. * Close the message in the window where it appeared. * Bring the second window to the foreground. * Send another message. In one of the 2 cases the window not in the foreground will have got the message.
I can't seem to reproduce this with the steps provide, Mark. Everytime the chat appears in my currently active window. Given you can reproduce can you investigate further? Is this a regression we introduced at some point (ie. does it go back to Firefox 17)? or is this an issue with Facebook's implementation?
This affects us on Mac with Talkilla as well. My STR are: 1) Open sidebar, open a new window 2) Go back to the first window. 3) Click on a user to open conversation window. Conversation window appears in second window, not the first one (and there's no indication it has done so). This appears to be a platform limitation for Mac and Linux, as can be seen in the code: http://hg.mozilla.org/mozilla-central/annotate/17a47dcef75d/toolkit/components/social/MozSocialAPI.jsm#l281
Summary: Facebook messages conversation boxes should be opened in the currently active window → Chat boxes should be opened in the currently active window (Linux/Mac)
(In reply to Mark Banner (:standard8) from comment #10) > This appears to be a platform limitation for Mac and Linux, as can be seen > in the code: > > http://hg.mozilla.org/mozilla-central/annotate/17a47dcef75d/toolkit/ > components/social/MozSocialAPI.jsm#l281 I was just writing a reply indicating why I thought that code was OK, when I discovered a subtlety I'd missed. getEnumerator() is documented as iterating "from the oldest window to the youngest" - which I'd always read as "from the least recently used to the most recently used" - but that's not what it does... It looks like we want an enumerator based on the loop used for getMostRecentWindow, at http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsWindowMediator.cpp#297 - but it doesn't seem to exist and |mTimeStamp| (which would allow us to fix this in js) doesn't seem to be exposed...
hmmm - http://mxr.mozilla.org/mozilla-central/search?string=BROKEN_WM_Z_ORDER implies things might actually be fixed. <... bugzilla searches...> OK - maybe not fixed, but looks like maybe getting traction - see bug 462222. I think the simplest is just for us to block on that and drop the BROKEN_WM_Z_ORDER code completely...
I don't see any traction in that bug. It seems to me that we need to fix the social BROKEN_WINDOW_Z_ORDER path to just use the last suitable window in the iterator (similar to what e.g. http://hg.mozilla.org/mozilla-central/annotate/582ffcd0459a/browser/modules/RecentWindow.jsm#l49 does).
No longer depends on: 462222
(In reply to :Gavin Sharp (use firstname.lastname@example.org for email) from comment #13) > It seems to me that we need to fix the > social BROKEN_WINDOW_Z_ORDER path to just use the last suitable window in > the iterator Mark pointed out on IRC that it already does this; I misread the code. The bug is just a reflection of the fact that that fallback behavior is not equivalent to the getZOrderDOMWindowEnumerator code, and indeed we would need to remove it (and find an alternative that is equivalent, if it's not getZOrderDOMWindowEnumerator) to fix this.
assigning to myself so I don't forget to test how things work without the work-around. I'll probably unassign if it doesn't work though :)
Assignee: nobody → mhammond
I tested this today. On Mac things work well without the workaround. Unfortunately, on Linux getZOrderDOMWindowEnumerator is still broken (it returned an empty enumerator, which caused social api to fail to open a chat window). However, getMostRecentWindow works on all platforms, so we can use it for the common case, and keep the existing workaround for the unfortunate case of the foreground window being a popup on Linux. Something similar already exists at http://hg.mozilla.org/mozilla-central/annotate/4887845b1142/browser/components/sessionstore/src/SessionStore.jsm#l3578
Assignee: mhammond → florian
Attachment #795522 - Flags: review?(mhammond)
Comment on attachment 795522 [details] [diff] [review] Patch Review of attachment 795522 [details] [diff] [review]: ----------------------------------------------------------------- We have an existing test testChatWindowChooser() in browser_social_chatwindow, but obviously it's not very good :) I suspect the test simply hits the case that works (ie, we create a new window and test that - it should also re-activate the first and check the chats re-appear there. I suspect that would then cause a failure on Mac *and* Linux. You patch should then fix Mac and leave Linux broken, which we can handle with a "todo" in the test. Any chance of looking into the above to ensure we have test coverage for this change?
Attachment #795522 - Flags: review?(mhammond) → feedback+
Added test coverage. Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ca572a89b2b9
Comment on attachment 796266 [details] [diff] [review] Patch v2 Review of attachment 796266 [details] [diff] [review]: ----------------------------------------------------------------- Awesome, thank you!
Attachment #796266 - Flags: review?(mhammond) → review+
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Comment on attachment 796266 [details] [diff] [review] Patch v2 [Approval Request Comment] Bug caused by (feature/regressing bug #): N/A User impact if declined: Social API chat windows are hardly usable on Mac/Linux if the user has several browser windows, as they can randomly be opened on background windows. Testing completed (on m-c, etc.): The patch has been on m-c for almost two weeks, and has an automated test. Risk to taking this patch (and alternatives if risky): Low. If we weren't so late in the cycle, I would request approval-beta too, as it's one of the top issues encountered while dogfooding Talkilla. String or IDL/UUID changes made by this patch: none.
Attachment #796266 - Flags: approval-mozilla-aurora?
Comment on attachment 796266 [details] [diff] [review] Patch v2 Thanks for the rationale - no issue with this skipping up the trains.
Attachment #796266 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Pushed this to try above mozilla-aurora's current tip to check that the patch passes unit tests there: https://tbpl.mozilla.org/?tree=Try&rev=aed02a848f40
(In reply to Florian Quèze [:florian] [:flo] from comment #24) > Pushed this to try above mozilla-aurora's current tip to check that the > patch passes unit tests there: > https://tbpl.mozilla.org/?tree=Try&rev=aed02a848f40 This didn't pass. Turns out http://hg.mozilla.org/mozilla-central/rev/d0064759d817 which landed on m-c before my patch here but after the latest uplift changes the test code a bit. I'm attaching a fixed patch for aurora, and pushed it to try again: https://tbpl.mozilla.org/?tree=Try&rev=1b687df66ff2
I could reproduce the issue on Linux 64-bit with Fx 23.0.1 using STR from Comment 8. Verified fixed on Fx 25 beta 4 (several scenarios).
Mozilla/5.0 (X11; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0 Verified as fixed on latest Aurora 26.0a2 (buildID: 20131002004002)
You need to log in before you can comment on or make changes to this bug.