Last Comment Bug 735331 - "Rename" in the context menu of a contact doesn't work
: "Rename" in the context menu of a contact doesn't work
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 17.0
Assigned To: Mike Conley (:mconley) - (Needinfo me!)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-13 11:05 PDT by Florian Quèze [:florian] [:flo] (PTO until August 29th)
Modified: 2012-07-27 10:32 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
Patch v1 (617 bytes, patch)
2012-07-19 08:51 PDT, Mike Conley (:mconley) - (Needinfo me!)
florian: review-
Details | Diff | Splinter Review
Patch v2 (1.76 KB, patch)
2012-07-19 10:37 PDT, Mike Conley (:mconley) - (Needinfo me!)
florian: review-
Details | Diff | Splinter Review
Patch v3 (1.79 KB, patch)
2012-07-20 13:32 PDT, Mike Conley (:mconley) - (Needinfo me!)
florian: review-
Details | Diff | Splinter Review
Patch v4 (2.93 KB, patch)
2012-07-23 10:29 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Patch v5 (1.99 KB, patch)
2012-07-23 11:24 PDT, Mike Conley (:mconley) - (Needinfo me!)
florian: review+
bwinton: approval‑comm‑aurora+
bwinton: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Florian Quèze [:florian] [:flo] (PTO until August 29th) 2012-03-13 11:05:37 PDT
Most of the code is here (copied from Instantbird), but the adaptation for Thunderbird isn't finished.
Comment 1 Mike Conley (:mconley) - (Needinfo me!) 2012-07-19 08:51:30 PDT
Created attachment 643866 [details] [diff] [review]
Patch v1

So it looks like InstantBird does this by switching the label binding for a contact to the textbox binding.

Does that look / feel alright to you, Blake?
Comment 2 Florian Quèze [:florian] [:flo] (PTO until August 29th) 2012-07-19 09:58:01 PDT
Comment on attachment 643866 [details] [diff] [review]
Patch v1

I see this error in the context menu when changing the name causes the position of the contact to change:
Timestamp: 19/07/12 18:51:56
Error: TypeError: this.parentNode is null
Source File: chrome://messenger/content/chat/imcontact.xml
Line: 147

This is because updateContactPosition (from imgroup.xml) will remove the contact and add it again, so the code in finishAliasing (imcontact.xml) is executed on a node that's no longer in the DOM.

I think 2 lines calling removing and deleting the event listener could be moved to before the new alias is saved.

I'm not sure about the focus call.
Comment 3 Mike Conley (:mconley) - (Needinfo me!) 2012-07-19 10:37:26 PDT
Created attachment 643916 [details] [diff] [review]
Patch v2

Ok, take two. Seems to still work as advertised, and the Error Console is clear.
Comment 4 Mike Conley (:mconley) - (Needinfo me!) 2012-07-19 10:38:26 PDT
Comment on attachment 643916 [details] [diff] [review]
Patch v2

How do we feel about the contact repositioning itself after a rename?
Comment 5 Florian Quèze [:florian] [:flo] (PTO until August 29th) 2012-07-20 04:59:34 PDT
Comment on attachment 643916 [details] [diff] [review]
Patch v2

>diff --git a/mail/components/im/content/imcontact.xml b/mail/components/im/content/imcontact.xml

>+        if (aSave) {
>+          let displayName = document.getAnonymousElementByAttribute(this, "anonid",
>+                                                                    "displayname");
>+          if (displayName)
>+            this.contact.alias = displayName.value;

Why have you added this |if (displayName)| test? It prevents removing the alias (which falls back to using what the contact has specified himself as his display name).
Comment 6 Mike Conley (:mconley) - (Needinfo me!) 2012-07-20 06:34:42 PDT
(In reply to Florian Quèze from comment #5)
> Comment on attachment 643916 [details] [diff] [review]
> Patch v2
> 
> >diff --git a/mail/components/im/content/imcontact.xml b/mail/components/im/content/imcontact.xml
> 
> >+        if (aSave) {
> >+          let displayName = document.getAnonymousElementByAttribute(this, "anonid",
> >+                                                                    "displayname");
> >+          if (displayName)
> >+            this.contact.alias = displayName.value;
> 
> Why have you added this |if (displayName)| test? It prevents removing the
> alias (which falls back to using what the contact has specified himself as
> his display name).

I added it because I was getting some error messages in my console about the displayname node not existing.

In the case where the user removes the alias, shouldn't displayname still exist, but displayname.value be empty?
Comment 7 Florian Quèze [:florian] [:flo] (PTO until August 29th) 2012-07-20 07:00:59 PDT
(In reply to Mike Conley (:mconley) from comment #6)

> > Why have you added this |if (displayName)| test? It prevents removing the
> > alias (which falls back to using what the contact has specified himself as
> > his display name).
> 
> I added it because I was getting some error messages in my console about the
> displayname node not existing.

Hmm... Any idea of why it didn't exist?

> In the case where the user removes the alias, shouldn't displayname still
> exist, but displayname.value be empty?

It should, I misread the code, sorry.
Comment 8 Mike Conley (:mconley) - (Needinfo me!) 2012-07-20 08:06:24 PDT
(In reply to Florian Quèze from comment #7)
> (In reply to Mike Conley (:mconley) from comment #6)
> 
> > > Why have you added this |if (displayName)| test? It prevents removing the
> > > alias (which falls back to using what the contact has specified himself as
> > > his display name).
> > 
> > I added it because I was getting some error messages in my console about the
> > displayname node not existing.
> 
> Hmm... Any idea of why it didn't exist?

No, I couldn't determine why this was happening (though I admittedly didn't dig too far into it), but I thought I'd put in a guard for that case.

If you think it's worth further investigation, I'll dig further. Let me know.
Comment 9 Florian Quèze [:florian] [:flo] (PTO until August 29th) 2012-07-20 09:53:20 PDT
Comment on attachment 643916 [details] [diff] [review]
Patch v2

(In reply to Mike Conley (:mconley) from comment #8)
> (In reply to Florian Quèze from comment #7)
> > (In reply to Mike Conley (:mconley) from comment #6)
> > 
> > > > Why have you added this |if (displayName)| test? It prevents removing the
> > > > alias (which falls back to using what the contact has specified himself as
> > > > his display name).
> > > 
> > > I added it because I was getting some error messages in my console about the
> > > displayname node not existing.
> > 
> > Hmm... Any idea of why it didn't exist?
> 
> No, I couldn't determine why this was happening (though I admittedly didn't
> dig too far into it), but I thought I'd put in a guard for that case.
> 
> If you think it's worth further investigation, I'll dig further. Let me know.

It happens because the focus call in finishAliasing causes the binding to receive a blur event before the "aliasing" attribute has been removed, which causes another call to finishAliasing.
Comment 10 Mike Conley (:mconley) - (Needinfo me!) 2012-07-20 13:32:57 PDT
Created attachment 644446 [details] [diff] [review]
Patch v3

Simply moving the removeAttribute call before the focus call did not cut it - it just seemed to break the rename functionality (after pressing enter, the name did not change).

Do we even need the call to focus? If I excise it, everything seems to work as expected.

Anyhow - any pointers here would be appreciated.
Comment 11 Florian Quèze [:florian] [:flo] (PTO until August 29th) 2012-07-20 15:02:09 PDT
If you remove the "aliasing" attribute, then the textbox binding is detached as your CSS selector no longer matches, so it's expected that things no longer work.

I think the focus call is needed because while you are editing the focus is on the textbox, and when you leave the editing mode, the listitem isn't focusable, so the focus isn't anywhere; putting it to the richlistbox (where the selected item will then appear highlighted) seems the right thing to do.

I think we are back to my first suggestion (over IRC I guess, as I don't see it in the comments here) of keeping the value of this.parentNode in a variable before saving the new alias, and then keeping the focus call where it is, but using the variable instead of this.parentNode that may no longer exist if the DOM node has been detached.
Comment 12 Florian Quèze [:florian] [:flo] (PTO until August 29th) 2012-07-23 04:21:44 PDT
Comment on attachment 644446 [details] [diff] [review]
Patch v3

r- as I'm expecting an updated patch taking into account comment 11.
Comment 13 Mike Conley (:mconley) - (Needinfo me!) 2012-07-23 10:29:56 PDT
Created attachment 644973 [details] [diff] [review]
Patch v4

Ok, as suggested, going with the parentNode cache.
Comment 14 Mike Conley (:mconley) - (Needinfo me!) 2012-07-23 11:24:26 PDT
Created attachment 644997 [details] [diff] [review]
Patch v5

Better, simpler, more correct.
Comment 15 Blake Winton (:bwinton) (:☕️) 2012-07-25 11:58:09 PDT
Comment on attachment 644997 [details] [diff] [review]
Patch v5

This seems like something we want for the initial IM release.

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