"Rename" in the context menu of a contact doesn't work

RESOLVED FIXED in Thunderbird 17.0

Status

Thunderbird
Instant Messaging
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: florian, Assigned: mconley)

Tracking

Trunk
Thunderbird 17.0

Thunderbird Tracking Flags

(thunderbird15 fixed, thunderbird16 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Most of the code is here (copied from Instantbird), but the adaptation for Thunderbird isn't finished.
Assignee: nobody → mconley
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?
Attachment #643866 - Flags: ui-review?(bwinton)
Attachment #643866 - Attachment is patch: true
Attachment #643866 - Flags: review?(florian)
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.
Attachment #643866 - Flags: review?(florian) → review-
Attachment #643866 - Flags: ui-review?(bwinton)
Created attachment 643916 [details] [diff] [review]
Patch v2

Ok, take two. Seems to still work as advertised, and the Error Console is clear.
Attachment #643866 - Attachment is obsolete: true
Attachment #643916 - Flags: review?(florian)
Comment on attachment 643916 [details] [diff] [review]
Patch v2

How do we feel about the contact repositioning itself after a rename?
Attachment #643916 - Flags: ui-review?(bwinton)
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).
(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?
(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.
(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 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.
Attachment #643916 - Flags: review?(florian) → review-
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.
Attachment #643916 - Attachment is obsolete: true
Attachment #643916 - Flags: ui-review?(bwinton)
Attachment #644446 - Flags: review?(florian)
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 on attachment 644446 [details] [diff] [review]
Patch v3

r- as I'm expecting an updated patch taking into account comment 11.
Attachment #644446 - Attachment description: Patch v3 (carrying over r+ from bienvenu) → Patch v3
Attachment #644446 - Flags: review?(florian) → review-
Created attachment 644973 [details] [diff] [review]
Patch v4

Ok, as suggested, going with the parentNode cache.
Attachment #644446 - Attachment is obsolete: true
Attachment #644973 - Flags: review?(florian)
Created attachment 644997 [details] [diff] [review]
Patch v5

Better, simpler, more correct.
Attachment #644973 - Attachment is obsolete: true
Attachment #644973 - Flags: review?(florian)
Attachment #644997 - Flags: review?(florian)
Attachment #644997 - Flags: review?(florian) → review+
Attachment #644997 - Flags: approval-comm-beta?
Attachment #644997 - Flags: approval-comm-aurora?
Comment on attachment 644997 [details] [diff] [review]
Patch v5

This seems like something we want for the initial IM release.
Attachment #644997 - Flags: approval-comm-beta?
Attachment #644997 - Flags: approval-comm-beta+
Attachment #644997 - Flags: approval-comm-aurora?
Attachment #644997 - Flags: approval-comm-aurora+
comm-central: https://hg.mozilla.org/comm-central/rev/9f34572f9f55
comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/3b338f186b45
comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/9414c17b9b7d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-thunderbird15: --- → fixed
status-thunderbird16: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 17.0
You need to log in before you can comment on or make changes to this bug.