Closed Bug 954576 Opened 10 years ago Closed 10 years ago

Collapsed participant list sometimes loses listitems

Categories

(Instantbird Graveyard :: Conversation, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

Details

Attachments

(1 file, 1 obsolete file)

*** Original post on bio 1143 at 2011-11-03 23:57:00 UTC ***

This is another candidate for the issue originally reported in bug 954563 (bio 1130).

After a netsplit, 11 participants rejoined simultaneously while the participant list was collapsed. Only one participant (and, according to the log, not the first or last one to join) was added correctly to the participant list. 

The incorrectly added participants are in the listbox as listitem elements without any children. Therefore, the displayed participant count is correct.

Putting the conversation on hold and reopening fixes the problem as the nicklist is repopulated.
*** Original post on bio 1143 at 2011-11-04 17:30:35 UTC ***

Behaviour observed a second time, this time without a netsplit.

Some more details on the incorrect listitems:
- They appear in the correct position in the list.
- Their attributes are set correctly, including -moz-binding
- There is no XBL binding
- and therefore the CSS rules from listbox.css are not applied either (but these only set some border attributes)

This occurred simultaneously with the top few listitems not being displayed (despite having the correct binding) due to their Position and Dimensions being set to zero (probably bug 954563 (bio 1130)).
*** Original post on bio 1143 at 2012-02-06 20:15:49 UTC ***

Thought I would document these quotes for future reference.

Comment on a listbox issue for another bug ("listitem-iconic loose his icons", https://bugzilla.mozilla.org/show_bug.cgi?id=294029), a bit old:

>Known XBL issue, covered in another bug somewhere.
>Basically the XBL for the listitem only gets attached for the visible listitems,
>so setting .image sets a JS property on the DOM wrapper rather than invoking the
>XBL property. The workaround is to set the image attribute instead.

Elsewhere:

>listbox doesn't create frames for hidden rows, and thus no XBL gets attached 
>to them. XBL doesn't bind until frame construction time.  And listbox only
>constructs frames for the visible items.
*** Original post on bio 1143 at 2012-04-03 19:33:53 UTC ***

Suspected cause: https://bugzilla.mozilla.org/show_bug.cgi?id=307098
Attached patch WIP (obsolete) — Splinter Review
*** Original post on bio 1143 as attmnt 1359 at 2012-04-14 20:44:00 UTC ***

OK, some small progress.

It is hard to get the listitems to bind. Asking on #developers led to various suggestions (set a JS property, use GetBoundingClientRect, access a property which requires the binding to calculate the result (e.g. height)) - none of these worked for listitems.

The only thing so far which seems to work is using .ensureElementIsVisible() on the listbox. Because listitems are bound lazily when they are scrolled into view, and this enforces it. 

Unfortunately this does not work when the listbox is collapsed, as nothing is visible then and sadly the method is smart enough to realize.

This WIP does two things:

1) It adds an ensureElementIsVisible call when adding a participant. Therefore as long as the nicklist is visible all elements are bound. 

This leads to 
- noticeably slower scrolling. 
- a potentially seconds-long wait on opening a channel like #ubuntu during which the UI freezes.

BUT it seems to "fix" bug 954563 (bio 1130)! - on scrolling, listitems no longer "go missing" (instead of lazily having their binding attached, which is what should happen, but doesn't always). 

(This is why I suspect this bug and bug 954563 (bio 1130) are at root the same - listitems which should be properly initialized are not always, when they are only briefly visible or not at all.)

2) It calls ensureElementIsVisible for all listitems when opening a collapsed nicklist. Listitems which have been added in while the list was collapsed now seem to appear as they should.
Again a seconds-long delay is possible.

Of course this WIP is unacceptable as a patch. The question is, can one learn anything useable from the result?

Question:
Whether a listitem is bound or not is visible in the DOM Inspector - how does it do this? Is there an easy way to tell whether an element's binding has been attached or not? This would allow the loop in 2) to be restricted to a small number of elements, which would be OK I think.

Any ideas?
Attachment #8353112 - Flags: feedback?
Comment on attachment 8353112 [details] [diff] [review]
WIP

*** Original change on bio 1143 attmnt 1359 at 2012-04-14 20:45:06 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353112 - Flags: feedback? → feedback?(florian)
*** Original post on bio 1143 at 2012-04-16 10:41:37 UTC ***

(In reply to comment #4)
> Created attachment 8353112 [details] [diff] [review] (bio-attmnt 1359) [details]

> Whether a listitem is bound or not is visible in the DOM Inspector - how does
> it do this? Is there an easy way to tell whether an element's binding has been
> attached or not?

I don't know. Seems like a question for #developers ;).

> This would allow the loop in 2) to be restricted to a small
> number of elements, which would be OK I think.

Would it be possible to restrict to only the items that should be visible? (the item at getIndexOfFirstVisibleRow, + the (listbox height/item height) next items)
*** Original post on bio 1143 at 2012-04-16 11:26:48 UTC ***

(In reply to comment #5)
> I don't know. Seems like a question for #developers ;).
> 
> > This would allow the loop in 2) to be restricted to a small
> > number of elements, which would be OK I think.
> 
> Would it be possible to restrict to only the items that should be visible? (the
> item at getIndexOfFirstVisibleRow, + the (listbox height/item height) next
> items)

This wouldn't really solve the problem, as you want to restrict to all the new listitems that have been added since the nicklist was collapsed (which get incorrectly initialized for some reason, so they get lost when the nicklist opens). What one could do is keep track of the added participants "by hand".

For the nicklist as a whole (bug 954563 (bio 1130) or part 1) here), the only thing I can think of is asynchronous population of the nicklist, so the UI doesn't freeze on opening a chat.
Comment on attachment 8353112 [details] [diff] [review]
WIP

*** Original change on bio 1143 attmnt 1359 at 2012-04-25 22:24:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353112 - Flags: feedback?(florian)
*** Original post on bio 1143 at 2012-06-28 00:11:38 UTC ***

So, I decided to take another look at this, and thankfully (since the workarounds noted above are terrible performance-wise and otherwise) happily since Gecko 13 it is not reproducible for me.

DOM Inspector suggests listitems are now bound correctly when they are bound dynamically, even when inserted while the listbox is collapsed. (See also bug 954563 (bio 1130)) 

I couldn't find the change in the code which fixed this, so while I am resolving this bug, it may need to be reopened as I don't know why it now works.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WORKSFORME
*** Original post on bio 1143 at 2012-07-02 21:23:59 UTC ***

Sadly, it happened again.

Key giveaway: Missing elements (in this case including my own nick), and on scrolling with the keyboard the selection disappears when on a "missing" item. List does not recover on scrolling with the mouse. 

Also some odd "jumping" behaviour as of course the scrollbar calculation is off due to the items that are not displayed, but expected.

Some, but not all, the missing elements lack their binding. The DOM Node carries the correct label etc. On the items that are missing but bound, the listcell-label carries the correct JS .value. However, .scrollHeight and .scrollWidth are zero (unlike for properly displayed elements). Ditto clientHeight/Width and the box model coordinates and dimension.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Summary: New participants not correctly added to collapsed participant list → Collapsed participant list sometimes loses listitems
Attached patch PatchSplinter Review
*** Original post on bio 1143 as attmnt 2164 at 2012-12-12 15:50:00 UTC ***

This patch provides what I hope is an acceptable workaround for this annoying bug. Note it will not cause flickering as the event handler runs before the nicklist is uncollapsed.

After adding some reliable instrumentation, it turns out there were really two bugs, both easily reproduced with busy channels.

- listbox.getIndexOfFirstVisibleRow() is unreliable after rapid scrolling with the scrollbar, i.e. it is possible the actual first visible listitem does not match what the listbox's internal state believes the first visible listitem to be. Usually this is only noticeable when (after this happens) scrolling the list with the keyboard, as the selection "goes off the screen" without the listbox scrolling. (I don't think this mozilla bug has been filed, probably as it only occurs with hundreds of listitems.)

- Listitems added to the collapsed listbox are not always bound when the nicklist is reopened, if they were assumed to be visible based on their position in the nicklist. They then remain missing from the nicklist (even after some scrolling), as the DOM believes they were already initialized. (This is part of the BMO bugs linked above, which are to be fixed in XBL2, etc.)

The second bug is the one we care about (the first only affects where it occurs).

To ensure the listitems get bound, we hide and then redisplay the listbox, which resets its state. The alternative workaround would be to search for the unbound listitems and enforce their binding - this is possible, but more cumbersome and probably slower as it requires removing and readding the affected listitems (in multiple passes if there is a block of more than one such items).

Some notes for future reference and diagnosis, in case something similar crops up again:

- To find out whether a listitem is bound properly, one can use !!item.ownerDocument.getAnonymousNodes(item)

- A visible listitem will have clientHeight and scrollHeight nonzero.

- listbox.ensureIndexIsVisible() only enforces a listitem is bound if the DOM doesn't believe it to be already bound, and is no help for this bug. (I'm not sure how "believes it to be bound" is represented internally.)
Attachment #8353926 - Flags: review?(florian)
Comment on attachment 8353112 [details] [diff] [review]
WIP

*** Original change on bio 1143 attmnt 1359 at 2012-12-12 15:50:06 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353112 - Attachment is obsolete: true
Assignee: nobody → aleth
Status: REOPENED → ASSIGNED
*** Original post on bio 1143 at 2012-12-12 18:55:22 UTC ***

This mozilla bug https://bugzilla.mozilla.org/show_bug.cgi?id=307098 suggests it would be a bad idea to set "display:none" on the listbox as soon as it is collapsed via CSS. (I also dislike it because we lose the scroll position that way)
Comment on attachment 8353926 [details] [diff] [review]
Patch

*** Original change on bio 1143 attmnt 2164 at 2012-12-14 23:09:17 UTC ***

This is ugly but self-contained, and that bug seems to really annoy you, so I'm willing to give this a try on nightlies.

Note that this adds a DOMAttrModified event listener for each conversation, so we will have more noise in the error console, as that's deprecated...
Attachment #8353926 - Flags: review?(florian) → review+
*** Original post on bio 1143 at 2012-12-14 23:31:13 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/8413304a42d2

I'd like to clean up those DOMAttrModified warnings then.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4
You need to log in before you can comment on or make changes to this bug.