Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Do not expect WM_CHAR if control or windows key is pressed #27064

Merged
fluttergithubbot merged 4 commits into from
Jul 7, 2021
Merged

Do not expect WM_CHAR if control or windows key is pressed #27064

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
adcab21
Do not expect WM_CHAR if control or windows key is pressed
knopp Jun 30, 2021
ce062cf
Address PR feedback
knopp Jul 6, 2021
167dbb3
Add unittest
knopp Jul 6, 2021
5bc088f
Reformat
knopp Jul 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions shell/platform/windows/window_win32.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,11 +371,18 @@ WindowWin32::HandleMessage(UINT const message,
// Check if this key produces a character. If so, the key press should
// be sent with the character produced at WM_CHAR. Store the produced
// keycode (it's not accessible from WM_CHAR) to be used in WM_CHAR.
const unsigned int character = MapVirtualKey(wparam, MAPVK_VK_TO_CHAR);
if (character > 0 && is_keydown_message) {
//
// Messages with Control or Win modifiers down are never considered as
// character messages. This allows key combinations such as "CTRL + Digit"
// to properly produce key down events even though `MapVirtualKey` returns
// a valid character. See https://github.com/flutter/flutter/issues/85587.
unsigned int character = MapVirtualKey(wparam, MAPVK_VK_TO_CHAR);
if (character > 0 && is_keydown_message && GetKeyState(VK_CONTROL) == 0 &&
GetKeyState(VK_LWIN) == 0 && GetKeyState(VK_RWIN) == 0) {
keycode_for_char_message_ = wparam;
break;
}
character = 0;
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Jul 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this line for?

Copy link
Member Author

@knopp knopp Jul 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was suggested here (for clarity), but somehow ended up in the wrong block :-/. Since it doesn't do anything anyway I'll make another PR to remove it.

unsigned int keyCode(wparam);
const unsigned int scancode = (lparam >> 16) & 0xff;
const bool extended = ((lparam >> 24) & 0x01) == 0x01;
Expand Down
22 changes: 22 additions & 0 deletions shell/platform/windows/window_win32_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,5 +84,27 @@ TEST(MockWin32Window, KeyDownPrintable) {
window.InjectWindowMessage(WM_CHAR, 65, lparam);
}

TEST(MockWin32Window, KeyDownWithCtrl) {
MockWin32Window window;

// Simulate CONTROL pressed
BYTE keyboard_state[256];
memset(keyboard_state, 0, 256);
keyboard_state[VK_CONTROL] = -1;
SetKeyboardState(keyboard_state);

LPARAM lparam = CreateKeyEventLparam(30);

// Expect OnKey, but not OnText, because Control + Key is not followed by
// WM_CHAR
EXPECT_CALL(window, OnKey(65, 30, WM_KEYDOWN, 0, false, true)).Times(1);
EXPECT_CALL(window, OnText(_)).Times(0);
Copy link
Contributor

@dkwingsmt dkwingsmt Jul 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check if this test fails without the change? As far as I remember, EXPECT_CALL.Times only checks if this many calls have been called, but not extra calls.

Copy link
Member Author

@knopp knopp Jul 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand. Without what change? This is a compltely new test. It should result in OnKey called once, and no OnText call.

Copy link
Contributor

@dkwingsmt dkwingsmt Jul 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without your fix to window_win32.cc. Basically, whether this test fails on master.

Copy link
Member Author

@knopp knopp Jul 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the test fails because OnKey is not invoked on the mock without the change. It waits for WM_CHAR, which never comes.

Copy link
Contributor

@dkwingsmt dkwingsmt Jul 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Thanks for confirming.


window.InjectWindowMessage(WM_KEYDOWN, 65, lparam);

memset(keyboard_state, 0, 256);
SetKeyboardState(keyboard_state);
}

} // namespace testing
} // namespace flutter