pressing delete with nothing selected breaks delete in Bookmark Manager

RESOLVED FIXED

Status

()

P3
normal
RESOLVED FIXED
14 years ago
12 years ago

People

(Reporter: dan, Assigned: vlad)

Tracking

({fixed-aviary1.0})

unspecified
x86
Windows 2000
fixed-aviary1.0
Points:
---
Bug Flags:
blocking-aviary1.0 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [eta 10/8] -- need help understanding tree code)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

14 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040614 Firefox/0.9
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040614 Firefox/0.9

If you are deleting items one after the other in the bookmark manager, the focus
will shift downwards in the list of items as you repetitively delete. When it
hits the bottom and no item is selected hitting delete again will cause the
bookmark manager to ignore subsequent delete commands even if you have selected
an actual item. Closing the bookmark manager and restarting it solves the problem.

Reproducible: Always
Steps to Reproduce:
1. Open Bookmark Manager
2. Select the final item
3. Delete it
4. No item is no selected, select the new final item
5. Delete commands are ignored

Actual Results:  
The bookmark manager ignores delete commands until it is restarted.

Expected Results:  
Allowed the focus to be shifted to another item and deleted.

Comment 1

14 years ago
Confirming Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7)
Gecko/20040626 Firefox/0.9.1
This also disables the righ click context menu in the bookmarks manager.

Comment 2

14 years ago
I run into this all the time and never realized what the root cause was. If you
accidentally hit delete with nothing selected in the bookmarks manager, delete
is broken until to perform some other action like adding or moving an item. 

This is something we should get fixed for 1.0. 
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-aviary1.0?
Summary: When repetively deleting in the Bookmark Manager, selecting delete when no bookmark is selected causes subsequent deletions to be ignored → pressing delete with nothing selected breaks delete in Bookmark Manager

Comment 3

14 years ago
-> Vlad

I see three bugs here:

1. Deleting off the end of the list shouldn't leave you in a state where nothing
is selected. If you delete the last item, it should select the new last item.

2. Ctrl+clicking on the selected item shouldn't deselect it.

In other words, there really should be no way to not have something selected in
bookmarks. That state just doesn't really make sense.

And finally:

3. Pressing delete with nothing selected shouldn't break delete. This shouldn't
really be an issue if and when (1) and (2) are fixed but it's still a bug that
should be fixed.
Assignee: p_ch → vladimir
Flags: blocking-aviary1.0? → blocking-aviary1.0+

Updated

14 years ago
Priority: -- → P3

Comment 4

14 years ago
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040731
Firefox/0.9.1+ (bangbang023)

Bug is still effective in the latest builds.
Easy to reproduced just go to the end of your bookmarks and Delete twice.
Then try to delete anything else and it is broken.

I would totally make the delete go upwards if it reached the end of the list not
downwards as it appears to do.
Status: NEW → ASSIGNED
Ok, going to need some help from someone who knows trees better than I do.

The comment at

http://lxr.mozilla.org/aviarybranch/source/browser/components/bookmarks/content/bookmarksTree.xml#432

implies that the selection will be restored to the item previous to the one just
deleted if there are no items; that obviously isn't true. 
nsITreeSelection::rangedSelect does no validation of its arguments, so this just
blindly sets the selection to an invalid index.  I need to know:

a) how to get the last valid index in a tree;
b) how to get the previous sibling index, or maybe even previous visible index
(so we don't go upwards into container whose contents aren't expanded).

Updated

14 years ago
Severity: minor → normal
Whiteboard: [eta 10/8]

Updated

14 years ago
Whiteboard: [eta 10/8] → [eta 10/8] -- need help understanding tree code
Created attachment 161795 [details] [diff] [review]
248817-bm-delete-brokenness.patch

Don't screw up the selection if we delete the last item.  I don't bother to
reselect anything if you nuke the last item, but it breaks nothing.  Reselcting
the right thing (the previous visible thing in the tree, including nested
things that are open, etc.) is Hard, so we don't do it.
Comment on attachment 161795 [details] [diff] [review]
248817-bm-delete-brokenness.patch

r+=a=ben@mozilla.org
Attachment #161795 - Flags: review+
Attachment #161795 - Flags: approval-aviary+
(In reply to comment #7)
> Don't screw up the selection if we delete the last item.  I don't bother to
> reselect anything if you nuke the last item, but it breaks nothing.

It's annoying, though, since you're de facto selecting row -1: the up arrow key
doesn't do anything, and down arrow selects the first thing in the tree, far
from where you were.

> Reselcting
> the right thing (the previous visible thing in the tree, including nested
> things that are open, etc.) is Hard, so we don't do it.

Is it Hard? What case did I miss, that selecting rowCount-1 fails on?
(In reply to comment #9)
> > Reselcting
> > the right thing (the previous visible thing in the tree, including nested
> > things that are open, etc.) is Hard, so we don't do it.
> 
> Is it Hard? What case did I miss, that selecting rowCount-1 fails on?

The case that i'm worried about is when the last two items are a folder and a
bookmark; deleting the bookmark should move selection up to the folder itself,
but rowCount-1 will move it into the innermost last item in the folder, which
might not even be opened.

One solution is that we have to pass along the level of the item, and then walk
up the tree until we find the first item of that same level -- but if the folder
mentioned above /is/ opened, I think you'd want to select the innermost item
that's visible.  In other words, the item you'd want to have selected is the one
that's visually right above the one the user deleted, and figuring out what that
item is would take a bit of work.  It would be nice to fix this right at some point.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Keywords: fixed-aviary1.0
Resolution: --- → FIXED
(In reply to comment #10)
> The case that i'm worried about is when the last two items are a folder and a
> bookmark; deleting the bookmark should move selection up to the folder itself,
> but rowCount-1 will move it into the innermost last item in the folder, which
> might not even be opened.

Um, only if rowCount is buggy cross-platform: I'm running a 20041011 nightly on
WinXP with your patch plus:

            var maxCount = this.treeBoxObject.view.rowCount;
            if (nextRow < maxCount) {
              newRanges.push({min: nextRow, max: nextRow});
            } else {
              newRanges.push({min: maxCount-1, max: maxCount-1});
            }

hacked in, and it does this:

Last     Last-1            Selected after
BM       BM2               BM2
BM       Folder-open       Last BM in folder
BM       Folder-closed     Folder
BM       Separator         Separator
Folder   BM                BM
Folder   Folder-open       Last BM in folder
Folder   Folder-closed     Folder
Folder   Separator         Separator

Not so for you? (Ghu, I'm arguing on a closed bug, someone slap me.)
Created attachment 161805 [details] [diff] [review]
248817-bm-delete-brokenness-1.patch

I must've been doing something silly last time, because it wasn't working for
me.  But, apparently, it does work.  New patch attached, thanks :)
Attachment #161795 - Attachment is obsolete: true
Comment on attachment 161805 [details] [diff] [review]
248817-bm-delete-brokenness-1.patch

one more time, ben.. yes, this bug is already resolved :)
Attachment #161805 - Flags: review?(bugs)
Attachment #161805 - Flags: approval-aviary?

Updated

14 years ago
Attachment #161805 - Flags: review?(bugs)
Attachment #161805 - Flags: approval-aviary?
(I just cheated and piggybacked the additional patchlet to something else)
*** Bug 264641 has been marked as a duplicate of this bug. ***
*** Bug 224912 has been marked as a duplicate of this bug. ***
sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change
QA Contact: mconnor → bookmarks
You need to log in before you can comment on or make changes to this bug.