384763 - left click broken in bookmarks menu (only in the root)

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070616 Minefield/3.0a6pre ID:2007061621

repro:
Open bookmarks menu
Leftclick on a bookmark to open a page

result:
nothing happens

richtclick and middleclick are both fine

regression from Bug 384690

Comment 1

[Peter van der Woude [:Peter6]]

This is odd.
I could repro this , even after restart, but now the problem appears to be gone.
There were some more people complaining so I'll just leave it open just in case.

Comment 2

[Peter van der Woude [:Peter6]]

gotcha,
this is only a problem in the root, not when you go to a folder

Comment 3

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a6pre) Gecko/2007061702 Minefield/3.0a6pre
I see the same with a new profile and Bookmarks > Get Bookmark Add-ons

Comment 4

[Peter van der Woude [:Peter6]]

actually, this was broken by bug 337855
(sorry for not properly investigating this in the first place)

Comment 5

[pal-moz]

(In reply to comment #4)
> actually, this was broken by bug 337855
> 

no problem with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/2007061519, bug#337855 was landed.
so I don't think "this was broken by bug 337855".

Comment 6

[Peter van der Woude [:Peter6]]

(In reply to comment #5)
> (In reply to comment #4)
> > actually, this was broken by bug 337855
> > 
> 
> no problem with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre)
> Gecko/2007061519, bug#337855 was landed.
> so I don't think "this was broken by bug 337855".
> 
You're so rigt
At the risk of getting in trouble with my wife I downloaded all builds and found as regressionwindow (two builds later):

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1181964720&maxdate=1181970839

Comment 7

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

peter, thanks for narrowing down a small regression window.

that window points to me (and bug #382136), so I'll investigate.

Comment 8

[Ria Klaassen (not reading all bugmail)]

Also impossible to delete a bookmark in the main menu.

Comment 9

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

> that window points to me (and bug #382136), so I'll investigate.

this regression is definitely caused by my change.

here's what's happening:

1) we get the DOMMenuItemActive event for the menu item when we hover over it.
2) we get the mousedown event when we click on it
3) we get two DOMMenuItemInactive events (perhaps as the menu is dismissed?) which clears the selection
4) our places controller gets the command event and we call openSelectedNodeWithEvent() and ask the menu for the selectedNode (which is null, as my DOMMenuItemInactive handler sets selection to null).  since the selectedNode is null, we don't load the bookmark.
5) then our menu gets the command event and set the selection to be the node we clicked.

note for submenus, steps 4 and 5 are reversed and the menu gets the command event before the controller, so that is why this bug is "only in the root".

Comment 10

[Peter van der Woude [:Peter6]]

wouldn't it be possible to change bookmarks in a way that there is just 1 folder in the root, containing the bookmars and folders (basically containing the current root).
This way there would be no root visable and all bookmars and folders would be non-root.

Comment 11

[Asaf Romano (gone)]

Sure it would... except there is no reason (ui-wise) to do so.

Comment 12

[Peter van der Woude [:Peter6]]

If it would simplify the code because we can treat everything equal, that would be a good reason.
UI-wise you'd never notice of course.

Comment 13

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

Attachment: patch, v1

instead of using DOMMenuItemInactive handler, clear the selection if the event is not valid.

note, the minimal version of this patch to fix this regression and still fix bug #382136 would be to add the else clause to the mousedown event.  

in the case of bug #382136, the event target is the menupopup, so I've explictly called that out in eventValid().

testing this patch now...

Comment 15

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

> testing this patch now...

this regression is fixed and bug #382136 is still fixed, so seeking mano's review.

Comment 16

[Asaf Romano (gone)]

Attachment: another approach

Comment 17

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

Comment on attachment 268721 [details] [diff] [review]
another approach

from a quick test, unlike my approach, this will not address bug #382136.

note, It seems incorrect that you are calling setSelectionForEvent() from the DOMMenuItemInactive handler.

Comment 18

[Asaf Romano (gone)]

Attachment: another one

Comment 19

[Asaf Romano (gone)]

Attachment: fix "area between separators" issue

Comment 20

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

Comment on attachment 268865 [details] [diff] [review]
fix "area between separators" issue

r=sspitzer

Comment 21

[Asaf Romano (gone)]

mozilla/browser/components/places/content/menu.xml 1.75

Comment 22

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

there is a spin off bug about when you get a context menu for a dom node
without a places node, the context menu has items it shouldn't and/or they
don't work.  see bug #384968 for that issue.

Comment 23

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070619 Minefield/3.0a6pre ID:2007061909

VERIFIED

Comment 25

[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