MUC Participants list not in order caused by listbox removal

RESOLVED FIXED in Thunderbird 66.0

Status

defect
RESOLVED FIXED
10 months ago
3 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)

(Reporter)

Description

10 months ago
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.

Updated

8 months ago
Duplicate of this bug: 1491710

Comment 2

8 months ago
Any fix in sight?

Updated

7 months ago
Duplicate of this bug: 1499374

Updated

7 months ago
Keywords: regression
Summary: Participants list not in order → IRC Participants list not in order caused by listbox removal
Whiteboard: [regression:TB63]
(Reporter)

Comment 4

7 months ago
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?
(Reporter)

Comment 6

4 months ago
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.
(Assignee)

Comment 7

4 months ago
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.

Comment 8

4 months ago
Thanks, Florian.

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

Comment 9

4 months ago
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

4 months ago
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)

Updated

4 months ago
Attachment #9034619 - Attachment is patch: true
Attachment #9034619 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 11

4 months ago
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)

Comment 12

4 months ago
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)

Comment 13

4 months ago
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")

Comment 15

4 months ago
f+ since it's also working. I don't know which version is preferable.
Attachment #9034686 - Flags: feedback+

Comment 16

4 months ago
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())
(Assignee)

Comment 17

4 months ago
(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)

Comment 19

4 months ago
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

Comment 20

4 months ago
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
Last Resolved: 4 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)
(Assignee)

Comment 22

4 months ago

(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)

Updated

3 months ago
Blocks: 1532904
You need to log in before you can comment on or make changes to this bug.