Closed Bug 874566 Opened 6 years ago Closed 6 years ago

Chat boxes should be opened in the currently active window (Linux/Mac)

Categories

(Firefox Graveyard :: SocialAPI, defect)

23 Branch
x86_64
Linux
defect
Not set

Tracking

(firefox25 verified, firefox26 verified)

VERIFIED FIXED
Firefox 26
Tracking Status
firefox25 --- verified
firefox26 --- verified

People

(Reporter: marco, Assigned: florian)

References

Details

Attachments

(2 files, 1 obsolete file)

See also bug 835111.
See Also: → 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
Blocks: Talkilla
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...
Depends on: 462222
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 gavin@gavinsharp.com 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
Attached patch Patch (obsolete) — Splinter Review
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+
Attached patch Patch v2Splinter Review
Added test coverage. Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ca572a89b2b9
Attachment #795522 - Attachment is obsolete: true
Attachment #796266 - Flags: review?(mhammond)
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+
https://hg.mozilla.org/mozilla-central/rev/28d046b9513e
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
Attached patch Patch for auroraSplinter Review
(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).
QA Contact: petruta.rasa
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)
Status: RESOLVED → VERIFIED
Keywords: verifyme
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.