From 706a90eeab04356df1419a7d152573aee683fe92 Mon Sep 17 00:00:00 2001 From: Filip Filmar Date: Thu, 18 Nov 2021 12:55:19 -0800 Subject: [PATCH] [fuchsia] Fixes the HID usage handling The old code stripped the USB hid usage page value from the Fuchsia key. It shouldn't be doing that. This change fixes the issue, and adds tests that rely on the key constants to verify that the change indeed does what it is supposed to do. In the process of fixing this, filed a few known issues and marked them as TODO. These issues should be handled in separate PRs. Fixes: https://github.com/flutter/flutter/issues/93890 --- shell/platform/fuchsia/flutter/keyboard.cc | 24 +++++++++++++-- shell/platform/fuchsia/flutter/keyboard.h | 22 +++++++++++++- .../fuchsia/flutter/keyboard_unittest.cc | 30 ++++++++++++++++++- .../fuchsia/flutter/platform_view_unittest.cc | 15 +++++----- 4 files changed, 79 insertions(+), 12 deletions(-) diff --git a/shell/platform/fuchsia/flutter/keyboard.cc b/shell/platform/fuchsia/flutter/keyboard.cc index 5353535003e67..8b8e58bf9a475 100644 --- a/shell/platform/fuchsia/flutter/keyboard.cc +++ b/shell/platform/fuchsia/flutter/keyboard.cc @@ -285,7 +285,7 @@ bool Keyboard::IsShift() { } bool Keyboard::IsKeys() { - return (static_cast(last_event_.key()) & 0xFFFF0000) == 0x00070000; + return LastHIDUsagePage() == 0x7; } uint32_t Keyboard::Modifiers() { @@ -298,12 +298,16 @@ uint32_t Keyboard::Modifiers() { } uint32_t Keyboard::LastCodePoint() { + // TODO(https://github.com/flutter/flutter/issues/93891): + // `KeyEvent::key_meaning` should be used here to do key mapping before US + // QWERTY is applied. The US QWERTY should be applied only if `key_meaning` + // is not available. static const int qwerty_map_size = sizeof(QWERTY_TO_CODE_POINTS) / sizeof(QWERTY_TO_CODE_POINTS[0]); if (!IsKeys()) { return 0; } - const auto usage = LastHIDUsage(); + const auto usage = LastHIDUsageID(); if (usage < qwerty_map_size) { return QWERTY_TO_CODE_POINTS[usage][IsShift() & 1]; } @@ -311,8 +315,22 @@ uint32_t Keyboard::LastCodePoint() { return 0; } +uint32_t Keyboard::GetLastKey() { + // TODO(https://github.com/flutter/flutter/issues/93898): key is not always + // set. Figure out what to do then. + return static_cast(last_event_.key()); +} + uint32_t Keyboard::LastHIDUsage() { - return static_cast(last_event_.key()) & 0xFFFF; + return GetLastKey() & 0xFFFFFFFF; +} + +uint16_t Keyboard::LastHIDUsageID() { + return GetLastKey() & 0xFFFF; +} + +uint16_t Keyboard::LastHIDUsagePage() { + return (GetLastKey() >> 16) & 0xFFFF; } } // namespace flutter_runner diff --git a/shell/platform/fuchsia/flutter/keyboard.h b/shell/platform/fuchsia/flutter/keyboard.h index 86135488f4f78..fea877ef73914 100644 --- a/shell/platform/fuchsia/flutter/keyboard.h +++ b/shell/platform/fuchsia/flutter/keyboard.h @@ -28,9 +28,26 @@ class Keyboard final { // the state of the modifier keys. uint32_t LastCodePoint(); - // Gets the last encountered HID usage. + // Gets the last encountered HID usage. This is a 32-bit number, with the + // upper 16 bits equal to `LastHidUsagePage()`, and the lower 16 bits equal + // to `LastHIDUsageID()`. + // + // The key corresponding to A will have the usage 0x7004. This function will + // return 0x7004 in that case. uint32_t LastHIDUsage(); + // Gets the last encountered HID usage page. + // + // The key corresponding to A will have the usage 0x7004. This function will + // return 0x7 in that case. + uint16_t LastHIDUsagePage(); + + // Gets the last encountered HID usage ID. + // + // The key corresponding to A will have the usage 0x7004. This function will + // return 0x4 in that case. + uint16_t LastHIDUsageID(); + private: // Return true if any level shift is active. bool IsShift(); @@ -39,6 +56,9 @@ class Keyboard final { // point associated. bool IsKeys(); + // Returns the value of the last key as a uint32_t. + uint32_t GetLastKey(); + // Set to false until any event is received. bool any_events_received_ : 1; diff --git a/shell/platform/fuchsia/flutter/keyboard_unittest.cc b/shell/platform/fuchsia/flutter/keyboard_unittest.cc index bb00d319e29f5..6fabb629cf6d8 100644 --- a/shell/platform/fuchsia/flutter/keyboard_unittest.cc +++ b/shell/platform/fuchsia/flutter/keyboard_unittest.cc @@ -60,12 +60,40 @@ class KeyboardTest : public testing::Test { } // Converts a pressed key to usage value. - uint32_t ToUsage(Key key) { return static_cast(key) & 0xFFFF; } + uint32_t ToUsage(Key key) { return static_cast(key) & 0xFFFFFFFF; } private: zx_time_t timestamp_ = 0; }; +// Checks whether the HID usage, page and ID values are reported correctly. +TEST_F(KeyboardTest, UsageValues) { + std::vector keys; + keys.emplace_back(NewKeyEvent(KeyEventType::SYNC, Key::CAPS_LOCK)); + Keyboard keyboard; + ConsumeEvents(&keyboard, keys); + + // Values for Caps Lock. + // See spec at: + // https://cs.opensource.google/fuchsia/fuchsia/+/main:sdk/fidl/fuchsia.input/keys.fidl;l=177;drc=e3b39f2b57e720770773b857feca4f770ee0619e + EXPECT_EQ(0x07u, keyboard.LastHIDUsagePage()); + EXPECT_EQ(0x39u, keyboard.LastHIDUsageID()); + EXPECT_EQ(0x70039u, keyboard.LastHIDUsage()); + + // Try also an usage that is not on page 7. This one is on page 0x0C. + // See spec at: + // https://cs.opensource.google/fuchsia/fuchsia/+/main:sdk/fidl/fuchsia.input/keys.fidl;l=339;drc=e3b39f2b57e720770773b857feca4f770ee0619e + // Note that Fuchsia does not define constants for every key you may think of, + // rather only those that we had the need for. However it is not an issue + // to add more keys if needed. + keys.clear(); + keys.emplace_back(NewKeyEvent(KeyEventType::SYNC, Key::MEDIA_MUTE)); + ConsumeEvents(&keyboard, keys); + EXPECT_EQ(0x0Cu, keyboard.LastHIDUsagePage()); + EXPECT_EQ(0xE2u, keyboard.LastHIDUsageID()); + EXPECT_EQ(0xC00E2u, keyboard.LastHIDUsage()); +} + // This test checks that if a caps lock has been pressed when we didn't have // focus, the effect of caps lock remains. Only this first test case is // commented to explain how the test case works. diff --git a/shell/platform/fuchsia/flutter/platform_view_unittest.cc b/shell/platform/fuchsia/flutter/platform_view_unittest.cc index bd76c79fc9385..13c27a2afe898 100644 --- a/shell/platform/fuchsia/flutter/platform_view_unittest.cc +++ b/shell/platform/fuchsia/flutter/platform_view_unittest.cc @@ -1250,18 +1250,19 @@ TEST_F(PlatformViewTests, OnKeyEvent) { std::vector events; // Press A. Get 'a'. + // The HID usage for the key A is 0x70004, or 458756. events.emplace_back(EventFlow{ MakeEvent(fuchsia::ui::input3::KeyEventType::PRESSED, std::nullopt, fuchsia::input::Key::A), fuchsia::ui::input3::KeyEventStatus::HANDLED, - R"({"type":"keydown","keymap":"fuchsia","hidUsage":4,"codePoint":97,"modifiers":0})", + R"({"type":"keydown","keymap":"fuchsia","hidUsage":458756,"codePoint":97,"modifiers":0})", }); // Release A. Get 'a' release. events.emplace_back(EventFlow{ MakeEvent(fuchsia::ui::input3::KeyEventType::RELEASED, std::nullopt, fuchsia::input::Key::A), fuchsia::ui::input3::KeyEventStatus::HANDLED, - R"({"type":"keyup","keymap":"fuchsia","hidUsage":4,"codePoint":97,"modifiers":0})", + R"({"type":"keyup","keymap":"fuchsia","hidUsage":458756,"codePoint":97,"modifiers":0})", }); // Press CAPS_LOCK. Modifier now active. events.emplace_back(EventFlow{ @@ -1269,14 +1270,14 @@ TEST_F(PlatformViewTests, OnKeyEvent) { fuchsia::ui::input3::Modifiers::CAPS_LOCK, fuchsia::input::Key::CAPS_LOCK), fuchsia::ui::input3::KeyEventStatus::HANDLED, - R"({"type":"keydown","keymap":"fuchsia","hidUsage":57,"codePoint":0,"modifiers":1})", + R"({"type":"keydown","keymap":"fuchsia","hidUsage":458809,"codePoint":0,"modifiers":1})", }); - // Pres A. Get 'A'. + // Press A. Get 'A'. events.emplace_back(EventFlow{ MakeEvent(fuchsia::ui::input3::KeyEventType::PRESSED, std::nullopt, fuchsia::input::Key::A), fuchsia::ui::input3::KeyEventStatus::HANDLED, - R"({"type":"keydown","keymap":"fuchsia","hidUsage":4,"codePoint":65,"modifiers":1})", + R"({"type":"keydown","keymap":"fuchsia","hidUsage":458756,"codePoint":65,"modifiers":1})", }); // Release CAPS_LOCK. events.emplace_back(EventFlow{ @@ -1284,7 +1285,7 @@ TEST_F(PlatformViewTests, OnKeyEvent) { fuchsia::ui::input3::Modifiers::CAPS_LOCK, fuchsia::input::Key::CAPS_LOCK), fuchsia::ui::input3::KeyEventStatus::HANDLED, - R"({"type":"keyup","keymap":"fuchsia","hidUsage":57,"codePoint":0,"modifiers":1})", + R"({"type":"keyup","keymap":"fuchsia","hidUsage":458809,"codePoint":0,"modifiers":1})", }); // Press A again. This time get 'A'. // CAPS_LOCK is latched active even if it was just released. @@ -1292,7 +1293,7 @@ TEST_F(PlatformViewTests, OnKeyEvent) { MakeEvent(fuchsia::ui::input3::KeyEventType::PRESSED, std::nullopt, fuchsia::input::Key::A), fuchsia::ui::input3::KeyEventStatus::HANDLED, - R"({"type":"keydown","keymap":"fuchsia","hidUsage":4,"codePoint":65,"modifiers":1})", + R"({"type":"keydown","keymap":"fuchsia","hidUsage":458756,"codePoint":65,"modifiers":1})", }); for (const auto& event : events) {