Closed Bug 212750 Opened 21 years ago Closed 16 years ago

Context-menu key and Shift-F10 do not work as expected in Bookmark menu


(Core :: XUL, defect, P3)






(Reporter: avbohemen, Assigned: enndeakin)



(Keywords: access)


(1 file, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030714 Mozilla Firebird/0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030714 Mozilla Firebird/0.6

Pressing the context menu key on a Windows keyboard (the key between the right
control-key and the right windows-key) or the equivalent when not using a
windows keyboard (shift-F10) do not open the bookmarks context menu, as
right-clicking a bookmark does.

Reproducible: Always

Steps to Reproduce:
1. Open the bookmarks menu and highlight a bookmark, either by pressing alt-b
and key-down buttons or by mouse. Do not left-click the bookmark or press enter.
2. Press the context-menu button on a windows keyboard, or shift-F10.

Actual Results:  
Pressing the context menu button brings up the context menu for the currently
opened web page (containing back, forward, reload etc).

Pressing shift-F10 closes the bookmarks menu.

Expected Results:  
The context menu for the bookmarks (open, open in new window, etc) should pop
up, in the same way as when you right-click the bookmark.

Shift-F10 and the Context-menu key on a windows keyboard have the same function
in any windows program, they open a context menu (if available). Firebird does
not handle these keystrokes right.
Confirming with 20030714 build on W2K
Ever confirmed: true
Additional, these keys do not work at all in the bookmarks sidebar (nothing
happens when you press the key). In other sidebars, they work fine.
Taking QA Contact as designated owner of Firebird-Menus. Sorry for bugspam.
QA Contact: asa → bugzilla
Keywords: access
I've been trying to use Firefox on a laptop as well as my desktop machines, and
keyboard navigation issues like this are really noticable.
This 'Works For Me' in 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a6) Gecko/20041204
can anyone confirm as fixed?
I will open another bug about the location of the context menu.
The bug has not changed at all over here (Mozilla/5.0 (Windows; U; Windows NT
5.1; en-US; rv:1.8a6) Gecko/20041207 Firefox/1.0+). 

The context menu is still the one for the currently open web page, not for the
bookmark. Shift-F10 still closes the menu. The location of the menu is less
important: the wrong menu is opened in the first place.
Context-menu key and Shift-F10 are crucial. Use oncontextmenu to get it right.

"9. Context menus must support the keyboard, via the VK_APPS key Microsoft
context menu key  and Shift+F10 on Windows, Shift+F10 on UNIX/Linux and
Control+Space on OS X. This should be automatically handled by oncontextmenu —
don't handle the right click directly."
Keywords: helpwanted
Priority: -- → P1
Flags: blocking-aviary1.1?
*** Bug 256073 has been marked as a duplicate of this bug. ***
Works in manage bookmarks, so marking P3
Priority: P1 → P3
When I press Shift-F10 with focus on a link and when the page is scrolled down a
bit, the Context Menu appears at the wrong position: too low, sometimes even
outside the Firefox window.
Flags: blocking-aviary1.1? → blocking-aviary1.1-
*** Bug 288249 has been marked as a duplicate of this bug. ***
Assignee: firefox → jruderman
QA Contact: bugzilla → menus
I'm not sure how to fix this.  I think the problem is that menus don't get focus
when they're visible, and nsEventListenerManager::FixContextMenuEvent assumes
the focused element should be the target of the context menu.  Do I want to make
nsMenuListener be an nsIDOMContextMenuListener, prevent the normal context menu
from appearing, and make a context menu appear for the correct element and in
the correct place from there?  Or do I want to teach
nsEventListenerManager::FixContextMenuEvent how to behave properly when menus
are visible?
*** Bug 318785 has been marked as a duplicate of this bug. ***
*** Bug 345860 has been marked as a duplicate of this bug. ***
Assignee: jruderman → nobody
Looks vaguely like something the XUL popup manager could be handling - if a popup is open then capture context menu events and open the context menu for the active menuitem.
Sounds about right. Shouldn't be too hard to implement.
Attached patch Patch rev. 1 (obsolete) — Splinter Review
Question: I'm not sure if "ISUPPORTS5" is enough here since
nsIDOMEventListener is inherited from both nsIDOMKeyListener and
nsIDOMContextMenuListener now.  Do I need to tell cycle collection
about that?  I saw that nsXULPopupListener have something explicit:

Note: silently discarding the event for disabled items is intentional.
Currently (on Windows only) when you right-click a disabled item we
present a context menu for the Bookmarks menu itself - I consider this
to be a bug, we should not open a context menu in this case either.
Assignee: nobody → mats.palmgren
Attachment #306426 - Flags: superreview?(enndeakin)
Attachment #306426 - Flags: review?(enndeakin)
Keywords: helpwanted
OS: Windows 2000 → All
Assignee: mats.palmgren → nobody
Component: Menus → XP Toolkit/Widgets: Menus
Product: Firefox → Core
QA Contact: menus → xptoolkit.menus
Assignee: nobody → mats.palmgren
(In reply to comment #20)
> Created an attachment (id=306426) [details]

I'd think it would be better and use less code to do this in nsEventListenerManager::FixContextMenuEvent and just check if a popup is open, and if so, just use the current menuitem as the target instead of the focused item.
I tried doing that but it didn't work, possibly because the menuitem in
this case has a different pres context.  The stack is:
#0  nsEventListenerManager::FixContextMenuEvent (this, aPresContext ...
#1  in nsEventListenerManager::HandleEvent (this, aPresContext ...
#2  in nsEventTargetChainItem::HandleEvent (this, aVisitor ...
#3  in nsEventTargetChainItem::HandleEventTargetChain (this, aVisitor ...
#4  in nsEventDispatcher::Dispatch (aTarget, aPresContext ...
(aVisitor has a mPresContext member).

Even if we can make it work somehow, we would still have to add most
of nsXULPopupManager::ContextMenu() in there and Boris' comment:
indicates adding yet another MOZ_XUL block in there isn't a good idea...

Regarding the current patch: I regret adding that block checking
if the item is disabled; I think it's valid for disabled items to
have a context menu (eg Delete) if they choose to.  This is something
the application layer should decide, we shouldn't prohibit it here.
(the bug I described in my last comment really is bug 416662)
Nominating as blocking, as this is really annoying for keyboard users.
Flags: blocking1.9?
Aw... I'm a convinced keyboard user, but this? Blocking? Naahh... Not with a normal severity, I think. But I'm not sure how this flagging thing is really supposed to work, so I just leave my 2 cents here in the comments.
(In reply to comment #25)
> Aw... I'm a convinced keyboard user, but this? Blocking? Naahh... Not with a
> normal severity, I think. But I'm not sure how this flagging thing is really
> supposed to work, so I just leave my 2 cents here in the comments.

The problem is that this appears right in front of users navigating the Web and the actual software with keyboard. Not fixing this means that you have to open Library and navigate it (which is more difficult that just using the context menu).

Plus, the context menu is a already established interaction form that we should care not to break.

IMHO, keyboard and mouse navigation should be given the same priorities.
It ins't blocking no. It would be nice to have, but is not a regression and the functionality is available elsewhere.
-'ing based on comment #27.  I agree.
Flags: blocking1.9? → blocking1.9-
It's important, yes. But I don't regard it as blocking as well. Is as important as if the mouse functions wouldn't work. Nothing more, nothing less. m2c
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.menus → xptoolkit.widgets
Attached patch move context menu handling (obsolete) — Splinter Review
Assignee: mats.palmgren → enndeakin
This patch does two things for keyboard-activated context menus:
 - it checks for menus being opened and then looks for the active menu
 - moves the context menu handling out of the event listener manager. This means we can properly set the target so that the context menu events fire on the same targets as they would for mouse-activated context menus.

I don't think the PresShell is the right place to put this stuff, perhaps a utility class somewhere.

Smaug, any ideas where a better place for this would be?
Attachment #306426 - Flags: superreview?(enndeakin)
Attachment #306426 - Flags: review?(enndeakin)
Great to remove those contextmenu related methods from ELM!
I can't really think of any better place than PresShell for this.
(It would be easier to read the patch if you used -U 8 -p)
Attached patch updated patch with more context (obsolete) — Splinter Review
Attachment #306426 - Attachment is obsolete: true
Attachment #339460 - Attachment is obsolete: true
Attachment #340140 - Flags: review?(Olli.Pettay)
Comment on attachment 340140 [details] [diff] [review]
updated patch with more context

>+PresShell::AdjustContextMenuKeyEvent(nsEvent* aEvent)
>+#ifdef MOZ_XUL
>+  // if a menu is open, open the context menu relative to the active item on the menu.
>+  // XXXndeakin Mac doesn't fire mouse-triggered context menus while another
>+  // menu is open. Maybe we should prevent keyboard-tiggered context menu events too. 
Whatever is the normal behavior on OSX, IMO.

>+  nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
>+  if (pm) {
>+    // XXXndeakin should use GetTopVisibleMenu instead?
Could you explain this. What is the difference in practice?

>+    nsIFrame* popupFrame = pm->GetTopPopup(ePopupTypeMenu);
>+    if (popupFrame) {
>+      nsIFrame* itemFrame = 
>+        (static_cast<nsMenuPopupFrame *>(popupFrame))->GetCurrentMenuItem();
>+      if (!itemFrame)
>+        itemFrame = popupFrame;
>+      nsCOMPtr<nsIWidget> widget = itemFrame->GetWindow();
>+      (static_cast<nsMouseEvent*>(aEvent))->widget = widget;
>+      nsRect widgetRect(0, 0, 1, 1);
>+      widget->WidgetToScreen(widgetRect, widgetRect);
>+      aEvent->refPoint = itemFrame->GetScreenRect().BottomLeft() - widgetRect.TopLeft();
>+      mCurrentEventContent = itemFrame->GetContent();
>+      mCurrentEventFrame = itemFrame;
>+      return NS_OK;
Please add some tests to check that event.clientX/Y and .screenX/Y  contain reasonable values in this case.

Apparently other parts of the patch essentially just moves code from ELM to PresShell (few changes 
needed to get presshell/viewmanager).

Looks ok.
Attachment #340140 - Flags: review?(Olli.Pettay) → review+
- made it so that keyboard-invoked contextmenu events do not occur on Mac while another menu is open, as mouse-invoked ones never fire.
- removed the XXX comment
- improved the test to check for client/screen positions in more cases
Attachment #340140 - Attachment is obsolete: true
Attachment #344298 - Flags: superreview?(roc)
Attachment #344298 - Flags: review?(Olli.Pettay)
Attachment #344298 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 344298 [details] [diff] [review]
address issues, include better test

r=me, if you have tested that context menu key works properly also when UI is zoomed.
I guess the easiest way to check that is to load browser chrome (chrome://browser/content/browser.xul) in a tab and zoom in or out and test bookmarks menu of that browser instance.
Comment on attachment 344298 [details] [diff] [review]
address issues, include better test

+  PRBool AdjustContextMenuKeyEvent(nsEvent* aEvent);

Make this an nsMouseEvent* so all casting is in the caller.

+  PRBool PrepareToUseCaretPosition(nsIWidget* aEventWidget, nsPoint& aTargetPt);


+                                           nsPoint& aTargetPt);

nsIntPoint, and return it as the function result instead of an out param
Attachment #344298 - Flags: superreview?(roc) → superreview+
(In reply to comment #37)
> nsIntPoint, and return it as the function result instead of an out param

What would you expect to have returned on error, or when the caret isn't visible?
Closed: 16 years ago
Resolution: --- → FIXED
I had disabled a test here, but have now fixed it as part of bug 487631.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.