1480057 - MUC Participants list not in order caused by listbox removal

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Martin Giger [:freaktechnik]]

Participants that change or join after the room was joined by the client are moved to the bottom of the participant list instead of being inserted at the correct point.

Comment 2

[Jorg K (CEST = GMT+2)]

Any fix in sight?

Comment 4

[Martin Giger [:freaktechnik]]

This bug does not only affect IRC chat rooms, but all protocols.

Comment 5

[Magnus Melin [:mkmelin]]

What is the right order? Alphabetical?

Comment 6

[Martin Giger [:freaktechnik]]

Yes, to be precise char code order: https://searchfox.org/comm-central/source/mail/components/im/content/imconversation.xml#1133

I think the initially built list still does this properly, but users that join the MUC while TB is open just get added to the bottom of the list.

Comment 7

[Florian Quèze [:florian]]

Attachment: Tentative patch

I found	the problem using the debugger: .getAttribute("label") returns an empty	string on richlistitem nodes, because the 'label' is now a child node.
Possible fixes:
  .querySelector("label").value
or a shorthand using the "label" attribute getter of the richlistitem binding:
  .label

I'm using the latter in	the attached patch. The	shorthand doesn't work for setting the label though.

The patch is completely	untested as I don't have a local TB build right now.

Comment 8

[Jorg K (CEST = GMT+2)]

Thanks, Florian.

TypeError: aListItem.label is undefined - imconversation.xml:1132:14 :-( - Now showing 0 participants. But it can't be far off.

Comment 9

[Florian Quèze [:florian]]

Bah, of course, the list item isn't in the DOM yet when it's the paramater to the addNick method, so the XBL binding isn't attached yet.

.label should be .querySelector("label").value on that line.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: muc-sort-order (v2)

Comment 11

[Florian Quèze [:florian]]

Hi Jorg,

Could you please explain the change to .firstChild.nextSibling in your version of the patch? Is there an edge case where .querySelector("label") doesn't work? I'm afraid ".firstChild.nextSibling" is something that could easily get broken the next time someone does a change to the markup structure. (I just tested locally my version of the patch with the change from comment 9 included and couldn't spot any obvious bug.)

Comment 12

[Jorg K (CEST = GMT+2)]

Hi Florian :-)

I think it's a matter of taste. We construct the richlistitem and add image and label to it:
https://searchfox.org/comm-central/rev/52a3807508b46405267df7aa72050be66e40e52e/mail/components/im/content/imconversation.xml#1104

In bug 1475817, where we switched a lot of stuff to richtlistbox (and should have covered these three call sites as well), we usually added something with firstChild, see:
https://hg.mozilla.org/comm-central/rev/aa5d645f1d72#l1.15
https://hg.mozilla.org/comm-central/rev/a494924db574#l8.42
https://hg.mozilla.org/comm-central/rev/02051239a1bd#l3.32 - Chat
https://hg.mozilla.org/comm-central/rev/02051239a1bd#l3.35 - Chat
https://hg.mozilla.org/comm-central/rev/2501e7eabc9f#l1.13
(I could find more if I wanted).

Since we're currently on the de-XBL crusade, I'd personally avoid anything with XBL, like:
.querySelector("label").value
-----------------------^^^^^^

Also, querySelector appears a little "indeterministic" to me and maybe slower, it's a search. We know where the label is since we've put it there. Of course, changing that construction, we'd have to revisit these call sites. But we could add a comment:
// If the richtlistitem construction is changed, .firstChild.nextSibling needs to be adjusted.

Comment 13

[Jorg K (CEST = GMT+2)]

The important part is to settle for a solution so I can ship it in TB 65 beta 2 today. The mixed-up list is a serious draw-back.

Comment 14

[Magnus Melin [:mkmelin]]

I think the querySelector("label") version is clearer. 

You can use .value for custom elements too, but may be clearer to use .getAttribute("value")

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: muc-sort-order-florian.patch - Florian's version

f+ since it's also working. I don't know which version is preferable.

Comment 16

[Jorg K (CEST = GMT+2)]

Magnus, maybe you can submit the final patch. Even if you can avoid .value, you cannot avoid .label here since some magic getter gets the label through some DOM search:

-           if (nick < nicklist.getItemAtIndex(middle)
-                              .getAttribute("label").toLowerCase())
+           if (nick < nicklist.getItemAtIndex(middle).label.toLowerCase())

Comment 17

[Florian Quèze [:florian]]

(In reply to Jorg K (GMT+1) from comment #16)
> Magnus, maybe you can submit the final patch. Even if you can avoid .value,
> you cannot avoid .label here since some magic getter gets the label through
> some DOM search:
> 
> -           if (nick < nicklist.getItemAtIndex(middle)
> -                              .getAttribute("label").toLowerCase())
> +           if (nick < nicklist.getItemAtIndex(middle).label.toLowerCase())

The .label here is actually a XBL attribute implemented at https://searchfox.org/mozilla-central/rev/f8de61826903996f6bdf41b11a2844dd59ac144f/toolkit/content/widgets/richlistbox.xml#40-52

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 9034686 [details] [diff] [review]
muc-sort-order-florian.patch - Florian's version

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

I think we can just go with this one. r=mkmelin

Comment 19

[Jorg K (CEST = GMT+2)]

TB 65 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/714a4978437b8083c9aa8d15122aa660876f0d28
fix sorting of chat participants list by accessing label/name correctly after switch to richlistbox. r=mkmelin a=jorgk

Will land on truck soon. Thanks, Florian!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3df2a9ba3c39
fix sorting of chat participants list by accessing label/name correctly after switch to richlistbox. r=mkmelin

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Attachment: screenshot

It's weird, things mostly work for me now with 65.0b2, but I still get an unsorted list when I switch from a private conversation to a regular IRC room. Switching to another IRC room and back re-sorts the participant list. Any idea why that particular switch would have issues, Florian?

Comment 22

[Florian Quèze [:florian]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #21)

I still get an unsorted list when I switch from a private conversation to a regular IRC room.

When I do that I get this JS error in the console, and I have a single nick in the participant list:
TypeError: nicklist.getItemAtIndex(...).label is undefined

The error is on the if (nick < nicklist.getItemAtIndex(middle).label.toLowerCase()) line.
I guess the XBL binding isn't attached yet when we attempt to use the label getter.

At this point this should be fixed in a follow-up.