172276 - implement context-menu on menupopup

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Pierre Chanial]

needed for opening the bookmarks of a folder in tabs.

Comment 1

[Pierre Chanial]

-> ideally 0.3, since we have annouced in the 0.2 release notes that we will
handle differently groupmarks. But you can push it, since there is already
support for toolbar buttons and trees.

Comment 2

[Asa Dotzler [:asa]]

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

Comment 3

[David Hyatt]

Attachment: Patch to support context menus on menupopups

Comment 4

[David Hyatt]

Moving to widgets. This is not just a phoenix bug.

Comment 5

[Joe Hewitt (gone)]

Comment on attachment 101613 [details] [diff] [review]
Patch to support context menus on menupopups

sr=hewitt

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 101613 [details] [diff] [review]
Patch to support context menus on menupopups

>+++ base/src/nsMenuPopupFrame.cpp	3 Oct 2002 23:48:13 -0000
>@@ -620,13 +620,25 @@
>       targetDocument->GetShellAt(0, getter_AddRefs(shell));
>       nsCOMPtr<nsIViewManager> viewManagerTarget;
>       if ( shell ) {
>-        shell->GetViewManager(getter_AddRefs(viewManagerTarget));
>-        if ( viewManagerTarget ) {
>-          nsIView* rootViewTarget;
>-          viewManagerTarget->GetRootView(rootViewTarget);
>-          if ( rootViewTarget ) {
>-            nscoord unusedX, unusedY;
>-            rootViewTarget->GetOffsetFromWidget(&unusedX, &unusedY, *getter_AddRefs(targetDocumentWidget));
>+        // We might be inside a popup widget. If so, we need to use that widget and
>+        // not the root view's widget.
>+        nsIFrame* targetFrame;
>+        shell->GetPrimaryFrameFor(targetAsContent, &targetFrame);
>+        nsIView* parentView = nsnull;
>+        GetRootViewForPopup(mPresContext, targetFrame, &parentView);

No way null could come back from GPFF or GetRootViewForPopup, I guess.

>+        nsCOMPtr<nsIWidget> parentViewWidget;

Where is parentViewWidget used?

>+        GetWidgetForView(parentView, *getter_AddRefs(targetDocumentWidget));
>+        if (!targetDocumentWidget) {
>+          // We aren't inside a popup. This means we should use the root view's
>+          // widget.
>+          shell->GetViewManager(getter_AddRefs(viewManagerTarget));
>+          if ( viewManagerTarget ) {
>+            nsIView* rootViewTarget;
>+            viewManagerTarget->GetRootView(rootViewTarget);
>+            if ( rootViewTarget ) {

Since you're changing these lines, why not fix the if ( foo ) bogo-style to if
(foo) while you're there?

>@@ -1756,6 +1816,16 @@
> NS_IMETHODIMP
> nsMenuPopupFrame::KeyboardNavigation(PRUint32 aKeyCode, PRBool& aHandledFlag)
> {
>+  // See if we have a context menu open.
>+  nsCOMPtr<nsIMenuParent> contextMenu;
>+  GetContextMenu(getter_AddRefs(contextMenu));
>+  if (contextMenu) {
>+    printf("Found a context menu.\n");
>+    return contextMenu->KeyboardNavigation(aKeyCode, aHandledFlag);
>+  }
>+
>+  printf("Didn't find one. Checking here.\n");
>+

Why are you checking in printfs?

>+++ base/src/nsPopupSetFrame.cpp	3 Oct 2002 23:48:14 -0000
>@@ -408,11 +408,30 @@
>   nsPopupFrameList* entry = mPopupList->GetEntryByFrame(aPopup);
>   if (entry && entry->mCreateHandlerSucceeded)
>     ActivatePopup(entry, PR_FALSE);
>+
>+  if (entry->mElementContent && entry->mPopupType == NS_LITERAL_STRING("context")) {
>+    // If we are a context menu, and if we are attached to a menupopup, then hiding us
>+    // should also hide the parent menu popup.
>+    nsCOMPtr<nsIAtom> tag;
>+    entry->mElementContent->GetTag(*getter_AddRefs(tag));
>+    if (tag && tag.get() == nsXULAtoms::menupopup) {
>+      nsIFrame* popupFrame = nsnull;
>+      nsCOMPtr<nsIPresShell> presShell;
>+      mPresContext->GetShell(getter_AddRefs(presShell));
>+      presShell->GetPrimaryFrameFor(entry->mElementContent, &popupFrame);
>+      if (popupFrame) {
>+        nsCOMPtr<nsIMenuParent> menuParent(do_QueryInterface(popupFrame));
>+        if (menuParent)
>+          menuParent->HideChain();
>+      }
>+    }
>+  }
>+

OFFTOPIC: Don't you love the pres-context/pres-shell separation?  Where's
bryner's patch to fuse them?

Clean up unused variables, remove printfs, and r=brendan.


/be

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 101613 [details] [diff] [review]
Patch to support context menus on menupopups

Hyatt assures me he's removing printfs and stuff.

/be

Comment 8

[David Hyatt]

Fixed.

Comment 9

[[e-mail]]

How is this related to bug 64324 and bug 50504 ?

Comment 10

[Pierre Chanial]

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

Comment 11

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

this might have caused a crasher.  see bug 173590

Comment 12

[Brendan Eich [:brendan]]

sspitzer: sorry, obvious oversight on hyatt's, hewitt's, and my parts -- the
pointer can be null, then the new code is suddenly dereferencing it?  Bleh. 
I'll try to do better super-review next time.

/be

Comment 13

[[e-mail]]

Um, how woul a XUL programmer *use* that feature? Is there a doc anywhere?

Comment 14

[Boris Zbarsky [:bzbarsky]]

This caused bug 180329 -- "make debugging anything involving context menus
unbearable"

Comment 15

[Boris Zbarsky [:bzbarsky]]

I have to wonder whether this code ever worked.... the GetRootViewForPopup()
call passes in the wrong prescontext, so it will never get a view...