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

knopp · 2021-06-30T10:13:33Z

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2021-06-30T10:13:35Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

dkwingsmt

The change looks legit. Can you add some tests? See window_win32_unittests.cc for guidance. (Using recorded event information from manual tests would be the best).

dkwingsmt · 2021-06-30T21:28:37Z

shell/platform/windows/window_win32.cc

+      // As an exception with control or windows key pressed WM_CHAR may not
+      // come even though the event produced a character (i.e. CTRL + number).


Suggested change

// As an exception with control or windows key pressed WM_CHAR may not

// come even though the event produced a character (i.e. CTRL + number).

//

// 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.

Also might be better to force character to be 0 when GetKeyState(VK_CONTROL) == 0 && GetKeyState(VK_LWIN) == 0 && GetKeyState(VK_RWIN) == 0 for clarity.

knopp · 2021-07-06T20:14:28Z

@dkwingsmt, thank you for the feeddback. Can you re-review the PR?

dkwingsmt · 2021-07-06T23:00:18Z

shell/platform/windows/window_win32_unittests.cc

+  // 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);


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.

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.

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

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

Got it. Thanks for confirming.

dkwingsmt

LGTM

…lutter/engine#27064)

stuartmorgan-g · 2021-07-07T22:41:50Z

shell/platform/windows/window_win32.cc

        keycode_for_char_message_ = wparam;
        break;
      }
+      character = 0;


What is this line for?

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.

tomm1e · 2021-07-10T04:27:44Z

Pressing CTRL once will make all input produce twice, pressing CTRL again will restore normal input.

It even persists if you close the app. Press CTRL once to activate, close the executable, launch it again, its still producing twice.

knopp · 2021-07-10T06:56:26Z

I can reproduce this. Not sure how I missed this but apparently on Windows all modifiers key can have the toggle bit set. From GetKeyState documentation I assumed this only applies to toggle-able keys such as CapsLock, but apparently not. I'll submit PR for this shortly.

…7064)

flutter-dashboard bot added the platform-windows label Jun 30, 2021

flutter-dashboard bot added the needs tests label Jun 30, 2021

google-cla bot added the cla: yes label Jun 30, 2021

knopp requested review from dkwingsmt and stuartmorgan-g June 30, 2021 10:18

dkwingsmt suggested changes Jun 30, 2021

View reviewed changes

knopp added 3 commits July 6, 2021 20:16

Do not expect WM_CHAR if control or windows key is pressed

adcab21

Address PR feedback

ce062cf

Add unittest

167dbb3

knopp force-pushed the win_ctrl_number branch from eb49595 to 167dbb3 Compare July 6, 2021 20:17

knopp removed the needs tests label Jul 6, 2021

Reformat

5bc088f

dkwingsmt reviewed Jul 6, 2021

View reviewed changes

dkwingsmt approved these changes Jul 6, 2021

View reviewed changes

knopp added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jul 6, 2021

fluttergithubbot merged commit b63d6e2 into flutter:master Jul 7, 2021

engine-flutter-autoroll mentioned this pull request Jul 7, 2021

Roll Engine from 1bce28c946a7 to dd17c0883ec0 (3 revisions) flutter/flutter#86027

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 7, 2021

b63d6e2 Do not expect WM_CHAR if control or windows key is pressed (f…

f11dd68

…lutter/engine#27064)

stuartmorgan-g reviewed Jul 7, 2021

View reviewed changes

knopp mentioned this pull request Jul 7, 2021

Remove unnecessary variable assignment #27227

Merged

8 tasks

tomm1e mentioned this pull request Jul 10, 2021

Windows: Dead keys are not producing accented characters flutter/flutter#86179

Closed

knopp mentioned this pull request Jul 10, 2021

Windows: Toggling CTRL key produces duplicate input flutter/flutter#86217

Closed

dkwingsmt mentioned this pull request Jul 14, 2021

[Windows] Dispatch non-surrogate events at WM_KEYDOWN #27391

Closed

8 tasks

knopp deleted the win_ctrl_number branch July 17, 2021 09:25

moffatman pushed a commit to moffatman/engine that referenced this pull request Aug 5, 2021

Do not expect WM_CHAR if control or windows key is pressed (flutter#2…

4b2b8b0

…7064)

dkwingsmt mentioned this pull request Aug 11, 2021

Windows keyboard integration tests and fix a number of issues #27921

Merged

8 tasks

naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021

Do not expect WM_CHAR if control or windows key is pressed (flutter#2…

961d4f8

…7064)

		// As an exception with control or windows key pressed WM_CHAR may not
		// come even though the event produced a character (i.e. CTRL + number).

-      // As an exception with control or windows key pressed WM_CHAR may not
-      // come even though the event produced a character (i.e. CTRL + number).
+      //
+      // 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.

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

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

Uh oh!

Conversation

knopp commented Jun 30, 2021

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Jun 30, 2021

Uh oh!

dkwingsmt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knopp commented Jul 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkwingsmt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomm1e commented Jul 10, 2021

Uh oh!

knopp commented Jul 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants