Closed Bug 17159 Opened 25 years ago Closed 25 years ago

right click doesn't work within context menus

Categories

(Core :: XUL, defect, P3)

Product:
Core
Component:
XUL
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: dbaron, Assigned: mikepinkerton)

References

Details

(Whiteboard: [PDT+]w/b minus on 03/03 - Fix in hand)

Attachments

(5 files)

Proposed fix. This allows right-clicks to execute menu commands. Not sure if this is the best way to do it...
768 bytes, patch
Details | Diff | Splinter Review
Bug fix - context menus can now accept right-clicks
3.12 KB, patch
Details | Diff | Splinter Review
implemented Hyatt's suggestions
5.25 KB, patch
Details | Diff | Splinter Review
Gee, I guess it would help if I implemented all of the functions.
5.08 KB, patch
Details | Diff | Splinter Review
Updated with Dave Hyatt's latest comments
6.12 KB, patch
Details | Diff | Splinter Review 
DESCRIPTION:  One should be able to use a right-click to select an item within a
context menu.  Right now I have to right-click to get the context menu and
left-click to select an item within it.  (Is this a regression?)

STEPS TO REPRODUCE:
 * launch apprunner
 * right click on the document
 * right click on something in the context menu

ACTUAL RESULTS:
 * nothing happens

EXPECTED RESULTS:
 * the item should be activated, just like it is for a left-click

DOES NOT WORK CORRECTLY ON:
 * Linux, apprunner, 1999-10-24-08-M11
Assignee: trudelle → saari
Target Milestone: M13
reproduced in today's opt bits, reassigning to saari as p3 for m13
mass-moving all m13 bugs to m14
QA Contact: claudius → elig
[Contextual menu bug --- QA Assigning to self.]
Component: XP Toolkit/Widgets → XPMenus
QA Contact: elig → sairuh
reassigned QA contact to me & updated component.
Status: NEW → ASSIGNED
Target Milestone: M14 → M15
*** Bug 21646 has been marked as a duplicate of this bug. ***
Assignee: saari → pinkerton
Status: ASSIGNED → NEW
Taking menu/popup bugs.
BULK MOVE: Changing component from XP Menus to XP Toolkit/Widgets: Menus.  XP 
Menus component will be deleted.
Component: XPMenus → XP Toolkit/Widgets: Menus
Blocks: 21549
With NN 4.7, at least on NT, the right mouse button can be used to activate
a context menu item in one of two ways:
1. Right-click to bring up the menu, right-click again to chose the item.
2. Press the right mouse button to bring up the menu, hold it down, move the 
   cursor to the desired item, let up the button to chose the item.

(1) makes this bug [4.xP]; (2) makes bug 21549, dependent on this bug, [4.xP].

Tested with: Netscape Navigator 4.7 on Windows NT.
Summary: right click doesn't work within context menus → [4.xP] right click doesn't work within context menus
No longer blocks: 21549
bulk accepting xpmenu/popup bugs. sigh.
Status: NEW → ASSIGNED
Is this related to bug 16766?
no, they're separate issues.

Setting the keyword all open [4.xp] bugs to 4xp.
Keywords: 4xp
*** Bug 25398 has been marked as a duplicate of this bug. ***
*** Bug 25398 has been marked as a duplicate of this bug. ***
Does the attachment also address bug <A 
HREF="http://bugzilla.mozilla.org/show_bug.cgi?id=16766">#16766</A>?  I can't 
seem to get things to compile properly (I finally took the leap and am 
attempting to get this working), or I'd check it myself.
OK, I got things up and compiling and it looks like the patch is close, but it
may have gone too far.  Now a right-click works on both context menus and
menu-bar menus (might want to save this for future features, eg. right-click on
bookmarks menu).  What's missing in the patch is a check to see if the current
menu is a context menu.  Not quite sure how to do this yet...

And no, it doesn't address Bug 16766, either.

ps. Sorry about the botched url in my last comment.  Some day I'll learn how to
enter these comments properly.

It looks to me like the reason that smorrison's patch works a little too well is 
because the isMenuBar flag will be true for any right-clicks on the menu unless 
the right click takes place on the actual menu bar.

I guess the question is, what's the proper way to tell if the current menu is a 
popup/context menu or not?  I was looking for / dreaming of an IsContextMenu() 
function, but wasn't really surprised when I couldn't find one.
hyatt knows, he will comment. we talked about this at denny's tonight.

This patch isn't quite right, as Dean points out, but hopefully I can give you 
enough guidelines to stab it out fully.

(1) Context menus are only created through createPopup over in 
layout/xul/base/src/nsPopupSetFrame.cpp.  The popup type ("context") is passed 
in. 

(2) When you create the popup (over in the createPopup method) set some flag on 
it that indicates that it is a context menu.  Do this if the popup type is 
("context").  You'll need to add a new method to nsMenuPopupFrame (or perhaps 
the nsIMenuParent interface; nsMenuBarFrame can simply ignore you).

(3) When submenu popups are created, they'll need to inherit the fact that 
they're context menus.

(4) Only process the right mouse down if the menu parent is a context menu.

I'd love to see a complete patch for this in M14.  Send me the patch in email 
when it's ready, and i'll be happy to review it and get it into the tree 
quickly.

Thanks for working on this!

Meant to say "right mouse up", not "right mouse down".

Well, hey, I'm a sucker for punishment.  This is along the lines of what I was 
considering doing before I realized that I have to get some sleep between now 
and four hours from now when work starts.  If nobody minds waiting a few 
days for this, I'll try and take a shot at this by the end of the weekend.
I was trucking merrily away on this (bah! who needs sleep?) and got down to 
compile time when I ran into problems.  Steps 1, 3, and 4 should be no 
problem.  But maybe it's the lack of sleep, or the fact that this is my first 
couple hours of looking at the menu code, because I'm missing the link between 
nsPopupSetFrame and nsMenuPopupFrame (and thus nsIMenuParent).  To me, it looks 
like there isn't a direct one.  Which is going to make step 2 a little tricky.  
Is my mind just too fragged right now to be thinking about this?
In case anyone was wondering it was the lack of sleep, although I haven't slept 
much since then.  I've got this working right now, and am trying to get a patch 
together that I can mail to Hyatt.  But I'm still really green with cvs so this 
may take a little while.
(Hmmm... this comment got lost along the way.)

I just realized that I didn't confirm that this works for sub-menus in context 
menus.  Any attempts that I made to hack the xul to create this, however, failed 
pitifully.  Can anyone whip up a test-case navigator.xul that implements a 
cascading context menu?
Great!  This is nearly ready to go.  Here are the things that still need to be 
fixed.

(1) nsIMenuParent is an XPCOM interface, and that means that we don't want it to 
contain anything other than pure virtual functions.  That means no member 
variables.  You need to make Get/SetAllowRightClick pure virtual functions on 
nsIMenuParent, and then you need to implement those functions in nsMenuBarFrame 
and nsMenuPopupFrame.  In nsMenuBarFrame, they'd simply do nothing.  In 
nsMenuPopupFrame, add the member variable and define the functions just as you 
did in nsIMenuParent.

(2) I'd rather you used Set/GetIsContextMenu rather than "AllowRightClick".  I 
suspect we'll be using this in more than one place, and so I don't think the 
methods should be so specifically named.

Remove the "fix for bug #17159" comments.  This is just basic functionality, and 
we don't need to include that.

Thanks for doing this!  This is really great.  If you have any other menu bugs 
you'd like to tackle, I would be happy to give you more roadmaps for those bugs 
(I know how to fix them all, so I can continue to give instructions).
  
Also, make sure to initialize the member variable to PR_FALSE in the 
nsMenuPopupFrame constructor.

These two shouldn't be a problem.  Just to confirm, though, will I have to make 
changes to nsMenuBarFrame or can I just leave it without any knowledge of the 
new functions?

Also, I started with "IsContextMenu" but moved to "AllowRightClick" because I 
was thinking that in the future we will probably allow such actions as 
right-clicking on bookmarks in the menu to get a context menu.  In this case the 
bookmarks menu wouldn't be a context menu, but it would allow/accept the right 
click.
If it inherits from nsIMenuParent, you will have to change nsMenuBarFrame when 
adding the two pure virtual methods. that's just c++.

Regarding your 2nd point, stick with IsContextMenu() for now and if we ever need 
that, we can always add another method that just returns the same value, but with 
the appropriate name.

Thanks so much for your help on this.

awesome, i'll try to get this in today or this weekend.

"awesome, i'll try to get this in today or this weekend."

Ummm.... it's not done yet.  I haven't implemented Hyatt's suggestions yet.
oops, i commented on the wrong bug ;)

OK, I've attached a patch with Hyatt's suggestions implemented.  I clobbered the 
layout and rebuilt it and everything worked properly.  The only thing I'm not 
sure of is how I implemented the GetIsContextMenu() and SetIsContextMenu() 
functions in nsMenuBarFrame.h.
Crap, I lied.  The build didn't go.  I'll have a new patch soon.  Anyone want to 
give a newbie C++ guy a hint as to how to resolve the error:

raptorxulbase_s.lib(nsMenuPopupFrame.obj) : error LNK2001: unresolved external s
ymbol "public: virtual unsigned int __stdcall nsMenuPopupFrame::GetIsContextMenu
(int &)" (?GetIsContextMenu@nsMenuPopupFrame@@UAGIAAH@Z)
Summary: [4.xP] right click doesn't work within context menus → right click doesn't work within context menus
Odd.  I was doing my work using the M13 tarball and it worked fine.  Now when I 
pull a fresh tree and apply my patch it fails.  No idea why yet, but I'm 
looking...
Aw crap, helps if I do a clobber_layout first when I'm modifying interfaces et 
al.  I've got a couple more comments from Dave and then I'll post hopefully the 
last patch.
Keywords: patch
dean, i want to get this in, but we're getting lots of shit from management about 
doing things not marked PDT+ for M14, and this bug ain't. Don't fret, we love 
this patch and we want to get it in, just be patient with us.

I was beginning to think that you had forgotten me ( * sniff * ).Just jokin'.  
No big rush for me, because I already have the patch installed, but I know of 
mroe than a few people that would be really happy to get this fix.
Nominating for beta1. Fix in hand from external developer. Very well understood, 
very localized. Tested on mac, win32, linux (builds and works). Patch has 
undergone several iterations with me and hyatt and the external contributor.

Not being able to use the right mouse button to select an item in a context menu 
is a big usability black eye. It's just one of those things that will make people 
swear at us and wonder what we've been wasting all of aol's money on all this 
time.
Keywords: beta1
Whiteboard: Fix in hand
Putting on PDT+ radar for beta1.  But will move to PDT- if not fixed by 03/03 or 
regression occurs.
Whiteboard: Fix in hand → [PDT+]w/b minus on 03/03 - Fix in hand
whoo hoo! fixed!
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
Sorry to be repetitive here, but woo-hoo!
[Win32] Now that I look at this again (it's been a while), I notice that when I 
right-click a menu item, the browser window becomes inactive for a split second.  
This doesn't happen when I left-click an item.  Minor, I know, but still Ugh.  
Should this be a new bug?
After some further investigation, what I just mentioned is unrelated to this, so
I opened a separate bug (bug 29851).

verif w/this morning's opt bits [2000.03.01] on linux [comm], winNT [comm] and
mac [mozilla].
Status: RESOLVED → VERIFIED
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: bugzilla → xptoolkit.widgets
right click sometimes not working.. (opens right click menu once every 4~5 clicks) 
mac 10.11.6 
firefox 59.0.1 (64-bit)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: