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

Conversation

bleroux
Copy link
Contributor

@bleroux bleroux commented Sep 5, 2022

Description

#33686, introduced the Android implementation of the backend of HardwareKeyboard.
#34827 fixed a crash related to incorrect metaState.
This PR fixes another crash related to incorrect metaState and a zeroed scanCode.

Implementation

#34827 synthesizes a key up event when there is a down event with incorrect metaState.
If the scanCode is zeroed the computed physical key codes are not the same (for the down event and the up event) and leads to an incorrect state.
This PR updates how the key up event is synthesized by using the same physical key code (the one in the down event).

Related Issue

Fixes flutter/flutter#110640

Tests

Adds 1 test.

@chinmaygarde
Copy link
Member

Ping @justinmc @LongCatIsLooong

Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM with nits about comments 👍

Thanks for fixing this!

I see no problem with passing through eventPhysicalKey but anyone with more experience in the Android engine feel free to chime in.

@@ -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! 😊


// 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 than KeyboardManager.getPhysicalKey
Copy link
Contributor

Choose a reason for hiding this comment

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

"same way than" => "same was as"

And also period at the end.

@Test
public void synchronizeModifiersForZeroedScanCode() {
// Test if ShiftLeft can be correctly synchronized during down events of
// ShiftLeft that has 0 for its metaState and 0 for its scanCode.
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly:

"has" => "have"
"its" => "their"

@bleroux bleroux force-pushed the fix_android_crash_zeroed_scancode branch from d98e78b to 24337ea Compare September 13, 2022 18:34
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.

Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

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

LGTM

@bleroux bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 14, 2022
@auto-submit auto-submit bot merged commit fe5b84a into flutter:main Sep 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 14, 2022
@bleroux bleroux deleted the fix_android_crash_zeroed_scancode branch September 14, 2022 10:01
itsjustkevin pushed a commit that referenced this pull request Sep 20, 2022
…ting text in `TextField` (#36288)

* [Android] Synchronize incorrect metaState using post-synchronization (#34827)

(cherry picked from commit a56d98e)

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

(cherry picked from commit fe5b84a)

Co-authored-by: Tong Mu <[email protected]>
Co-authored-by: Bruno Leroux <[email protected]>
Oleh-Sv pushed a commit to Oleh-Sv/engine that referenced this pull request Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Flutter 3.3] Fatal crash with java.lang.AssertionError when selecting text in TextField
4 participants