307426 - ensureSelectedElementIsVisible does not handle the case of no selected element

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Rob Adams]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.7.10) Gecko/20050825 Firefox/1.0.6 (Ubuntu package 1.0.6)
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.7.10) Gecko/20050825 Firefox/1.0.6 (Ubuntu package 1.0.6)

there are situations in which the code calls ensureSelectedElementIsVisible when
no such element exists (such as from _refreshSelection) but this function does
not handle this case.

Trivial patch to be attached to simply make this function check before trying to
select a null list item.

Reproducible: Always

Comment 1

[Rob Adams]

Attachment: Add a check for a selected item in ensureSelectedItemIsVisible

Comment 2

[Doron Rosenberg (IBM)]

Are you sure the scrollbox doesn't already take that into consideration?

Comment 3

[Rob Adams]

Quite certain, since with current branch builds, my xulrunner app sees lots
XPCOM bad pointer complaints stemming from this.  It may make sense to fix it in
scrollbox rather than the XBL though; not sure of the desired semantics here.

In my experience, most XPCOM object hate it when you pass null to them :-)

Comment 4

[Doron Rosenberg (IBM)]

do we need a toolkit peer review?

Comment 5

[Rob Adams]

ping?

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Trunk:
mozilla/toolkit/content/widgets/richlistbox.xml; new revision: 1.23;

Comment 7

[Mike Schroepfer]

Comment on attachment 195190 [details] [diff] [review]
Add a check for a selected item in ensureSelectedItemIsVisible

Per bug meeting - approved for 1.8b5.

Comment 8

[Asaf Romano (gone)]

1.8 Branch:
Checking in richlistbox.xml;
/cvsroot/mozilla/toolkit/content/widgets/richlistbox.xml,v  <--  richlistbox.xml
new revision: 1.13.2.7; previous revision: 1.13.2.6
done

Comment 9

[Simon Bünzli]

(In reply to comment #2)
> Are you sure the scrollbox doesn't already take that into consideration?

Just FYI: the scrollbox in fact didn't take this into consideration, which is
why we got Bug 308098. The patch over there however got a little closer to where
the problem actually occurred (ensureElementIsVisible got also called with a
null parameter from otherwhere). This patch here won't hurt, though.