97421 - Default outliner xbl key event handler causes invalid selection

Status: RESOLVED WONTFIX

(Core :: XUL, defect, P3)

Description

[Mike Gratton]

When tracking down a mailnews polish bug, I found that the outliner's default
key event handler can set an invalid selection.

The current code (outliner.xml:411) grabs the index of the current selection,
and if it is not selected, toggles it. The problem is when there is nothing
selected, the current selection is -1 and the handler will cause the outliner
select the -1'th row, via toggleSelect().

This causes a bunch of hassles with anything using nsOutlinerSelection - there
will be a selected row with an index of -1. There is one selection range, but
the range's start and end index is -1. Etc, etc.

I've got a patch, will attach it.

Comment 1

[Mike Gratton]

Attachment: patch to check for valid index

Comment 2

[John Morrison]

bryner, can you have a look at this patch for outliner.xml

Comment 3

[Mike Gratton]

This blocks bug 44572, the invalid selection range is causing the delete button
to remain disabled.

Comment 4

[Mike Gratton]

CC'ing some more people. Guys, can you have a look at the patch?

Comment 5

[Jan Varga [:janv]]

looks good, r=varga

Comment 6

[David Hyatt]

Shouldn't you be building this error-handling right into the toggleSelect
method?  I assume this can happen through other code paths, so having
toggleSelect error-check would bullet-proof all of them.

Comment 7

[Mike Gratton]

The question is, where would be the best place to put such a check?

ToggleSelect ends up calling a bunch of other methods, like Select,
SetCurrentIndex, etc, etc. So, you'd want to put the check up the top of
ToggleSelect. But then these methods should also check the given index to make
sure it is valid. So, a call to ToggleSelect with a valid index could end up
checking the index several times over. Bearing in mind that the check should
verify the index against both the upper and lower bounds of valid indexes, I'm
not sure what the performance hit would be like. Probably not all that great.

Anyway, because of this I though it was probably best to just check the index
before giving it to the outliner. That way, nothing else expecting the incorrect
behaviour would break.

I'll put together a patch which adds a check to the appropriate
nsOutlinerSelection methods, if you think it's worthwhile.

Comment 8

[Peter Trudelle]

->hyatt

Comment 9

[Jan Varga [:janv]]

stealing

Comment 10

[Jan Varga [:janv]]

I don't think anyone is going to work on this given the plan to remove XUL tree widget in bug 1446335.