763872 - The listbox selectedItem property doesn't work correctly before all listitem elements have their XBL binding attached

Status: NEW

(Toolkit :: UI Widgets, defect)

Description

[Florian Quèze [:florian]]

Attachment: Patch

I saw several situations where setting selectedItem on a listbox early (before all listitems have had their XBL binding attached) breaks the selected item of the list (that item won't receive any styling when selected, even if another item is selected and the item is then selected again).

It turns out the cause is the listitem binding setting the "selected" property on the listitem, and then when the listitem binding is finally attached, its "selected" getter and setter are never called, so the "selected" DOM attribute is never set.

Comment 1

[aleth [:aleth]]

If this patch also affects the listitem-iconic class, there may be further properties to be copied (e.g. "image").

Comment 2

[Florian Quèze [:florian]]

The only other property that could be affected is "checked" in the listitem-checkbox binding, but it's set only from a keypress event handler of the listbox binding, so I don't think this can happen before the listitem bindings are attached.

Comment 3

[Neil Deakin]

> I saw several situations...

Which situations? Is this just bug 250123?

Comment 4

[Florian Quèze [:florian]]

(In reply to Neil Deakin from comment #3)
> > I saw several situations...
> 
> Which situations?

I have 2 situations in mind:
- The log viewer in Instantbird, where the selectedItem is set before the window is actually visible. An ugly work around with a timer was added.
- The list of previous conversations of the right pane of the Chat tab of the Thunderbird UI, typically when the Chat tab isn't selected at the time the list of previous conversations is displayed.

In both of these cases I guess the XBL bindings aren't attached immediately when the listbox is populated.

> Is this just bug 250123?

No. It's definitely related, but only bug 250123 comment 15 describes the same problem.
As I understand it, bug 250123 is mostly about things not working for listitems that have never been visible (and don't have the XBL binding attached at all). The situation here is that after the XBL binding is attached, the listitem remains broken (and visibly so) because some JS properties set before the binding was attached override some getters/setters of the binding.
The patch I attached here seems much better to me than the work arounds based on timers or ensureElementIsVisible calls.

Comment 5

[Neil Deakin]

Does it work if you return true right away from nsListBoxBodyFrame::ContinueReflow? (skipping all of the code in that function). Or, with a screen reader active?

Comment 6

[Florian Quèze [:florian]]

(In reply to Neil Deakin from comment #5)
> Does it work if you return true right away from
> nsListBoxBodyFrame::ContinueReflow? (skipping all of the code in that
> function). Or, with a screen reader active?

I don't know, my build is with --disable-accessibility because it didn't build on OS X 10.5.

Comment 7

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)

> I have 2 situations in mind:
> - The log viewer in Instantbird, where the selectedItem is set before the
> window is actually visible. An ugly work around with a timer was added.

This workaround was added in bug 954683. It would be nice to remove it eventually.

Comment 8

[Neil Deakin]

Comment on attachment 632191 [details] [diff] [review]
Patch

Clear old reviews.

Comment 9

[Axel Grude]

I have just been hit with this bug in my Thunderbird Add-on quickFilters - in the latest version Thunderbird 60.0 releease. I have a filter assistant window where you select a filter template. I have an event on the selection and selectedItem is completely missing.

This is a xul dialog with preference bindings. Have selectedItem bindings been removed in m-c recently?

Comment 10

[Jorg K (CEST = GMT+2)]

listbox has been removed at mozilla63, so there's little hope for this to be fixed :-(

Comment 11

[Axel Grude]

I will replace with richlistbox, seems a pretty trivial fix overall.