Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Android] Fix a crash related to zeroed scanCode #35924

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ void updatePressingState(@NonNull Long physicalKey, @Nullable Long logicalKey) {
// the current event in consideration.
//
// Events that should be synthesized before the main event are synthesized
// immediately, while events that should be syntehsized after the main event are appended to
// immediately, while events that should be synthesized after the main event are appended to
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for catching that! 😊

// `postSynchronize`.
//
// Although Android KeyEvent defined bitmasks for sided modifiers (SHIFT_LEFT_ON and
Expand All @@ -133,6 +133,7 @@ void synchronizePressingKey(
PressingGoal goal,
boolean truePressed,
long eventLogicalKey,
long eventPhysicalKey,
KeyEvent event,
ArrayList<Runnable> postSynchronize) {
// During an incoming event, there might be a synthesized Flutter event for each key of each
Expand All @@ -151,8 +152,8 @@ void synchronizePressingKey(
// 2. Derive the pre-event state of the event key (if applicable.)
for (int keyIdx = 0; keyIdx < goal.keys.length; keyIdx += 1) {
final KeyboardMap.KeyPair key = goal.keys[keyIdx];
nowStates[keyIdx] = pressingRecords.containsKey(goal.keys[keyIdx].physicalKey);
if (goal.keys[keyIdx].logicalKey == eventLogicalKey) {
nowStates[keyIdx] = pressingRecords.containsKey(key.physicalKey);
if (key.logicalKey == eventLogicalKey) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm, are these two lines of change make any difference, or purely cosmetic? (I'm ok with either)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Purely cosmetic. The 'key' variable was already defined (before this PR) and It could be used on this two lines.

switch (getEventType(event)) {
case kDown:
preEventStates[keyIdx] = false;
Expand All @@ -161,7 +162,7 @@ void synchronizePressingKey(
postSynchronize.add(
() ->
synthesizeEvent(
false, key.logicalKey, key.physicalKey, event.getEventTime()));
false, key.logicalKey, eventPhysicalKey, event.getEventTime()));
}
break;
case kUp:
Expand Down Expand Up @@ -273,7 +274,12 @@ private boolean handleEventImpl(
final ArrayList<Runnable> postSynchronizeEvents = new ArrayList<>();
for (final PressingGoal goal : KeyboardMap.pressingGoals) {
synchronizePressingKey(
goal, (event.getMetaState() & goal.mask) != 0, logicalKey, event, postSynchronizeEvents);
goal,
(event.getMetaState() & goal.mask) != 0,
logicalKey,
physicalKey,
event,
postSynchronizeEvents);
}

for (final TogglingGoal goal : togglingGoals.values()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1350,7 +1350,7 @@ public void synchronizeOtherModifiers() {
@Test
public void synchronizeModifiersForConflictingMetaState() {
// Test if ShiftLeft can be correctly synchronized during down events of
// ShiftLeft that has 0 for its metaState.
// ShiftLeft that have 0 for their metaState.
final KeyboardTester tester = new KeyboardTester();
final ArrayList<CallRecord> calls = new ArrayList<>();
// Even though the event is for ShiftRight, we still set SHIFT | SHIFT_LEFT here.
Expand Down Expand Up @@ -1396,6 +1396,31 @@ public void synchronizeModifiersForConflictingMetaState() {
calls.clear();
}

// Regression test for https://github.com/flutter/flutter/issues/110640
@Test
public void synchronizeModifiersForZeroedScanCode() {
// Test if ShiftLeft can be correctly synchronized during down events of
// ShiftLeft that have 0 for their metaState and 0 for their scanCode.
final KeyboardTester tester = new KeyboardTester();
final ArrayList<CallRecord> calls = new ArrayList<>();

tester.recordEmbedderCallsTo(calls);
tester.respondToTextInputWith(true); // Suppress redispatching

// Test: DOWN event when the current state is 0 and scanCode is 0.
final KeyEvent keyEvent = new FakeKeyEvent(ACTION_DOWN, 0, KEYCODE_SHIFT_LEFT, 0, '\0', 0);
// Compute physicalKey in the same way as KeyboardManager.getPhysicalKey.
final Long physicalKey = KEYCODE_SHIFT_LEFT | KeyboardMap.kAndroidPlane;

assertEquals(tester.keyboardManager.handleEvent(keyEvent), true);
assertEquals(calls.size(), 2);
assertEmbedderEventEquals(
calls.get(0).keyData, Type.kDown, physicalKey, LOGICAL_SHIFT_LEFT, null, false);
assertEmbedderEventEquals(
calls.get(1).keyData, Type.kUp, physicalKey, LOGICAL_SHIFT_LEFT, null, true);
calls.clear();
}

@Test
public void normalCapsLockEvents() {
final KeyboardTester tester = new KeyboardTester();
Expand Down