318532 - Lost focus when deleting first item in history sidebar

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[David Benes]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

Deleting first item in History Sidebar causes no item have focus.

Deleting any other item sets focus on the next item after the deleted one (and that is correct).


Reproducible: Always

Steps to Reproduce:
1. Open history sidebar
2. Sort items by date
3. Delete category Today (which is certainly the first one)

You can alternatively use history search and delete fist item found.

Actual Results:  
None of items or categories have focus.

Expected Results:  
Focus should be set on the next item after deleted one.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051130 Firefox/1.6a1 ID:2005113005
Yeah, I've also seen that for a very long time. I've never reported it because it was so obvious that I was pretty sure it was a well-known issue although I couldn't find the dupe.

Comment 2

[David Benes]

Attachment: First patch - just RFC

Patch for mozilla/toolkit/obsolete/content/nsTreeController.js

There is condition: if (max.value) {...

Once somebody does delete the first item in history list, value of 'max.value' is zero and condition is not met. This results selecting the 'undefined' item.

Comment 3

[Adam Guthrie]

*** Bug 356520 has been marked as a duplicate of this bug. ***

Comment 4

[Adam Guthrie]

David, you should request review from someone on the patch. Neil Deakin (enndeakin@) would be a good choice.

Comment 5

[David Benes]

Attachment: Patch for 318532, Firefox v. 2.0.0.1

Created new patch for the latest Firefox version (2.0.0.1).

Comment 6

[Neil Deakin]

Comment on attachment 255359 [details] [diff] [review]
Patch for 318532, Firefox v. 2.0.0.1

Looks OK, assuming there isn't some way I'm not seeing of not having value set at all.

Comment 7

[David Benes]

(In reply to comment #6)
> (From update of attachment 255359 [details] [diff] [review])
> Looks OK, assuming there isn't some way I'm not seeing of not having value set
> at all.
> 

Max.value is set everytimes nsTreeController_delete() is called.
Empty tree doesn't cause any harm. There should be no problem.

Comment 8

[[e-mail]]

Comment on attachment 255359 [details] [diff] [review]
Patch for 318532, Firefox v. 2.0.0.1

>+  if (max.value >= 0) {
This is always going to be true, right? I'm wildly guessing that what hewitt was trying to do was to check that there was at least one row left to select. I think that would be a good idea if you did that check ;-)

>     var newIndex = max.value - (max.value - min.value);
Wow, what was hewitt smoking? Might as well fix this at the same time ;-)

Comment 9

[David Benes]

Attachment: Optimized version of previous patch, changed range checking.

Hi Neil(s),

Here is my little research:
(might be obvious, but just for sake of completness)

min.value and max.value are both always numeric (never undefined).
min.value contains begining index of the first continuous selection.
max.value contains ending index of the first continuous selection.
Tree indexing is zero based.
Error values of min.value and max.value returned by
 this.treeSelection.getRangeAt(i, min, max) are -1.
 (according to layout/xul/base/src/tree/src/nsTreeSelection.cpp)

Max.value won't be negative except in error case, so i changed the condition
accordingly to:

if (max.value != -1) {

Note that the original condition "if (max.value)" would skip processing of the first deleted item - item with index 0, and so skips focus setting.

Then, deleting the last remaining item in the tree would now cause selecting
item with index -1. So i added condition to prevent this:
if (newIndex >= 0) 

>>var newIndex = max.value - (max.value - min.value);
>Wow, what was hewitt smoking? Might as well fix this at the same time ;-)

Don't know what was hewitt smokin, but he evidently forgot the
optimization part ;-)

Comment 10

[[e-mail]]

Comment on attachment 255443 [details] [diff] [review]
Optimized version of previous patch, changed range checking.

Hmm... if max.value (and therefore min.value) is -1, then newIndex >= 0 will be false anyway, no?

Comment 11

[David Benes]

(In reply to comment #10)
> (From update of attachment 255443 [details] [diff] [review])
> Hmm... if max.value (and therefore min.value) is -1, then newIndex >= 0 will be
> false anyway, no?
> 

If max.value is -1, then newIndex will be undefined. Sorry if I didn't get
the point. newIndex is set only if max.value(and therefore min.value)
is different from -1.

Deleting last remaining item looks this way:
max.value is 0
min.value is 0
therefore: newIndex is 0

Item has been already removed, so rowCount is 0.

if (newIndex >= this.treeView.rowCount) --newIndex;
means: if (0 >= 0) --newIndex;
therefore: newIndex is -1

Then newIndex >= 0 is false. We don't want to set
item focus in the empty tree.

Comment 12

[Nickolay_Ponomarev]

toolkit/obsolete/content/nsTreeController.js  1.5

Comment 13

[Nickolay_Ponomarev]

Can/should this have a unit test?