If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Lost focus when deleting first item in history sidebar

RESOLVED FIXED

Status

()

Toolkit
XUL Widgets
--
minor
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: David Benes, Assigned: David Benes)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

12 years ago
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.
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.
(Assignee)

Comment 2

12 years ago
Created attachment 205571 [details] [diff] [review]
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.
(Assignee)

Updated

12 years ago
OS: Linux → All

Updated

12 years ago
Component: History → XUL Widgets
Product: Firefox → Toolkit
QA Contact: history → xul.widgets
Hardware: PC → All
Version: unspecified → Trunk

Comment 3

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

Comment 4

11 years ago
David, you should request review from someone on the patch. Neil Deakin (enndeakin@) would be a good choice.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 5

11 years ago
Created attachment 255359 [details] [diff] [review]
Patch for 318532, Firefox v. 2.0.0.1

Created new patch for the latest Firefox version (2.0.0.1).
Attachment #205571 - Attachment is obsolete: true
Attachment #255359 - Flags: first-review?(enndeakin)

Comment 6

11 years ago
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.
Attachment #255359 - Flags: first-review?(enndeakin) → first-review+
(Assignee)

Comment 7

11 years ago
(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.
(Assignee)

Updated

11 years ago
Attachment #255359 - Flags: second-review?(neil)

Comment 8

11 years ago
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 ;-)
Attachment #255359 - Flags: second-review?(neil) → second-review-
(Assignee)

Comment 9

11 years ago
Created attachment 255443 [details] [diff] [review]
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 ;-)
Attachment #255359 - Attachment is obsolete: true
Attachment #255443 - Flags: second-review?(enndeakin)
Attachment #255443 - Flags: first-review?(neil)

Comment 10

11 years ago
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?
Attachment #255443 - Flags: first-review?(neil) → first-review+
(Assignee)

Comment 11

11 years ago
(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.

Updated

11 years ago
Attachment #255443 - Flags: second-review?(enndeakin) → second-review+
(Assignee)

Updated

11 years ago
Whiteboard: [checkin needed]

Comment 12

11 years ago
toolkit/obsolete/content/nsTreeController.js  1.5
Assignee: nobody → dave_forward
Whiteboard: [checkin needed]

Comment 13

11 years ago
Can/should this have a unit test?
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.