804591 - message window remains minimized when I click on a name

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Andrew McCreight (out of office until 8/21) [:mccr8]]

If I don't have a chat window open for somebody, then when I click on the message dropdown for that person, their chat window appears, but if it was minimized when I last closed it, it appears minimized.

With the chat app on Facebook's actual webpage, if you do that, the chat window will not appear minimized, which I think is better, because you obviously want to interact with it if you just caused it to be created.

In my particular case, I was on a page with a gray background, so I didn't even notice at first that the tiny minimized bar appeared.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Does this only happen when the sidebar is hidden? I vaguely recall differences in behavior between sidebar- vs. worker-opened chat windows, but I forget whether that's a limitation we enforce.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

I can't reproduce this.  If the sidebar is hidden, the chat window will indeed open minimized - but whether it is initially opened normally or minimized, clicking the titlebar of the chat reliably toggles the minimized state for me.

Andrew - can you still reproduce this?

Comment 3

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Yes, clicking on the title bar of the chat itself toggles the minimized state, my comment was about how it initially appears after you click on a particular person in the chat menu thing that drops down from the icon in the upper right. I posit that it should appear non-minimized any time you click on a conversation.

Sorry, I missed your comment before, Gavin.

sidebar opened, chat already open, minimized: chat does not unminimize
sidebar opened, chat not already open: chat appeared unminimized as expected
sidebar opened, chat already open, not minimized: stays unminized as expected
sidebar closed, chat already open minimized, or not open: chat appears minimized

Comment 4

[Mark Hammond [:markh] [:mhammond]]

When you say "I posit that it should appear non-minimized any time you click on a conversation.", do you mean "...  click on a *contact in the sidebar*" or "... click on a *chat window*"?

I think the former - in which case I'm inclined to agree and isn't what actually happens.  If the latter, then I can't repro and/or still don't understand :)

Comment 5

[Andrew McCreight (out of office until 8/21) [:mccr8]]

The latter works fine for me.

I mean click on a contact/conversation in the "Messages" menu that drops down when you click on the little blue messages icon in the upper right. Sorry, I guess I am using the wrong jargon here.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Attachment: If a request is made to open a minimized chat, restore the chat.

This should fix most cases of this (although it will depend slightly on the provider behaviour)

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 676837 [details] [diff] [review]
If a request is made to open a minimized chat, restore the chat.

Review of attachment 676837 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that minor part of the test changed.

::: browser/base/content/test/browser_social_chatwindow.js
@@ +69,5 @@
> +          port.postMessage({topic: "test-chatbox-open"});
> +          break;
> +        case "chatbox-opened":
> +          is(e.data.result, "ok", "the sidebar says it got a chatbox");
> +          if (seen_opened) {

Handle the first case first, so make this:
if (!seen_opened) {
  ...
} else {
  ...
}

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Attachment: updated to fix (and test) regression from bug 798083

Sadly bug 798083 not only caused a conflict with this patch, it also introduced a regression.  Specifically, if a worker tried to open the same chat twice (and the worker always gets a minimized chat), the chatbox would have magically become unminimized - it should have stayed minimized in that case.

This patch is very similar to the old one but also fixes that regression and adds a new test specifically for that case.

Even though the patch is similar, it isn't similar enough that I feel comfortable carrying the r= forward - sorry jaws :)

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 679009 [details] [diff] [review]
updated to fix (and test) regression from bug 798083

Review of attachment 679009 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/socialchat.xml
@@ +364,5 @@
>            let cb = this.chatboxForURL.get(aURL);
>            if (cb) {
>              cb = cb.get();
>              if (cb.parentNode) {
> +              this.showChat(cb, aMode != "minimized");

I would rather that this variable get passed in as a string to showChat, and then put the comparison within showChat so it is easier to follow the code path here.

This would be particularly helpful in the oncommand attribute of the nubMenu since the extra boolean 'true' doesn't really describe to the reader what impact that is likely to have.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 679009 [details] [diff] [review]
updated to fix (and test) regression from bug 798083

I'll grant review now with the expectation that you'll make that change to the API before landing.

Comment 11

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22e38380a161

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Attachment: As landed

Comment 13

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5a2f571e1f6d for the weirdest set of later tests timing out.

Comment 14

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/216ad89546ab

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/216ad89546ab

Comment 16

[Alex Keybl [:akeybl]]

Is this Social API fix still desired on FF18? If so, please nominate asap.

Comment 17

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 681344 [details] [diff] [review]
As landed

This one is user-facing and still applies, so nominating.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: The social chat-boxes will not be in a consistent or user-expected state.
Testing completed (on m-c, etc.): Landed on 19 some time ago, includes tests
Risk to taking this patch (and alternatives if risky): Low risk, limited to social chat
String or UUID changes made by this patch: None

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/43f9c25bf7a7

Comment 19

[u279076]

Marking verified fixed since this landed with tests. Please add verifyme keyword if there's something you need QA to test.

Comment 20

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Yeah, this has been working for me recently. There's some slight weirdness with getting a scroll bar in the text input box, but that's a different issue. I'll file a new bug if I figure out how to reproduce it or whatever.

Comment 21

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Oh, I was thinking of another bug. I'll try this out at some point and file a new bug if I am still seeing issues of some sort...