Closed Bug 384763 Opened 15 years ago Closed 15 years ago

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

Categories

(Firefox :: Bookmarks & History, defect)

x86
Windows XP
defect
Not set
major

Tracking

()

VERIFIED FIXED
Firefox 3 alpha6

People

(Reporter: Peter6, Assigned: mano)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

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
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.
gotcha,
this is only a problem in the root, not when you go to a folder
Summary: left click broken in bookmarks menu → left click broken in bookmarks menu (only in the root)
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
actually, this was broken by bug 337855
(sorry for not properly investigating this in the first place)
Blocks: 337855
No longer blocks: 384690
(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".
(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

No longer blocks: 337855
peter, thanks for narrowing down a small regression window.

that window points to me (and bug #382136), so I'll investigate.
Assignee: nobody → sspitzer
Also impossible to delete a bookmark in the main menu.
> 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".
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.
Sure it would... except there is no reason (ui-wise) to do so.
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.
Attached patch patch, v1 (obsolete) — Splinter Review
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...
> testing this patch now...

this regression is fixed and bug #382136 is still fixed, so seeking mano's review.
Status: NEW → ASSIGNED
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3 alpha6
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.
Attachment #268721 - Flags: review?(sspitzer) → review-
Attached patch another one (obsolete) — Splinter Review
Attachment #268721 - Attachment is obsolete: true
Attachment #268861 - Flags: review?(sspitzer)
Assignee: sspitzer → mano
Attachment #268693 - Attachment is obsolete: true
Attachment #268861 - Attachment is obsolete: true
Attachment #268865 - Flags: review?(sspitzer)
Attachment #268693 - Flags: review?(mano)
Attachment #268861 - Flags: review?(sspitzer)
Attachment #268865 - Attachment is patch: true
Attachment #268865 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 268865 [details] [diff] [review]
fix "area between separators" issue

r=sspitzer
Attachment #268865 - Flags: review?(sspitzer) → review+
mozilla/browser/components/places/content/menu.xml 1.75
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070619 Minefield/3.0a6pre ID:2007061909

VERIFIED
Status: RESOLVED → VERIFIED
Flags: blocking-firefox3? → blocking-firefox3+
Duplicate of this bug: 384893
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.