According to crash reports, we may find WM_CHAR whose wParam is 0 and scancode is 0xFF with a call of PeekMessage(PM_NOREMOVE) but we'll remove usual char message with a call of PeekMessage(PM_REMOVE).
In such case, we should ignore the found odd message and take the usual char message which was removed from the queue actually.
MozReview-Commit-ID: Gw8LvCXxul
Currently, we use alias NS_VK_* for WidgetKeyboardEvent::mKeyCode. Similarly, we should create alias enum for nsIDOMKeyEvent::DOM_KEY_LOCATION_*. Then, we can reduce the length and avoid to include nsIDOMKeyEvent in some cpp files.
MozReview-Commit-ID: 5cs4zQ061Lc
I think that when PeekMessage(PM_REMOVE) failed to remove a char message but next key message is still a char message, it may be possible that the odd keyboard layout or utility hook only PeekMessage(PM_NOREMOVE) and GetMessage(). If so, we can explain what occurs in this case.
I'm still not sure this fixes the case of bug 1336322 comment 0, but we should try to do this because I don't have better idea.
MozReview-Commit-ID: CxoO24n167t
This patch depends on bug 1336080.
When PeekMessage() fails to remove found char message, NativeKey::GetFollowingCharMessage() tries to check next key message in the queue again. Then, when next key message becomes non-char message, such as WM_KEYDOWN or WM_KEYUP, the char message must be removed by odd keyboard layout or something. Similarly, when next key message is a char message but it's caused by different key, the found char message must be removed by one of them too.
So, in these cases, NativeKey::GetFollowingCharMessage() should treat the key operation is already handled or canceled by the odd keyboard layout or somebody else. Additionally, in the latter case, following char message should be handled as orphan char message(s) as usual.
MozReview-Commit-ID: 8ahs8I0HUQ2
When NativeKey::GetFollowingCharMessage() tries to remove a char message from the queue, the message might be changed by odd keyboard layout or something. In such case, if the new char message is also caused by same physical key, the char message must be overwritten. Then, we should take the new char message instead.
Note that this patch saves original found char message into kFoundCharMsg and it's logged by each points for indicating if this case has occurred.
MozReview-Commit-ID: HAduq8sfwFt
NativeKey::GetFollowingCharMessage() may remove a char message which is different from previously found message in the queue because hacky keyboard layout or utility can overwrite the wParam when it's removed from the queue.
Now, we should assume that newer message, i.e., actually removed from the queue, is the expected message by the user. See bug 1336028 comment 0 for the actual scenarios which are collected by crash reports.
https://bugzilla.mozilla.org/show_bug.cgi?id=1336028#c0
MozReview-Commit-ID: 9ZgukHH1vfi
Currently, NativeKey::GetFollowingCharMessage() tries 5 times to remove found char message from the queue. It was enough when we found this issue at developing Metrofox.
However, this hack is not enough for some odd keyboard layouts because we see some crash reports which gives up to remove a char message from the queue because 5 WM_NULL messages are returned.
For preventing this crash, we should check if there is the message which is trying to remove from the queue when NativeKey receives WM_NULL. Then, when there is no key message in the queue or next key message becomes non-char message,, NativeKey should dispatch consumed keydown event because we can assume that the key operation may have already been handled or canceled. Otherwise, NativeKey should retry to remove the message again (until 50 times!, it's just enough big magic number, there is no concrete reason).
MozReview-Commit-ID: 1c6Y4OoQdrP
There are still a lot of crash due to failing to get following WM_CHAR message. Almost half of them are, we found a WM_CHAR message but it's gone at removing the message from the queue.
Although, we're still not sure what happen actually. It could be possible if somebody hooks PeekMessage() or something. Then, we can assume that the found message isn't necessary for the user because it must be removed by somebody when it became unnecessary or is handled.
This patch mark a new bool flag, mCharMessageHasGone, as true in such case. Then, NativeKey will dispatch keypress events without following char messages and mark the event as consumed.
MozReview-Commit-ID: mporX1sihC
When NativeKey crashes by itself, it means that we detect an impossible situation in usual environment. In such case, active 3rd party's keyboard layout or something other utility may hook API and returns odd result to us.
For investigating the cause and deciding what we should do in such cases, we should collect active keyboard layout name via crash reports.
MozReview-Commit-ID: HYRj24GwDHZ
I did my best to remove as much stuff as possible in this patch. The starting
point was to remove all the IsVistaOrLater() and IsWin7OrLater() calls, but I
also grepped for various strings and found some other removable stuff that way.
I may have still missed some things.
Notable things done by this patch.
- It removes numerous blocklist entries.
- It removes CanComputeVirtualKeyCodeFromScanCode(), because it's always true
now.
- It removes ShowXP{Folder,File}Picker(), even though these were available as
fallbacks on Vista+. The "when platform is built without the longhorn SDK"
condition in the comment above nsFilePicker::ShowXPFolderPicker() sounds like
it won't ever happen any more.
- It removes the config.trim_on_minimize preference. This requires adding a
bool sHaveInitializedPrefs variable; previously the lack of pref
initialization was indicated by the tri-state sTrimOnMinimize variable having
the value 2.
Notable things *not* done by this patch.
- ClearThemeRegion() still exists. The comment suggests that it is XP/Vista
only, but the code suggests otherwise. jimm thinks the comment is wrong.
- The comment in WinWakeLockListener::Callback() suggests that the StartTimer()
call is no longer needed to block the screen saver. I'm uncertain about this
and so I think it's best left to a follow-up.
When mWidget was already destroyed, anybody shouldn't dispatch WidgetEvent on it. Therefore, NativeKey::InitKeyEvent() has MOZ_CRASH() for detecting such dangerous bug and some users hit it.
Each message handler of NativeKey should check if mWidget has already gone.
Ideally, nsWindow shouldn't create NativeKey and try to handle the message with it. However, using NativeKey's message handlers can put some information to the log. Therefore, this patch doesn't touch nsWindow.
MozReview-Commit-ID: 4k5VfaKHPgG
Currently, KeyboardLayout doesn't support chained dead keys because probably, the initial developer didn't expect there are such keyboard layout. Additionally, if we'd try to handle them with KeyboardLayout, it'd need to create too big and too complicated table at loading such keyboard layout. It's really nightmare. Therefore, this patch takes different approach.
Currently, when WM_(SYS)KEYDOWN is received, KeyboardLayout (and NativeKey) respects following WM_(SYS)CHAR. Similarly, this patch makes KeyboardLayout respect WM_(SYS)DEADCHAR when it handles dead key. If WM_(SYS)KEYDOWN is followed by WM_DEADCHAR, that means that the key press is in a dead key sequence and not finishing the existing dead key sequence. Therefore, when WM_(SYS)KEYDOWN is followed by WM_(SYS)DEADCHAR, KeyboardLayout activates dead key sequence.
For supporting dead key chain, this patch makes KeyboardLayout::mActiveDeadKey and KeyboardLayout::mDeadKeyShiftState arrays. When dead keydown message is received, KeyboardLayout appends an item to each of them. (I.e., when the array is not empty, it's in a dead key sequence.)
When WM_(SYS)KEYUP is received, KeyboardLayout checks if it's in mActiveDeadKey. If it's included in the array, it initializes NativeKey as a dead keyup event.
Otherwise, when non-printable key (probably) is received in a dead key sequence, KeyboardLayout doesn't handle it as a part of the dead key sequence. For example, a modifier key may be pressed for next key. (Even if the keyboard layout maps text input to a non-printable key, we can ignore them because such key's KeyboardEvent.key value should be decided only with the virtual keyboard.)
MozReview-Commit-ID: 9n8B0YYuKCO
Fortunately, UniCharsAndModifiers instances are only in stack. Therefore, we can make it a stack class and use nsAutoString and AutoTArray for not using heap at handling inputs from usual keyboard layouts.
MozReview-Commit-ID: 9ZPbdjGst64
Now, we have an security issue. mCommittedCharsAndModifiers may be initialized with multiple WM_(SYS)CHAR messages. However, if it's generated by odd (or malicious) middleware, mCommittedCharsAndModifiers may be overflown because it has only fixed array. For fixing this issue, first, we should hide the members for making the users not depend on the design of UniCharsAndModifiers.
This patch changes UniCharsAndModifiers to a class and hiding mChars and adding |CharAt() const|.
MozReview-Commit-ID: 5EjrIhmCdE4
This patch creates NativeKey::DispatchKeyPressEventsWithRetrievedCharMessages() for dispatching eKeyPress event with mCommittedCharsAndModifiers when it stores following printable WM_(SYS)CHAR messages.
Using loop for dispatching eKeyPress event for every WM_(SYS)CHAR message is wrong because WidgetKeyboardEvent::mKeyValue is initialized with mCommittedCharsAndModifiers and it causes TextEventDispatcher dispatching multiple eKeyPress events at every call of MaybeDispatchKeypressEvents(). Therefore, if mKeyValue is "^^", eKeyPress event is dispatched 4 times --for the first message, eKeyPress events are fired for each "^" and for the second message, eKeyPress events are fired again for each "^"--. Therefore, when it handles WM_(SYS)KEYDOWN and it causes inputting one or more printable characters, it's the easiest way not to use HandleCharMessage().
The new method calls TextEventDispatcher::MaybeDispatchKeypressEvents() only once and it requests to call the callback method with new argument of MaybeDispatchKeypressEvents() when it needs to dispatch 2 or more eKeyPress events. Then, NativeKey::WillDispatchKeyboardEvent() can set each eKeyPress event to raw information of the message and proper modifier state.
With this change, we can dispatch multiple eKeyPress events with retrieved WM_(SYS)CHAR message information rather than retrieved information from active keyboard layout. Therefore, NeedsToHandleWithoutFollowingCharMessages() doesn't return true even when mCommittedCharsAndModifiers stores two or more characters.
FYI: there is a bug in test_keycodes.xul. That is, Alt+'A' of Greek keyboard layout should cause WM_SYSCHAR with a corresponding Greek character but ASCII characters are specified. Therefore, this patch includes the fix of these bugs
MozReview-Commit-ID: JVm7ZJVug0O
First, mCommittedCharsAndModifiers should be initialized with following char messages because the messages could be different from the information of current keyboard layout.
So, this patch guarantees that mCommittedCharsAndModifiers are same as the user expected when there is one or more WM_CHAR or WM_SYSCHAR messages.
MozReview-Commit-ID: I5Ack0xccoL
This is a simple mistake and blocks following patchs' automated tests.
For example, when Alt+Shift+foo doesn't cause text, this returns false even though it should return true.
MozReview-Commit-ID: 91L33vZhouT
Now, NativeKey::HandleCharMessage() has almost same code, one is for dispatching eKeyPress event for non-printable keys or printable keys when one of Alt or Ctrl key is pressed, the other is for printable keys when Alt or Ctrl key is pressed.
The difference of them is, the former block removes Alt state and Ctrl state for handling AltGr key. When AltGr key is pressed, both Alt and Ctrl state are true. However, EditorBase treas keypress events whose altKey or ctrlKey is true as non-printable key event. Therefore, we need to set these modifier state to false when AltGr key is pressed and the key causes some text.
Note that as far as we know, when a key press with AltGr doesn't cause any characters, WM_CHAR isn't generated. Therefore, we don't need to check with complicated logic if the key event is actually inputting a character.
MozReview-Commit-ID: BRNWfICvkSm
Current shortcut key handling is really difference from what we did before struggling with "key hell". Therefore, remaining hacks for charCode in NativeKey::HandleCharMessage() are not necessary because they are for old code.
MozReview-Commit-ID: 3hvsBOiJ6VV
This patch makes NativeKey::HandleCharMessage() stop dispatching eKeyPress event when the message is inputting a control character. NativeKey::HandleCharMessage() runs following cases:
1. WM_KEYDOWN followed by a WM_CHAR message whose wParam is not a control character causes inputting printable characters.
2. WM_CHAR message is received without WM_KEYDOWN message.
So, when HandleCharMessage() is called for a control character, it's unusual case. E.g., another application sends only WM_CHAR message with a control character.
On the other hand, Gecko is the only browser which dispatches "keypress" event even if pressed printable key doesn't cause inputting any characters. And such "keypress" event is used for shortcut key handling and some default action handling. So, even if we stop dispatching eKeyPress event from HandleCharMessage(), it shouldn't affect most users.
Note that this patch does NOT change the behavior when the user inputs a control character with usual keyboard layout such as Ctrl+A of en-US keyboard layout because DispatchKeyPressEventsWithoutCharMessage() dispatches eKeyPress event in such cases.
This patch also adds a lot of tests with Ctrl or Alt state for detecting regressions.
MozReview-Commit-ID: KUNqTp7RDSc
Currently, Enter and Backspace keys are handled without following WM_(SYS)CHAR message. However, older code needs hack for them for avoiding conflict with Ctrl+J vs. Ctrl+Enter, etc.
So, we can get rid of them now. And when a keydown causes inputting a control character, NativeKey should handle it with keyboard layout (i.e., without following char message(s)).
MozReview-Commit-ID: 6bVuK0BzFbx
Currently, NativeKey::HandleCharMessage() is only a public method and it takes any char message. However, when it's called outside of NativeKey, it should work only with NativeKey::mMsg.
Therefore, we should make current HandleCharMessage() a private method and create new overload method which doesn't take MSG as an argument.
MozReview-Commit-ID: LowV2FUmR3U
NativeKey shouldn't include characters which are provided by WM_SYSCHAR message or WM_DEADCHAR message into mCommittedCharsAndModifiers.
MozReview-Commit-ID: Ax1BmO5wTy0
For consistency with IsPrintableCharMessage(), IsFollowedByNonControlCharMessage() should be renamed to IsFollowedByPrintableCharMessage().
MozReview-Commit-ID: CBJFPO4FZej
Currently, NativeKey::IsFollowedByNonControlCharMessage() returns true only when the first char message is a printable char message. Although, I don't know actual cases coming printable WM_CHAR message after non-printable char message, i.e.,
1. WM_SYSCHAR or WM_DEADCHAR
2. WM_CHAR for a printable character
or
1. WM_CHAR with a non-printable character (a control character)
2. WM_CHAR with a printable character
, we should make it return true because when one or more characters are being inputted, we should ignore non-printable char messages and handle printable char messages in the path handling text input.
MozReview-Commit-ID: 1v7v5mCRFCP
Currently, NativeKey::IsPrintableCharMessage() returns true if given message is WM_CHAR or WM_SYSCHAR. However, WM_SYSCHAR never causes inputting any character and even if WM_CHAR has a control character (i.e., non-printable character), it returns true.
MozReview-Commit-ID: IoU5F06WImz
When key combination is reserved by the system, web apps shouldn't be able to consume the key events. In such case, we've decided that we never expose the key events in web contents (including chrome contents). Therefore, NativeKey stops dispatching keyboard events only for Alt+Space.
However, new code of the fix of bug 1300003 always consumes WM_SYSCHAR which follows WM_SYSKEYDOWN of Alt+Space. This is the cause of bug 1305943.
For fixing this bug, NativeKey should have a helper method, IsReservedBySystem(), and when it returns true, it should do nothing when Handle*Message() is called and should not consume following WM_SYSCHAR messages at handling WM_*KEYDOWN.
Unfortunately, it's impossible to test this regression because nsDOMWindowUtils::SendNativeKeyEvent() may call nsIWidget::SynthesizeNativeKeyEvent() asynchronously. See <https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.cpp#1105-1108>. Therefore, it cannot return if it's consumed.
MozReview-Commit-ID: 9ABh2rYNkFs
Currently, checking if it's in dead key sequence directly refers mActiveDeadKey. However, this isn't clear for most developers. So, let's create a helper method, IsInDeadKeySequence(), and wrap the nonintuitive check.
MozReview-Commit-ID: 7qTer9R8Gfs
For preventing wrapping long lines, old KeyboardLayout::InitNativeKey() uses two variables |virtualKey| and |isKeyDown| which are never modified. However, they may be not clear for some developers where they came from. Additionally, preceding patches reduced a lot of users of them and indent level. Therefore, even if we remove these variables, we don't need additional line breaks in most cases. So, removing them is better for easier to understand.
MozReview-Commit-ID: 680bYVINPAE
This patch creates KeyboardLayout::MaybeInitNativeKeyAsDeadKey() for wrapping dead key handling code in KeyboardLayout::InitNativeKey().
This makes InitNativeKey() code simpler. Now, any developers must be able to understand what InitNativeKey() does easier than before fixing this bug.
MozReview-Commit-ID: C59ESUXeTxU
There is DeactivateDeadKeyState() but not ActivateDeadKeyState(). There should be both of them and hides the dead key state management into them.
MozReview-Commit-ID: JvAPE5f2HVy
VirtualKey::GetCompositeChar() needs index of virtual keycode which may make around the caller messy. So, KeyboardLayout::GetCompositeChar() should wrap it and KeyboardLayout::MaybeInitNativeKeyWithCompositeChar() should use it.
MozReview-Commit-ID: 8KLTJpCFZ8u
Similar to VirtualKey::GetUniChars(), VirtualKey::GetNativeUniChars() needs key index. So, it should be wrapped by a new helper method, KeyboardLayout::GetNativeUniCharsAndModifiers(), and KeyboardEvent::InitNativeKey() should use this instead of accessing its member's GetNativeUniChars().
MozReview-Commit-ID: 7M9OlNF698Y
When InitNativeKey() retrieves UniCharsAndModifiers for a key, it needs key index for the given virtual keycode. Therefore, wrapping the code with GetUniCharsAndModifiers() makes InitNativeKey() code simpler since each call specifies the virtual keycode to the method instead of key index.
MozReview-Commit-ID: Azy8chXexaz
This patch gets rid of |shiftState| from KeyboardLayout::InitNativeKey() and make each caller clearer. This must make other developers understand what modifier state is used at each call.
MozReview-Commit-ID: 6zydP1jkffv
KeyboardLayout::InitNativeKey() is very messy because it handles a lot of cases without helper methods.
It's important to make it simpler implementation for preventing regressions caused by some patches which are written with misunderstanding. So, let's rewrite the method with helper method.
First, this patch make InitNativeKey() use IsDeadKey() instead of referring the table because calling IsDeadKey() is easier to understand.
MozReview-Commit-ID: DtN9qoh7Gz7
When NativeKey tries to remove a char message from the queue, another NativeKey instance might be created due to odd API hook or something. In such case, the old instance should handle the received message and the new instance should do nothing for keeping the order of message handling.
This patch makes:
* NativeKey::GetFollowingCharMsg() store removing char message to mRemovingMsg before calling PeekMessage() with PM_REMOVE.
* NativeKey::InitWithChar() set the old instance's mReceivedMsg to the received char message and do nothing even if HandleCharMessage() is called later.
* NativeKey::GetFollowingCharMsg() return received char message if mReceivedMsg is set during a call of PeekMessage().
MozReview-Commit-ID: KXYW0GgC9AY
For detecting nested creation of NativeKey instances, NativeKey should manage the latest instance with sLastestInstance for the other instances and previous instance with mLastInstance.
MozReview-Commit-ID: BFZ0cr1640S
For logging message, we should have better log text generator for MSG.
Then, we can improve the note of crash reports which is appended in NativeKey::GetFollowingCharMessage().
MozReview-Commit-ID: 4sbZJaWFH2o
Currently, NativeKey::DispatchPluginEventsAndDiscardsCharMessages() calls nsWindowBase::DispatchPluginEvent() and nsWindowBase::DispatchPluginEvent() does nothing when windowless plugin doesn't have focus. However, this is unclear when other developers read the code of DispatchPluginEventsAndDiscardsCharMessages() and it causes unnecessary virtual calls. So, it should try to dispatch plugin events only when a windowless plugin has focus.
For making the check safer, nsWindowBase should expose the method to check it as ShouldDispatchPluginEvent(). Unfortunately, we cannot mark this method as const due to PluginHasFocus() needs to be not const method, though. Then, for loops in DispatchPluginEventsAndDiscardsCharMessages() should check it at each try.
This change helps to log the behavior (working on bug 1297013) without noise.
Additionally, this patch renames the method to MaybeDispatchPluginEventsForRemovedCharMessages() because it doesn't remove any char messages anymore.
MozReview-Commit-ID: F14Lcx47M6U
NativeKey removes odd WM_CHAR messages which are caused by ATOK or WXG if the user tries to do "Kakutei-Undo" (meaning "undo the last commit") after dispatching eKeyDown event.
However, NativeKey should remove them from the queue first because there are some problems:
* If focus is moved during dispatching an eKeyDown event, we'll fail to remove the messages from the queue.
* If dispatching eKeyDown advance native event loop due to entering to a modal loop or dosing synchronous XHR, the message pattern is broken before removing the odd messages from the queue.
After removing the odd char messages, NativeKey should store them with mRemovedOddCharMsgs because NativeKey needs to dispatch them to focused windowless plugin.
Note that this patch also fixes a bug of the loop to remove the odd WM_CHAR messages. Old code removes only one WM_CHAR messages, but we need to remove all WM_CHAR messages before WM_KEYUP. This must be a regression.
MozReview-Commit-ID: I60bcIx2SFS
NativeKey is now removing and storing following char messages when it's created for a keydown message. Therefore, IsFollowedByNonControlCharMessage() just refers the stored messages and it's enough fast. So, we can get rid of mIsFollowedByNonControlCharMessage.
MozReview-Commit-ID: 542A2sHNXeC
