Closed Bug 1480057 Opened 2 years ago Closed 2 years ago
MUC Participants list not in order caused by listbox removal
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.
Any fix in sight?
Summary: Participants list not in order → IRC Participants list not in order caused by listbox removal
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.
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.
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.)
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.
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+
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!
Pushed by firstname.lastname@example.org: 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: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.