382136 - in Bookmarks Menu - delete separator at bottom position, deletes bookmark

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070526 Minefield/3.0a5pre ID:2007052604 [cairo]

repro:
Open FF
Open bookmarks menu
Add a separator (or more than one) at the bottom of one of the folders
Open Bookmarks menu
Go back to that folder , rightclick on the separator you just added and select delete

result:
the bookmark above the separator is deleted, the separator remains

Comment 1

[Ria Klaassen (not reading all bugmail)]

Not possible to highlight a separator. At least it should take no action when nothing is highlighted. This looks related to Bug 381767 if I understand that bug correctly.
I managed to delete them sometimes but you need a pixel magnifier for that. You need to place your mouse carefully on the line or on the pixel below that line.

Comment 2

[Peter van der Woude [:Peter6]]

Yeah, a pixel magnifier might help me ;-)
It doesn't matter that much though.

We should NEVER delete a bookmark if it isn't selected/highlighted.

Comment 3

[(not reading, please use [e-mail] instead)]

I'm having problems reproducing this on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070612 Minefield/3.0a6pre.

Peter / Ria:  are you still able to reproduce it?

Comment 4

[Ria Klaassen (not reading all bugmail)]

Yeah, fully.

Comment 5

[(not reading, please use [e-mail] instead)]

I'm able to reproduce this.

1)  create a new profile
2)  open the bookmarks menu, open the "Mozilla Firefox" menu, right click on "Open in Tabs" and create a new separator (which will add it to the bottom, under "About us" bookmark.
3)  open the bookmarks menu, open the "Mozilla Firefox" menu, right click on the separator under "About us" bookmark, delete

actual results:

a) about us bookmark is deleted

expected results:

b) separator is deleted

Comment 6

[(not reading, please use [e-mail] instead)]

after a bit of debugging, here's what I think is going on:

_removeRange() in controller.js is getting called with the wrong range.

specifically, we are getting called with the last bookmark that was active.

the reason is that in menu.xml, we set the view's selection when the DOMMenuItemActive event is handled, but that event is not getting fired for separators.

Additionally, I think we might want to clear the selection (set this._selection = null) on the DOMMenuItemInactive event.

Comment 7

[(not reading, please use [e-mail] instead)]

as for how this worked in firefox 2, see http://lxr.mozilla.org/seamonkey/source/browser/components/bookmarks/content/bookmarksMenu.js#173

Comment 8

[(not reading, please use [e-mail] instead)]

Attachment: patch

Comment 9

[(not reading, please use [e-mail] instead)]

the fix helps explain why it can be tricky to reproduce this.

menu.xml also contains a handler for mousedown that will also set the selection.

http://lxr.mozilla.org/seamonkey/source/browser/components/places/content/menu.xml#750

but it can be really tough to click EXACTLY on the pixel for the separator.

If you do, we fire the mousedown, and the event.target is the menuseparator, and things will work as expected.

miss it slightly, and you think you are going to delete the menuseparator, but you are really not, and since we don't clear the selection (on DOMMenuItemInactive), you end up deleting the last bookmark that was active.

note, this is a windows only bug since mac doesn't allow you to do context menus on bookmark menus.

stealing from steve.

Comment 10

[(not reading, please use [e-mail] instead)]

Comment on attachment 268285 [details] [diff] [review]
patch

actually, might be better to set the selection to the result note.

testing that patch now...

Comment 11

[(not reading, please use [e-mail] instead)]

Comment on attachment 268285 [details] [diff] [review]
patch

for now, I think this is correct as it prevents dataloss.

In this scenario, you'll only be able to do new folder, new bookmark, new separator.  all other other commands will be disabled.

from some quick testing, we've got a bunch of bugs with context menus of containers and of the generated ("Open in Tabs" and "Open <feed name">) menu items which I'll spin off.

Comment 12

[(not reading, please use [e-mail] instead)]

> we've got a bunch of bugs with context menus of containers and of the generated > ("Open in Tabs" and "Open <feed name">) menu items which I'll spin off.

see bug #384375.

Comment 13

[(not reading, please use [e-mail] instead)]

fixed.

Checking in menu.xml;
/cvsroot/mozilla/browser/components/places/content/menu.xml,v  <--  menu.xml
new revision: 1.73; previous revision: 1.72
done

Comment 14

[(not reading, please use [e-mail] instead)]

note, this patch caused a regression, see bug #384763

Comment 15

[Al Billings [:abillings - ex-MoCo]]

I can't get any context menu off of the bookmarks menu following the instructions in comment 5. Is there some special trick to this or do you mean the sidebar?

Comment 16

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv