Last Comment Bug 740765 - Starting or selecting a conversation with the mouse should focus the input box immediately
: Starting or selecting a conversation with the mouse should focus the input bo...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 15.0
Assigned To: Florian Quèze [:florian] [:flo]
:
Mentors:
: 742637 (view as bug list)
Depends on: 756136
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-30 04:04 PDT by Florian Quèze [:florian] [:flo]
Modified: 2012-05-17 09:54 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
Patch (6.18 KB, patch)
2012-04-27 09:33 PDT, Florian Quèze [:florian] [:flo]
bwinton: review+
bwinton: ui‑review+
Details | Diff | Splinter Review
Patch v2 (6.76 KB, patch)
2012-05-01 07:46 PDT, Florian Quèze [:florian] [:flo]
florian: review+
florian: ui‑review+
mozilla: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description Florian Quèze [:florian] [:flo] 2012-03-30 04:04:29 PDT
- When a conversation is selected with a mouse interaction, it should be immediately possible to type in that conversation (the input box should be focused).
- Also, starting a conversation with the mouse (for example by clicking on the "Start conversation" bubble) should select that conversation automatically (currently it's done only if the the contact was selected at the time the conversation was started).
Comment 1 Kami 2012-04-05 04:48:06 PDT
*** Bug 742637 has been marked as a duplicate of this bug. ***
Comment 2 Florian Quèze [:florian] [:flo] 2012-04-27 09:33:18 PDT
Created attachment 619075 [details] [diff] [review]
Patch

Behavior implemented by this patch:
- When the user starts a conversation (either through the keyboard by pressing enter on a selected contact, or through the mouse by double clicking a contact or single clicking the chat bubble of a hovered contact), its textbox is focused immediately.
- Selecting a conversation with the mouse gives focus to the textbox immediately.
- Selecting a conversation through the keyboard (using the up/down arrow keys while the listbox of the left pane is focused) doesn't change the focus; it's still on the listbox.
- When the focus is on the listbox and a conversation is selected:
-- pressing the enter or return keys gives the focus to the input box of the conversation.
-- typing a character focuses the input box of the conversation AND resends the keyboard event so that the input box receives is.
Comment 3 Blake Winton (:bwinton) (:☕️) 2012-04-30 10:46:23 PDT
Comment on attachment 619075 [details] [diff] [review]
Patch

So, I like the "typing will auto-focus the content box", but I found it didn't handle ctrl-v, which seemed a little annoying.  I also wonder about people mis-sending messages to the wrong contact.

Still, the rest of this patch seems like an improvement, so ui-r=me with the paste problem mentioned above fixed.

Now, on to the code-review.  :)


>+++ b/mail/components/im/content/chat-messenger-overlay.js
>@@ -330,16 +339,23 @@ var chatHandler = {
>+  onListItemClick: function(aEvent) {
>+    if (aEvent.button != 0 || aEvent.detail != 1)
>+      return;

It would be nice to have a comment here explaining which button you intend to handle.

>+++ b/mail/components/im/content/imcontact.xml
>@@ -198,17 +198,19 @@
>      <method name="openConversation">
>       <body>
>        <![CDATA[
>-         this.contact.createConversation();
>+         let prplConv = this.contact.createConversation();
>+         let uiConv = Services.conversations.getUIConversation(prplConv);
>+         chatHandler.focusConversation(prplConv);

You don't seem to use uiConv anywhere in this function...

Other than that I think I like it.  r=me, too.

Thanks,
Blake.
Comment 4 Florian Quèze [:florian] [:flo] 2012-05-01 07:46:39 PDT
Created attachment 619925 [details] [diff] [review]
Patch v2

Updated patch taking into account Blake's comments. Carrying forward the r and ui-r flags.

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #3)

> >+++ b/mail/components/im/content/imcontact.xml
> >@@ -198,17 +198,19 @@
> >      <method name="openConversation">
> >       <body>
> >        <![CDATA[
> >-         this.contact.createConversation();
> >+         let prplConv = this.contact.createConversation();
> >+         let uiConv = Services.conversations.getUIConversation(prplConv);
> >+         chatHandler.focusConversation(prplConv);
> 
> You don't seem to use uiConv anywhere in this function...

Very good catch. chatHandler.focusConversation expects the uiConversation, not the prplConversation. This mistake wasn't visible during my testing because the prpl and ui conversation ids currently match, but they won't always match in the future.
Comment 5 Florian Quèze [:florian] [:flo] 2012-05-02 03:29:40 PDT
http://hg.mozilla.org/comm-central/rev/9d1568bb5b2a
Comment 6 Mark Banner (:standard8) 2012-05-09 08:59:04 PDT
I landed this on aurora for the closed tree, so that I could check for issues with the new automation:

https://hg.mozilla.org/releases/comm-aurora/rev/363737497ed6

Note You need to log in before you can comment on or make changes to this bug.