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: