MUC Participants list not in order caused by listbox removal

RESOLVED FIXED in Thunderbird 66.0

Status

defect
RESOLVED FIXED
Last year
5 months ago

People

(Reporter: freaktechnik, Assigned: florian)

Tracking

({regression})

Trunk
Thunderbird 66.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird65 fixed, thunderbird66 fixed)

Details

(Whiteboard: [regression:TB63])

Attachments

(2 attachments, 2 obsolete attachments)

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.
Duplicate of this bug: 1491710
Any fix in sight?
Duplicate of this bug: 1499374
Keywords: regression
Summary: Participants list not in order → IRC Participants list not in order caused by listbox removal
Whiteboard: [regression:TB63]
This bug does not only affect IRC chat rooms, but all protocols.
Summary: IRC Participants list not in order caused by listbox removal → MUC Participants list not in order caused by listbox removal
What is the right order? Alphabetical?
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.
Posted patch Tentative patch (obsolete) — Splinter Review
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.
Thanks, Florian.

TypeError: aListItem.label is undefined - imconversation.xml:1132:14 :-( - Now showing 0 participants. But it can't be far off.
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.
Posted patch muc-sort-order (v2) (obsolete) — Splinter Review
Assignee: nobody → jorgk
Attachment #9034617 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #9034619 - Flags: review?(mkmelin+mozilla)
Attachment #9034619 - Flags: feedback?(florian)
Attachment #9034619 - Attachment is patch: true
Attachment #9034619 - Attachment mime type: application/octet-stream → text/plain
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.)
Flags: needinfo?(jorgk)
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.
Flags: needinfo?(jorgk)
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.
I think the querySelector("label") version is clearer. 

You can use .value for custom elements too, but may be clearer to use .getAttribute("value")
f+ since it's also working. I don't know which version is preferable.
Attachment #9034686 - Flags: feedback+
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())
(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 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
Attachment #9034686 - Flags: review+
Attachment #9034619 - Attachment is obsolete: true
Attachment #9034619 - Flags: review?(mkmelin+mozilla)
Attachment #9034619 - Flags: feedback?(florian)
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!
Assignee: jorgk → florian
Target Milestone: --- → Thunderbird 66.0
Pushed by mozilla@jorgk.com:
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
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Posted image 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?

Flags: needinfo?(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.

Flags: needinfo?(florian)
Blocks: 1532904
You need to log in before you can comment on or make changes to this bug.