Closed Bug 865561 Opened 7 years ago Closed 5 years ago

When WM_APPCOMMAND is caused by a key press, the handler should dispatch key event with D3E's KeyboardEvent.key

Categories

(Core :: Widget: Win32, defect)

x86_64
Windows 8
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(1 file)

WM_APPCOMMAND may be caused by key press, then, we should dispatch key event with D3E KeyboardEvent.key (only when the value isn't "Unidentified").
Summary: When WM_APPCOMMAND is caused with key, the handler should dispatch key event with D3E's KeyboardEvent.key → When WM_APPCOMMAND is caused by a key press, the handler should dispatch key event with D3E's KeyboardEvent.key
Blocks: 900372
Status: NEW → ASSIGNED
No longer depends on: 842927
As I commented in NativeKey::HandleAppCommandMessage(), Windows sends WM_APPCOMMAND to the foreground window first. Then, if it is caused by a keypress and the key can be mapped to a virtual keycode, the DefaultWndProc posts WM_KEYDOWN and WM_KEYUP.

I believe that WM_APPCOMMAND's action should be a default action of its keydown event. Therefore, this patch dispatches a keydown event before handling the command. Then, if the keydown event is not consumed, the command is performed by our chrome. Then, if WM_KEYUP won't come, the method dispatches a keyup event. Otherwise, we should wait following WM_KEYUP event.

This patch makes the WM_APPCOMMAND handler completely move to NativeKey class. This may be odd if WM_APPCOMMAND isn't caused by a key press (e.g., mouse button press). However, calling a part of handler from NativeKey is complicated. And also I don't want to duplicate the code. Therefore, I take this approach.
Attachment #8446451 - Flags: review?(jmathies)
Attachment #8446451 - Flags: review?(bugs)
Comment on attachment 8446451 [details] [diff] [review]
Dispatch key events when WM_APPCOMMAND is fired for a keypress

Review of attachment 8446451 [details] [diff] [review]:
-----------------------------------------------------------------

looks flawless to me, nice code cleanup too.
Attachment #8446451 - Flags: review?(jmathies) → review+
Comment on attachment 8446451 [details] [diff] [review]
Dispatch key events when WM_APPCOMMAND is fired for a keypress

So I assume it isn't possible to get first appcommand for X, then
some random key events, and then some key events which have the
same virtualkeycode as what X had?

Also, what happens if one presses for example browser_forward button for
long time? Do we get multiple command events or multiple key events or both
or just one commant + keydown/up ?

But guess this is ok, though it would be perhaps nice to make
FF to use those key events, and not use Gecko-only AppCommand DOM event.
Attachment #8446451 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 8446451 [details] [diff] [review]
> Dispatch key events when WM_APPCOMMAND is fired for a keypress
> 
> So I assume it isn't possible to get first appcommand for X, then
> some random key events, and then some key events which have the
> same virtualkeycode as what X had?

First, Windows (OS) handles raw keyboard event, and then, sends WM_APPCOMMAND message to the focused window. This is the first time when we can know a multimedia key is pressed. Then, if we don't consume the message, the DefaultWndProc will post keydown message and keyup message (only when the key is released). Therefore, other messages might be inserted between WM_APPCOMMAND and WM_KEYDOWN and WM_KEYUP.

> Also, what happens if one presses for example browser_forward button for
> long time? Do we get multiple command events or multiple key events or both
> or just one commant + keydown/up ?

The former like this:

> <00001> 00081670 S WM_APPCOMMAND
> <00002> 00081670 R WM_APPCOMMAND
> <00003> 00081670 P WM_KEYDOWN nVirtKey:VK_BROWSER_BACK cRepeat:1 ScanCode:00 fExtended:1 fAltDown:0 fRepeat:0 fUp:0
> <00004> 00081670 S WM_APPCOMMAND
> <00005> 00081670 R WM_APPCOMMAND
> <00006> 00081670 P WM_KEYDOWN nVirtKey:VK_BROWSER_BACK cRepeat:1 ScanCode:00 fExtended:1 fAltDown:0 fRepeat:1 fUp:0
> <00007> 00081670 S WM_APPCOMMAND
> <00008> 00081670 R WM_APPCOMMAND
> <00009> 00081670 P WM_KEYDOWN nVirtKey:VK_BROWSER_BACK cRepeat:1 ScanCode:00 fExtended:1 fAltDown:0 fRepeat:1 fUp:0
> <00010> 00081670 S WM_APPCOMMAND
> <00011> 00081670 R WM_APPCOMMAND
> <00012> 00081670 P WM_KEYDOWN nVirtKey:VK_BROWSER_BACK cRepeat:1 ScanCode:00 fExtended:1 fAltDown:0 fRepeat:1 fUp:0
> <00013> 00081670 S WM_APPCOMMAND
> <00014> 00081670 R WM_APPCOMMAND
> <00015> 00081670 P WM_KEYDOWN nVirtKey:VK_BROWSER_BACK cRepeat:1 ScanCode:00 fExtended:1 fAltDown:0 fRepeat:1 fUp:0
> <00016> 00081670 P WM_KEYUP nVirtKey:VK_BROWSER_BACK cRepeat:1 ScanCode:00 fExtended:1 fAltDown:0 fRepeat:1 fUp:1
FYI: I merged a fix of bug 1091025 before landing.
Depends on: 1091025
Hello, you have a non-unified bustage from this patch. Can you please take a look?
(In reply to nigelbabu@gmail.com [:nigelb] from comment #7)
> Hello, you have a non-unified bustage from this patch. Can you please take a
> look?

Wow, sorry for the bustate. I start to check it... Wait a moment.
https://hg.mozilla.org/mozilla-central/rev/facf85b61c37
https://hg.mozilla.org/mozilla-central/rev/2f51262d2cab
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.