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

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Anton van Bohemen]

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.

Comment 1

[Simon Paquet [:sipaq]]

Confirming with 20030714 build on W2K

Comment 2

[Anton van Bohemen]

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.

Comment 3

[Simon Paquet [:sipaq]]

Taking QA Contact as designated owner of Firebird-Menus. Sorry for bugspam.

Comment 4

[James Slaughter]

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.

Comment 5

[Gids Goldberg]

This 'Works For Me' in 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a6) Gecko/20041204
Firefox/1.0+
can anyone confirm as fixed?
I will open another bug about the location of the context menu.

Comment 6

[Anton van Bohemen]

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.

Comment 7

[Aaron Leventhal]

Context-menu key and Shift-F10 are crucial. Use oncontextmenu to get it right.

See http://www.mozilla.org/access/xul-guidelines
"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."

Comment 8

[Aaron Leventhal]

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

Comment 9

[Aaron Leventhal]

Works in manage bookmarks, so marking P3

Comment 10

[Onno Ekker [:nONoNonO UTC+1]]

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.

Comment 11

[Jesse Ruderman]

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

Comment 12

[Jesse Ruderman]

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?

Comment 13

[Kevin Brosnan]

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

Comment 14

[Aaron Leventhal]

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

Comment 17

[[e-mail]]

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.

Comment 18

[Neil Deakin]

Sounds about right. Shouldn't be too hard to implement.

Comment 20

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

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:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/xul/content/src/nsXULPopupListener.cpp&rev=1.154&root=/cvsroot&mark=99-107#98

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.

Comment 21

[Neil Deakin]

(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.

Comment 22

[Mats Palmgren (inactive)]

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:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/events/src/nsEventListenerManager.cpp&rev=1.294&root=/cvsroot&mark=71-73#71
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)

Comment 24

[Andrés Delfino]

Nominating as blocking, as this is really annoying for keyboard users.

Comment 25

[Onno Ekker [:nONoNonO UTC+1]]

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.

Comment 26

[Andrés Delfino]

(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.

Comment 27

[Neil Deakin]

It ins't blocking no. It would be nice to have, but is not a regression and the functionality is available elsewhere.

Comment 28

[Damon Sicore (:damons)]

-'ing based on comment #27.  I agree.

Comment 29

[Axel]

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

Comment 30

[Neil Deakin]

Attachment: move context menu handling

Comment 31

[Neil Deakin]

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?

Comment 32

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

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)

Comment 33

[Neil Deakin]

Attachment: updated patch with more context

Comment 34

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

Comment on attachment 340140 [details] [diff] [review]
updated patch with more context

>+nsresult
>+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.

Comment 35

[Neil Deakin]

Attachment: address issues, include better test

- 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

Comment 36

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

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 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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);

nsIntPoint

+                                           nsPoint& aTargetPt);

nsIntPoint, and return it as the function result instead of an out param

Comment 38

[Neil Deakin]

(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?

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Alright, scrub that last point

Comment 40

[Neil Deakin]

I had disabled a test here, but have now fixed it as part of bug 487631.