384968 - generating context menus that don't do the right thing when you right click on dom nodes without places nodes

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

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

generating context menus that don't do the right thing when you right click on dom nodes without places nodes

steps to reproduce:

a) you right click on "Bookmark This Page"
b) or in between separators (see bug #382136)

mano writes:

we can either fix that menu to work right, or not show a menu at all.

in fx2, it does not show a context menu when you right click a static menu item, but if you right click the menupopup it show a [Open, Open In New Window, Open In New Tab] context menu, all do nothing.

Comment 1

[Asaf Romano (gone)]

Attachment: patch

Comment 2

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

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

<sspitzerMsgMe> ok, so while testing
<sspitzerMsgMe> I opened up the bookmarks menu
<sspitzerMsgMe> right clicked on "bookmark this page", and I did not get a context menu (good)
<sspitzerMsgMe> but before closing the menu, I right clicked on "get bookmark addons" and I didn't get a context menu with the proper commands enabled.
<sspitzerMsgMe> additionally, when checking the "right click in between separators" I got a context menu.
<Mano> oh, command updating fun
<sspitzerMsgMe> r=sspitzer noted on https://bugzilla.mozilla.org/show_bug.cgi?id=384936
<Mano> oh, command updating fun
<Mano> i.e. the menu is already "focused"
<Mano> so we don't update command per the new selection

Comment 3

[Asaf Romano (gone)]

Attachment: another attempt

Comment 4

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

Comment on attachment 268901 [details] [diff] [review]
another attempt

mano, there are still scenarios where we don't do the right thing.

for example, in a sub menu with 5 separator followed by a bookmark item, mouse over the bookmark item at the bottom (DOMMenuItemActive) then mouse up to the separator area and right click.

you get the context menu where if you didn't mouse over the bookmark menu item, you would not.

Comment 5

[Asaf Romano (gone)]

Hrm? You should get context menu for separators...

Comment 7

[Jesse Ruderman]

Nominating because the dup is a blocker.

Comment 8

[Dietrich Ayala (:dietrich)]

pushing to M10.

"> This only occurs if the user right-clicks menuitems such as "Open all
> in tabs", and does nothing if they select items on the context menu.
> Not only should this not block M9, I don't think it should block the
> release.

Agree on not blocking M9, but feels like something that'll feel buggy
in a final release."

Comment 9

[Mike Connor [:mconnor]]

Not going to continue to block on this.  If you disagree with this decision, please renominate with reasons why we can't ship with this in final

Comment 10

[Marco Bonardo [:mak]]

this should be blocking since selecting DELETE on a bogus context menu will delete the parent folder and all its contents (dataloss)

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #10)
> this should be blocking since selecting DELETE on a bogus context menu will
> delete the parent folder and all its contents (dataloss)

Is that due to the bug where the wrong thing is selected when deleting/dropping items in the context menu? If so, I'd say that this isn't a blocker but that other one is. If not, then this blocks for dataloss.

Comment 12

[Marco Bonardo [:mak]]

related but not completely, the selection is correct, if an item is invalid we select its parent, so if those items have a context menu their selection is (somehow correctly) their parent

Comment 13

[Asaf Romano (gone)]

What part of this is still reproducible, apart open-all-in-tabs being enabled for the "general" area. 

Comment 14

[Asaf Romano (gone)]

er, s/open-all-in-tabs/delete/

Comment 15

[Asaf Romano (gone)]

Attachment: patch for the delete-enabled issue

Comment 16

[Dietrich Ayala (:dietrich)]

Comment on attachment 312510 [details] [diff] [review]
patch for the delete-enabled issue

r=me

Comment 17

[Marco Bonardo [:mak]]

what's the matter in having the context menu in the "general" area at all? FX does not have it out of bookmark items... wouldn't be better not showing a context menu if the item does not have a node?

Comment 18

[Marco Bonardo [:mak]]

PS: apart the special case for "empty" item

Comment 19

[Asaf Romano (gone)]

Let's follow up on that, post-f3 for sure at this point.

Comment 20

[Asaf Romano (gone)]

mozilla/browser/components/places/content/menu.xml, 1.126
mozilla/browser/components/places/content/toolbar.xml 1.148

Comment 21

[Marco Bonardo [:mak]]

caused regression, bug 427200

Comment 22

[Tracy Walker [:tracy]]

Verified with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008042407 Minefield/3.0pre

Comment 23

[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