diff --git a/ui/base/accelerators/accelerator.h b/ui/base/accelerators/accelerator.h index 59008a42a14f13..780a45f9ca2dd6 100644 --- a/ui/base/accelerators/accelerator.h +++ b/ui/base/accelerators/accelerator.h @@ -93,6 +93,7 @@ class COMPONENT_EXPORT(UI_BASE) Accelerator { #if defined(OS_CHROMEOS) DomCode code() const { return code_; } + void reset_code() { code_ = DomCode::NONE; } #endif // Sets the key state that triggers the accelerator. Default is PRESSED. diff --git a/ui/base/accelerators/accelerator_map.h b/ui/base/accelerators/accelerator_map.h index 039940977124cc..e64eb5fc086cd4 100644 --- a/ui/base/accelerators/accelerator_map.h +++ b/ui/base/accelerators/accelerator_map.h @@ -79,8 +79,13 @@ class COMPONENT_EXPORT(UI_BASE) AcceleratorMap { V& GetOrInsertDefault(const Accelerator& accelerator) { #if defined(OS_CHROMEOS) - // Cannot explicitly register an accelerator with a DomCode. - DCHECK_EQ(DomCode::NONE, accelerator.code()); + // Ensure the DomCode is NONE before registering. The DomCode is only + // used during lookup to select the correct VKEY. + if (accelerator.code() != DomCode::NONE) { + Accelerator accelerator_copy = accelerator; + accelerator_copy.reset_code(); + return map_[accelerator_copy]; + } #endif return map_[accelerator]; } @@ -94,8 +99,16 @@ class COMPONENT_EXPORT(UI_BASE) AcceleratorMap { // accelerator was already in the map. void InsertNew(const std::pair& value) { #if defined(OS_CHROMEOS) - // Cannot explicitly register an accelerator with a DomCode. - DCHECK_EQ(DomCode::NONE, value.first.code()); + // Ensure the DomCode is NONE before registering. The DomCode is only + // used during lookup to select the correct VKEY. + if (value.first.code() != DomCode::NONE) { + Accelerator accelerator_copy = value.first; + accelerator_copy.reset_code(); + auto value_copy = std::make_pair(accelerator_copy, value.second); + auto result = map_.insert(value_copy); + DCHECK(result.second); + return; + } #endif auto result = map_.insert(value); DCHECK(result.second); @@ -154,12 +167,15 @@ class COMPONENT_EXPORT(UI_BASE) AcceleratorMap { return Accelerator(lookup_key_code, DomCode::NONE, accelerator.modifiers(), accelerator.key_state()); } -#endif +#endif // defined(OS_CHROMEOS) const_iterator FindImpl(const Accelerator& accelerator) const { #if defined(OS_CHROMEOS) - return map_.find(RemapAcceleratorForLookup(accelerator)); -#endif + auto iter = map_.find(RemapAcceleratorForLookup(accelerator)); + // Sanity check that a DomCode was never inserted into the map. + DCHECK(iter == map_.end() || iter->first.code() == DomCode::NONE); + return iter; +#endif // defined(OS_CHROMEOS) return map_.find(accelerator); } diff --git a/ui/base/accelerators/accelerator_map_unittest.cc b/ui/base/accelerators/accelerator_map_unittest.cc index 79d11a97ffa518..a89715d4bb94cd 100644 --- a/ui/base/accelerators/accelerator_map_unittest.cc +++ b/ui/base/accelerators/accelerator_map_unittest.cc @@ -8,6 +8,7 @@ #include "testing/gtest/include/gtest/gtest.h" #include "ui/base/accelerators/accelerator.h" +#include "ui/events/event.h" #include "ui/events/keycodes/dom/dom_code.h" namespace ui { @@ -206,6 +207,27 @@ TEST(AcceleratorMapTest, PositionalLookupVkeyMatchOnlyRegisteredDomCodeIsNone) { EXPECT_TRUE(IsValidMatch(&m, pressed, expected)); } +// When an accelerator is inserted to the map, if it contains a DomCode it +// should be stripped out. +TEST(AcceleratorMapTest, DomCodesStrippedWhenInserted) { + AcceleratorMap m; + m.set_use_positional_lookup(true); + + // Verify the accelerator has a DomCode and insert it. + Accelerator accelerator(ui::VKEY_F, DomCode::US_F, + ui::EF_ALT_DOWN | ui::EF_CONTROL_DOWN); + EXPECT_EQ(accelerator.code(), DomCode::US_F); + const int expected = 77; + m.InsertNew(std::make_pair(accelerator, expected)); + + // Reset the DomCode on the accelerator and perform a lookup and verify + // that it can still be found. + accelerator.reset_code(); + auto* value = m.Find(accelerator); + ASSERT_TRUE(value); + EXPECT_EQ(*value, expected); +} + #endif // defined(OS_CHROMEOS) } // namespace diff --git a/ui/base/accelerators/accelerator_unittest.cc b/ui/base/accelerators/accelerator_unittest.cc index 74282004e8e78f..e2e8e6ebfcee97 100644 --- a/ui/base/accelerators/accelerator_unittest.cc +++ b/ui/base/accelerators/accelerator_unittest.cc @@ -7,8 +7,10 @@ #include #include "base/strings/utf_string_conversions.h" +#include "base/test/scoped_feature_list.h" #include "build/build_config.h" #include "testing/gtest/include/gtest/gtest.h" +#include "ui/base/ui_base_features.h" #include "ui/events/event.h" namespace ui { @@ -74,4 +76,35 @@ TEST(AcceleratorTest, ShortcutTextForUnknownKey) { EXPECT_EQ(std::u16string(), accelerator.GetShortcutText()); } +TEST(AcceleratorTest, ConversionFromKeyEvent) { + ui::KeyEvent key_event(ui::ET_KEY_PRESSED, ui::VKEY_F, + ui::EF_ALT_DOWN | ui::EF_CONTROL_DOWN); + Accelerator accelerator(key_event); + + EXPECT_EQ(accelerator.key_code(), ui::VKEY_F); + EXPECT_EQ(accelerator.modifiers(), ui::EF_ALT_DOWN | ui::EF_CONTROL_DOWN); +} + +#if BUILDFLAG(IS_CHROMEOS_ASH) +TEST(AcceleratorTest, ConversionFromKeyEvent_Ash) { + base::test::ScopedFeatureList scoped_feature_list; + scoped_feature_list.InitAndEnableFeature( + ::features::kImprovedKeyboardShortcuts); + + ui::KeyEvent key_event(ui::ET_KEY_PRESSED, ui::VKEY_F, + ui::EF_ALT_DOWN | ui::EF_CONTROL_DOWN); + Accelerator accelerator(key_event); + + EXPECT_EQ(accelerator.key_code(), ui::VKEY_F); + EXPECT_EQ(accelerator.modifiers(), ui::EF_ALT_DOWN | ui::EF_CONTROL_DOWN); + + // Code is set when converting from a KeyEvent. + EXPECT_EQ(accelerator.code(), DomCode::US_F); + + // Test resetting code. + accelerator.reset_code(); + EXPECT_EQ(accelerator.code(), DomCode::NONE); +} +#endif // BUILDFLAG(IS_CHROMEOS_ASH) + } // namespace ui