887695 - WM_KEYDOWN and WM_SYSKEYDOWN should be handled before IME handles them

Status: RESOLVED WONTFIX

(Core :: Widget: Win32, defect, P2)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Currently, both WM_KEYDOWN and WM_SYSKEYDOWN are passed to IME before we handle them. Therefore, if IME consumes the key messages, we don't dispatch NS_KEY_DOWN event. This is different behavior from Gecko for Mac. And also, we need to fix this for fixing bug 354358.

I think that there are two approach for this bug.

1. Don't call TranslateMessage() before a call of DispatchMessage(). Then, the keydown message handler in nsWindow calls TranslateMessage() after dispatching NS_KEY_DOWN.

2. Before a call of TranslateMessage() in the message loop, call the message handler of the window directly. Then, ignore the messages via DispatchMessage().

I feel #1 is better. However, I don't know if it's a right way because I've never see such message handling code.

Do you guys have any suggestions?

Comment 1

[Masatoshi Kimura [:emk]]

I think it's better to handle the message entirely before calling TranslateMessage().
MFC has PreTranslateMessage() method for this very purpose.
http://msdn.microsoft.com/ja-jp/library/cw1sb70c%28v=vs.80%29.aspx
IsDialogMessage() and TranslateAccelerator() will also be called before TranslateMessage().
And if the functions return nonzero (which will correspond to preventDefault() call), TranslateMessage() and DispatchMessage() will not be called.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you, Kimura-san! I'll try to do similar approach.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

It seems that CoreDispatcher::ProcessEvents() calls TranslateMessage() and passes key messages to TSF automatically. This blocks to fix this bug on Metrofox.

Jimm, I'd like to know your idea for this issue. If we don't use CoreDispatcher for key messages (WM_KEYFIRST - WM_KEYLAST), we can handle them in lower level. However, it might cause another bug since CoreDispatcher::ProcessEvents() might do something we don't know.

Do you think that not using CoreDispatcher for key messages is too risky? If so, we'd need to mark this bug as WONTFIX on Metrofox.

Although, we shouldn't decide so as far as possible for consistent behavior between platforms.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI: Currently, Chrome in MetroApp mode has same issue. I.e., web developer cannot prevent composition by preventing default of keydown event since keydown event isn't fired when TSF consumes native key message.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Remove WM_KEYDOWN redirect system on Windows

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 Check IME state in widget::NativeKey::Handle*Message()

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.3 Don't cache following WM_*CHAR message at widget::NativeKey being constructed

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.4 Separate widget::NativeKey::HandleKeyDownMessage() to HandleKeyDownMessageBeforeIME() and HandleKeyDownMessageAfterIME()

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.5 Dispatch keydown event before WM_KEYDOWN or WM_SYSKEYDOWN is sent to IME on Desktop

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, I forgot the part.1. It seems that we need to call at least HandleKeyDownMessageBeforeIME() should be called before CoreDispatcher::ProcessEvents()...

Comment 11

[Jim Mathies [:jimm]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #3)
> It seems that CoreDispatcher::ProcessEvents() calls TranslateMessage() and
> passes key messages to TSF automatically. This blocks to fix this bug on
> Metrofox.
> 
> Jimm, I'd like to know your idea for this issue. If we don't use
> CoreDispatcher for key messages (WM_KEYFIRST - WM_KEYLAST), we can handle
> them in lower level. However, it might cause another bug since
> CoreDispatcher::ProcessEvents() might do something we don't know.
> 
> Do you think that not using CoreDispatcher for key messages is too risky? If
> so, we'd need to mark this bug as WONTFIX on Metrofox.
> 
> Although, we shouldn't decide so as far as possible for consistent behavior
> between platforms.

We have no idea what is inside CoreDispatcher::ProcessEvents(). You can put together a patch and we can land it and look for side effects. My concern would be that metro specific keyboard events might not trigger appropriate ui reactions. I guess it depends on where they pick those up. Generally more control over event processing in metro is probably a good thing assuming there aren't any serious regressions.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thanks. I'll try to find the lowest risky way for Metrofox.

Comment 13

[Jim Mathies [:jimm]]

Looks like we are already doing this?

http://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroWidget.cpp#591

Comment 14

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #13)
> Looks like we are already doing this?
> 
> http://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/
> MetroWidget.cpp#591

Meaning, we already intercept these events before they get to the def wnd proc. So I'd say go ahead and give it a try.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

No. At that time, TSF may already consume the message or ::TranslateMessage() is already called. So, perhaps, we need to handle WM_KEYDOWN at MetroAppShell.

Comment 16

[Jim Mathies [:jimm]]

Is this still a valid bug?

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Jim Mathies [:jimm] from comment #16)
> Is this still a valid bug?

Yes, it is. At least in TSF mode, we need to do this because TSF consumes WM_KEYDOWN messages. Therefore, we cannot dispatch keydown events during composition and keydown event of a key starting a composition.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Current our behavior matches with Chrome so that changing our behavior (for consistency between platforms) may cause new web-compat issue. So, we should not do this.