Closed Bug 97421 Opened 22 years ago Closed 5 years ago
Default outliner xbl key event handler causes invalid selection
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.
bryner, can you have a look at this patch for outliner.xml
This blocks bug 44572, the invalid selection range is causing the delete button to remain disabled.
CC'ing some more people. Guys, can you have a look at the patch?
looks good, r=varga
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.
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.
Assignee: trudelle → hyatt
Assignee: hyatt → varga
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: mozilla1.0.1 → mozilla1.0
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: jrgmorrison → xptoolkit.widgets
I don't think anyone is going to work on this given the plan to remove XUL tree widget in bug 1446335.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.