183841 - [gtk2] Shortcut for "Open Context Menu" doesn't work, shift-F10

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Jessie Li]

Build:Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.2) Gecko/20021204
OS: SunOS volleyball 5.9 Generic sun4u sparc SUNW,Sun-Blade-100

According to the mozilla keyboard plan, Shift+F10 is the general shortcut for
opening context menu. But it doesn't work for Solaris OS.

Comment 1

[Aaron Leventhal]

Can someone from browser-china-atf take this?

Comment 2

[Bolian Yin]

ok, let me try.

Comment 3

[Kyle Yuan]

works for gtk1.2 build. =>gtk2

Comment 4

[sairuh (rarely reading bugmail)]

hmmm, this is interesting: shift-F10 doesn't seem to work for me on Linux RH8.
i'm using the metacity window manager --it's a new wm (and machine and OS, for
that matter ;), so i'm not sure if there's some setting i'm neglecting.

bryner or akkana, are you able to get shift-F10 to bring up the context menu for
a focused item in a web page in RH8?

Comment 5

[sairuh (rarely reading bugmail)]

erk, scratch that. somehow the shortcut is now working for me. thought i had an
item already in focus *mumble*grumble*...

Comment 6

[Bolian Yin]

Attachment: patch

deal with the context menu key.

Comment 7

[Bolian Yin]

Attachment: patch against trunk

Comment 8

[Christopher Blizzard (:blizzard)]

Comment on attachment 108462 [details] [diff] [review]
patch against trunk

>+inline PRBool
>+nsWindow::IsContextMenuKey(const nsKeyEvent& aKeyEvent)
>+{
>+    enum { kContextMenuKey = NS_VK_F10 };

You don't need to make this an enum, just compare below.

>+
>+    return (aKeyEvent.keyCode == kContextMenuKey && aKeyEvent.isShift &&
>+            !aKeyEvent.isControl && !aKeyEvent.isMeta && !aKeyEvent.isAlt);
>+}

>+void
>+nsWindow::ConvertKeyEventToContextMenuEvent(const nsKeyEvent* aKeyEvent,
>+                                            nsMouseEvent* aCMEvent)
>+{
>+    *(nsInputEvent *)aCMEvent = *(nsInputEvent *)aKeyEvent;

Two things: we should fix signature on the function to use nsInputEvent instead
of casting here.  Two, can we use memcpy instead of copying the structure? 
It's a little more readable.

>Index: nsWindow.h
>===================================================================
>RCS file: /cvsroot/mozilla/widget/src/gtk2/nsWindow.h,v
>retrieving revision 1.30
>diff -u -r1.30 nsWindow.h
>--- nsWindow.h	16 Nov 2002 01:22:10 -0000	1.30
>+++ nsWindow.h	6 Dec 2002 08:26:49 -0000
>@@ -311,6 +311,10 @@
>     static guint DragMotionTimerCallback (gpointer aClosure);
>     static void  DragLeaveTimerCallback  (nsITimer *aTimer, void *aClosure);
> 
>+    inline PRBool IsContextMenuKey(const nsKeyEvent& inKeyEvent);
>+    void ConvertKeyEventToContextMenuEvent(const nsKeyEvent* inKeyEvent,
>+                                           nsMouseEvent* outCMEvent);

These don't change any class specific data and don't need to be used outside of
the class so you should just make them utility functions in nsWindow.cpp. 
There are examples.

Comment 9

[Bolian Yin]

Attachment: patch (add blizzard's comments)

Did not change ConvertKeyEventToContextMenuEvent prototype to nsInputEvent, we
need to assign values of nsMouseEvent.

Comment 10

[Christopher Blizzard (:blizzard)]

Comment on attachment 109206 [details] [diff] [review]
patch (add blizzard's comments)

>+static PRBool IsContextMenuKey(const nsKeyEvent& inKeyEvent);
>+static void ConvertKeyEventToContextMenuEvent(const nsKeyEvent* inKeyEvent,
>+                                              nsMouseEvent* outCMEvent);

Sorry, one more comment.  Please use the same style for the function names as
the statics functions declared above it.  That is, use a function name like:

is_context_menu_key() instead of IsContextMenuKey().  That way we can tell
static functions from methods which usually use StudlyCaps().

Comment 11

[Bolian Yin]

Attachment: patch(utility function name to lower case)

Comment 12

[Christopher Blizzard (:blizzard)]

Comment on attachment 109296 [details] [diff] [review]
patch(utility function name to lower case)

sr=blizzard

Comment 13

[Christopher Blizzard (:blizzard)]

Checked in.  Thanks!

Comment 14

[sairuh (rarely reading bugmail)]

where would gtk2 verification builds reside? i checked the following location,
but it's empty.

ftp://ftp.mozilla.org/pub/mozilla/nightly/latest/Red_Hat_8x_RPMS/

Comment 15

[sairuh (rarely reading bugmail)]

no builds, no info -> rs vrfy.

Comment 16

[Tom Schuster (MoCo)]

Attachment: WIP: Bug 183841 - Use RFPTarget::ScreenOrientation for the orientation listener

Depends on D180955

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Wrong bug?

Comment 19

[Tom Schuster (MoCo)]

Thanks Emilio, I missed a digit in the bug# while writing the commit message. Sadly bugzilla/phabricator seem to be a bit unreliable right now, so the patch hasn't been moved yet.

Comment 20

[Phabricator Automation]

Comment on attachment 9339097 [details]
WIP: Bug 183841 - Use RFPTarget::ScreenOrientation for the orientation listener

Revision D180956 was moved to bug 1838415. Setting attachment 9339097 [details] to obsolete.