329337 - Bookmarks and "Open in Tabs" don't work in Bookmarks Menu, but do from Places Toolbar

Status: RESOLVED FIXED

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

Description

[Frederic Bezies]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060304 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060304 Firefox/1.6a1

Simple to see. Build or get a very recent nightly build.

Try to use "open in tabs" from bookmarks menu

Reproducible: Always

Steps to Reproduce:
1.See details
2.
3.

Actual Results:  
Nothing happens

Expected Results:  
opening choosen bookmarks.

Related to bug 329266 ?!

Comment 1

[Wayne Woods]

Actually, I think this bug is broader than just "Open in Tabs"...

In a fresh browser window, NO bookmarks work at all from the menu in current nightlies (I'm surprised another bug hasn't been filed about that, but I can't find one). I think that issue is the same bug as the "Open in Tabs" one because they both exhibit the same peculiar behaviour...

Load a page by clicking on a button in the bookmarks/places toolbar. Having done that, "Open in Tabs" will now do something. But instead of working as it's supposed to, it'll just open a new tab with the same page you just loaded. The same thing happens if you now select a bookmark from the menu... it'll just reload the previous page rather than the URL it's supposed to.

Also of note, the Bookmarks Menu stopped working in the 20060304 build, which is the same build in which "Open in Tabs" reappeared on the menu (tested with fresh profile).

Comment 2

[David Lawhon]

I can certainly confirm this bug still occurs as of the 3/9/06 build (1.6a1 daily), running under Mac OS X 10.4.5 (PPC), 2GB RAM, Dual G5 2.5 Ghz.

Comment 3

[Josh Aas]

Attachment: incomplete fix, v0.5

According to Annie, we're not sending "DOMMenuItemActive" on Mac OS X when the mouse enters the rect for any given menu item.

This is a partial patch, which will recieve the event we want from Carbon and track down the associated nsMenuItem. However, I have yet to write the code that goes from nsIMenuItem->nsIContent and sends the actual event. I will do that soon if nobody else wants to finish that up. This patch at least takes care of the Carbon API coding that needs to be done.

Comment 4

[Josh Aas]

Attachment: incomplete fix, v0.8

This is another incomplete fix. I added an API for sending a DOM event to nsMenuItem objects. All that needs to be done is generate the DOM event we want to send in nsMenuX.cpp. I don't know how to do that, hoping somebody else can pick it up here.

Comment 5

[Phil Ringnalda (:philor)]

*** Bug 330301 has been marked as a duplicate of this bug. ***

Comment 6

[Josh Aas]

Attachment: fix v1.0

This makes Places work on Mac OS X. I only send an activate DOM event when we're over actual menu items, not separators or submenu items. Not sure if that is correct, but it works for Places and its better than what we do now, which is nothing.

Comment 7

[Josh Aas]

Note: fix v1.0 is against MOZILLA_1_8_BRANCH

Comment 8

[Josh Aas]

I noticed that the patch I put up sends activate events even when the mouse goes over disabled menus. If we don't want to do that it is an easy fix.

Comment 9

[Mark Mentovai]

Comment on attachment 214977 [details] [diff] [review]
fix v1.0

+    nsCOMPtr<nsIMenuItem> bTargetMenuItem(do_QueryInterface(aTargetMenuItem));

What's the b for?  I've only seen this used for bool args, which this isn't.

The code here looks OK.  I'd add an out param to DispatchDOMEvent to carry the status back to the caller.

Don't check in without accounting for cocoa widgets.

r=me, once you resolve the behavioral stuff for disabled menu items.  Nothing wrong with calling bTargetMenuItem->GetEnabled().

Comment 10

[Annie Sullivan]

(In reply to comment #8)
> I noticed that the patch I put up sends activate events even when the mouse
> goes over disabled menus. If we don't want to do that it is an easy fix.

I just tested on a windows build, and the activate events do fire for disabled menu items on windows, so I think this behavior is fine.

Comment 11

[Josh Aas]

I prefixed one target menu item pointer with "a" and the other with "b" since they are similar interfaces to the same thing. I had to come up with different names.

Comment 12

[Josh Aas]

Attachment: fix v1.1

This is just like v1.0 but it includes a stub impl for Cocoa so we don't break Camino, and it also relays whether or not a handler called prevent default through the nsIMenuItem API.

Comment 13

[Mark Mentovai]

Comment on attachment 215034 [details] [diff] [review]
fix v1.1

NS_METHOD -> NS_IMETHODIMP on checkin

Comment 14

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 215034 [details] [diff] [review]
fix v1.1

sr=[e-mail]

Land it on the trunk then request branch approval once it tests OK.

Comment 15

[Josh Aas]

fix v1.1 landed on trunk

Comment 16

[Josh Aas]

Attachment: cocoa trunk fix v1.0

The move to Cocoa on the trunk was very straightforward. This is exactly what we did for Carbon widgets, I'm just going to check it in.

Comment 17

[Josh Aas]

cocoa trunk fix v1.0 checked in

Comment 18

[[e-mail]]

Note that certain features might also rely on you firing matching DOMMenuItemInactive events.

Comment 19

[Boris Zbarsky [:bzbarsky]]

I have to wonder -- why are we getting the docshell and prescontext if we're not using them?  If we're just making sure they exist for some reason, that may be worth documenting so someone doesn't "fix" it later....

Comment 20

[Josh Aas]

+  // get a pres context
+  nsPresContext* ourPresContext;
+  nsCOMPtr<nsIDocShell> docShell = do_QueryReferent(mDocShellWeakRef);
+  if (!docShell) {
+    NS_WARNING("Failed to QI weak docshell ref to nsIDocShell");
+    return NS_ERROR_FAILURE;
+  }
+  rv = MenuHelpersX::DocShellToPresContext(docShell, &ourPresContext);
+  if (NS_FAILED(rv)) {
+    NS_WARNING("Failed to get pres context from docshell");
+    return NS_ERROR_FAILURE;
+  }

Yup - this code is unnecessary. I was using it before I switched over to using nsIDOMEventTarget.

Comment 21

[Josh Aas]

I checked in a followup fix to remove that dead code on the trunk. I'll remove it on checkin to the branch.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 215034 [details] [diff] [review]
fix v1.1


>+  
>+  // create DOM event
>+  nsIDOMEvent* event;
>+  DOMEventFactory->CreateEvent(NS_LITERAL_STRING("Events"), &event);
>+  event->InitEvent(eventName, PR_TRUE, PR_TRUE);

Er, why nsIDOMEvent*? It should be nsCOMPtr<nsIDOMEvent> and then
DOMEventFactory->CreateEvent(NS_LITERAL_STRING("Events"), getter_AddRefs(event));, otherwise you leak, right?

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

and please check the return value of CreateEvent. That may fail if OOM.

Comment 24

[Josh Aas]

I'll check that out tomorrow, seems like it is the case. Thanks!

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 215034 [details] [diff] [review]
fix v1.1

public/nsIMenuItem.h needs a new IID!

and you can't land interface changes on the branch

Comment 26

[Phil Ringnalda (:philor)]

*** Bug 330564 has been marked as a duplicate of this bug. ***

Comment 27

[Josh Aas]

Attachment: memory leak/iid fix

Comment 28

[Josh Aas]

fix for memory leak, iid, and CreateEvent return value checked in

Comment 29

[Mike Connor [:mconnor]]

Comment on attachment 215034 [details] [diff] [review]
fix v1.1

You can't change interfaces like this on the branch, as biesi noted.

Comment 30

[[e-mail]]

(In reply to comment #18)
>Note that certain features might also rely on you firing matching DOMMenuItemInactive events.
I was thinking of the statustext code in toolkit/content/widgets/toolbar.xml but as long as you don't send a DOMMenuBarActive event you'll be OK.

Comment 31

[Asaf Romano (gone)]

Josh, are you planing to port these patches to the 1.8 branch?

Comment 32

[Josh Aas]

No, so long as the Places people are able to work around the problem on the branch. I was told they had a fix for it.

Comment 33

[Asaf Romano (gone)]

At the very least, i need this for bug 333863.

Comment 34

[Asaf Romano (gone)]

Attachment: branch-friendly version

Comment 35

[Mark Mentovai]

Comment on attachment 218395 [details] [diff] [review]
branch-friendly version

>+class msIMenuItem_MOZILLA_1_8_BRANCH : public nsIMenuItem  {
        ^

Fix that throughout, and r+a=me.

Comment 36

[Mark Mentovai]

Comment on attachment 218395 [details] [diff] [review]
branch-friendly version

Also, you can remove some unnecessary code here as was done on the trunk:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsMenuItemX.cpp&branch=&root=/cvsroot&subdir=mozilla/widget/src/mac&command=DIFF_FRAMESET&rev1=1.31&rev2=1.32

Comment 37

[Asaf Romano (gone)]

Attachment: branch patch, as checked in

Checking in public/nsIMenuItem.h;
/cvsroot/mozilla/widget/public/nsIMenuItem.h,v  <--  nsIMenuItem.h
new revision: 1.29.16.1; previous revision: 1.29
done
Checking in src/mac/nsMenuItemX.cpp;
/cvsroot/mozilla/widget/src/mac/nsMenuItemX.cpp,v  <--  nsMenuItemX.cpp
new revision: 1.28.12.1; previous revision: 1.28
done
Checking in src/mac/nsMenuItemX.h;
/cvsroot/mozilla/widget/src/mac/nsMenuItemX.h,v  <--  nsMenuItemX.h
new revision: 1.7.16.1; previous revision: 1.7
done
Checking in src/mac/nsMenuX.cpp;
/cvsroot/mozilla/widget/src/mac/nsMenuX.cpp,v  <--  nsMenuX.cpp
new revision: 1.57.4.1; previous revision: 1.57
done
Checking in src/mac/nsMenuX.h;
/cvsroot/mozilla/widget/src/mac/nsMenuX.h,v  <--  nsMenuX.h
new revision: 1.14.12.1; previous revision: 1.14
done

Comment 38

[Asaf Romano (gone)]

*** Bug 330932 has been marked as a duplicate of this bug. ***

Comment 39

[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