Last Comment Bug 665406 - Support XF86Copy, XF86Paste, XF86Cut, XF86Undo, XF86Redo keysym
: Support XF86Copy, XF86Paste, XF86Cut, XF86Undo, XF86Redo keysym
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Gtk (show other bugs)
: unspecified
: x86 Solaris
: -- normal (vote)
: mozilla8
Assigned To: Ginn Chen
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-19 19:37 PDT by Ginn Chen
Modified: 2011-07-13 19:02 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
draft patch (20.78 KB, patch)
2011-06-19 20:08 PDT, Ginn Chen
roc: review+
Details | Diff | Review
patch v2 (22.24 KB, patch)
2011-06-23 20:13 PDT, Ginn Chen
masayuki: review+
bugs: review+
Details | Diff | Review

Description Ginn Chen 2011-06-19 19:37:10 PDT
With new Xorg server, Copy, Paste, Cut, Undo, Again on Sun keyboard or other multimedia keyboard return XF86Copy, XF86Paste, XF86Cut, XF86Undo, XF86Redo keysym.
GDK_Copy, GDK_Paste ... for GDK.
Qt::Key_Copy, Qt::Key_Paste ... for Qt.

We should stop using the "strange" F16, F18, F20, F14 keycode.

For backwards compatibility, we can map F16 to Copy, F20 to Cut, etc.
We do not map F12 to Redo because it may confuse PC keyboard users.
Comment 1 Ginn Chen 2011-06-19 20:08:33 PDT
Created attachment 540381 [details] [diff] [review]
draft patch

I removed "#ifdef SOLARIS" for old Sun keysym in nsGtkKeyUtils.cpp in case someone remote Firefox on Linux to Solaris Xserver.
I hope it is OK, I think in most cases F14-F20, SunF36, SunF37 are not used.

I removed Sun keycode conversion in nsQtKeyUtils.cpp.
I think X keysym should not be used here.
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-19 20:35:38 PDT
Comment on attachment 540381 [details] [diff] [review]
draft patch

Review of attachment 540381 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-19 21:08:53 PDT
Um... Even if we would take this patch, please use from 0xC1 to 0xCF for the new keycodes because this breaks the patch for bug 630810 and bug 630813.

On the other hand, I'm not sure these keys should cause keydown/keyup events.

Sine such virtual keys are not on Win and Mac, Web application developers can miss to handle of the keys, probably.

I think the better thing is that we should dispatch only content command events at handling the keys. How about you, guys?
http://mxr.mozilla.org/mozilla-central/source/widget/public/nsGUIEvent.h#477

Note that we don't dispatch key events on Windows for special keys on multimedia keyboard.

And Karlt, key handling bug fix like this bug could break bug 630813's patch because it redesigns the structure of key event handling. If I recreate such big patch several times, I could make mistake at merging, so, I'd like you to review it ASAP.
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-19 21:21:21 PDT
> Note that we don't dispatch key events on Windows for special keys on multimedia keyboard.

Ah, my MS multimedia keyboard emulates the general shortcut keys such as Ctrl+C, Ctrl+Z when I press such special keys. But I still don't think we should dispatch the key events with new keycodes.
Comment 5 Ginn Chen 2011-06-19 23:56:49 PDT
I don't really want to make it so complicate.
If it doesn't work for some web app, it's OK.

Probably we can use VK_XF86COPY as the name to make it clear the keycode is only used under X environment.

What do you think?
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-20 00:23:46 PDT
(In reply to comment #5)
> I don't really want to make it so complicate.

Why my idea makes it complicated? I said that I think that we should just dispatch content command event instead of key events and handling them on XUL level. The Windows' implementation is here:
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#5390

> If it doesn't work for some web app, it's OK.

Why OK? We are developing web application platform. And the different behavior of web applications makes our users unhappy. The compatibility between platforms is very important. I'm working on getting rid of the difference of key event behavior between platforms.

> Probably we can use VK_XF86COPY as the name to make it clear the keycode is
> only used under X environment.

The name isn't matter. And we shouldn't make the difference keycode for every platform's special keycode.

I worry about the count of keycode will reach 256. Then, we cannot use bit field for saving key state by PRUint32[8] (I'm not sure we're using this way actually on current trunk build).

And note that I don't think we should define new keycode for all keys after we implement DOM3 key property. I planed to implement it after I finish current keycode refactoring.
Comment 7 Ginn Chen 2011-06-23 20:13:22 PDT
Created attachment 541586 [details] [diff] [review]
patch v2

Thanks for your advice, here's patch v2.

There's a small different between NS_CONTENT_COMMAND_PASTE and access key.
NS_CONTENT_COMMAND_PASTE will test if clipboard is usable first.
Comment 8 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-23 21:21:24 PDT
Comment on attachment 541586 [details] [diff] [review]
patch v2

I like this better than the old patch. By this approach, we can avoid dispatching special key events which are dispatched only on Linux.

But I should confirm smaug's thought.
Comment 9 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-06-28 07:42:11 PDT
Comment on attachment 541586 [details] [diff] [review]
patch v2

I wasn't aware of ContentCommandEvent, but after reading that code
this looks ok.
Comment 10 Ginn Chen 2011-07-13 19:02:00 PDT
http://hg.mozilla.org/mozilla-central/rev/8644336b5d40

I have to reverse a little bit to use some XF86XK_ marcos, because the build server doesn't have a recent GTK+.

Note You need to log in before you can comment on or make changes to this bug.