diff --git a/ash/accelerators/accelerator_controller_impl.cc b/ash/accelerators/accelerator_controller_impl.cc index 9ceffe6132e0f3..e1a56eba9e05b6 100644 --- a/ash/accelerators/accelerator_controller_impl.cc +++ b/ash/accelerators/accelerator_controller_impl.cc @@ -1720,8 +1720,8 @@ void AcceleratorControllerImpl::UnregisterAll(ui::AcceleratorTarget* target) { bool AcceleratorControllerImpl::IsActionForAcceleratorEnabled( const ui::Accelerator& accelerator) const { - auto it = accelerators_.find(accelerator); - return it != accelerators_.end() && CanPerformAction(it->second, accelerator); + const AcceleratorAction* action_ptr = accelerators_.Find(accelerator); + return action_ptr && CanPerformAction(*action_ptr, accelerator); } bool AcceleratorControllerImpl::Process(const ui::Accelerator& accelerator) { @@ -1747,12 +1747,9 @@ bool AcceleratorControllerImpl::OnMenuAccelerator( const ui::Accelerator& accelerator) { accelerator_history()->StoreCurrentAccelerator(accelerator); - auto itr = accelerators_.find(accelerator); - if (itr == accelerators_.end()) - return false; // Menu shouldn't be closed for an invalid accelerator. - - AcceleratorAction action = itr->second; - return !base::Contains(actions_keeping_menu_open_, action); + // Menu shouldn't be closed for an invalid accelerator. + AcceleratorAction* action_ptr = accelerators_.Find(accelerator); + return action_ptr && !base::Contains(actions_keeping_menu_open_, *action_ptr); } bool AcceleratorControllerImpl::IsRegistered( @@ -1766,20 +1763,14 @@ AcceleratorHistoryImpl* AcceleratorControllerImpl::GetAcceleratorHistory() { bool AcceleratorControllerImpl::IsPreferred( const ui::Accelerator& accelerator) const { - auto iter = accelerators_.find(accelerator); - if (iter == accelerators_.end()) - return false; // not an accelerator. - - return base::Contains(preferred_actions_, iter->second); + const AcceleratorAction* action_ptr = accelerators_.Find(accelerator); + return action_ptr && base::Contains(preferred_actions_, *action_ptr); } bool AcceleratorControllerImpl::IsReserved( const ui::Accelerator& accelerator) const { - auto iter = accelerators_.find(accelerator); - if (iter == accelerators_.end()) - return false; // not an accelerator. - - return base::Contains(reserved_actions_, iter->second); + const AcceleratorAction* action_ptr = accelerators_.Find(accelerator); + return action_ptr && base::Contains(reserved_actions_, *action_ptr); } AcceleratorControllerImpl::AcceleratorProcessingRestriction @@ -1792,9 +1783,7 @@ AcceleratorControllerImpl::GetCurrentAcceleratorRestriction() { bool AcceleratorControllerImpl::AcceleratorPressed( const ui::Accelerator& accelerator) { - auto it = accelerators_.find(accelerator); - DCHECK(it != accelerators_.end()); - AcceleratorAction action = it->second; + AcceleratorAction action = accelerators_.Get(accelerator); if (!CanPerformAction(action, accelerator)) return false; @@ -1891,7 +1880,8 @@ void AcceleratorControllerImpl::RegisterAccelerators( CreateAccelerator(accelerators[i].keycode, accelerators[i].modifiers, accelerators[i].trigger_on_press); ui_accelerators.push_back(accelerator); - accelerators_.insert(std::make_pair(accelerator, accelerators[i].action)); + accelerators_.InsertNew( + std::make_pair(accelerator, accelerators[i].action)); } Register(ui_accelerators, this); } @@ -1910,7 +1900,8 @@ void AcceleratorControllerImpl::RegisterDeprecatedAccelerators() { accelerator_data.trigger_on_press); ui_accelerators.push_back(deprecated_accelerator); - accelerators_[deprecated_accelerator] = accelerator_data.action; + accelerators_.InsertNew( + std::make_pair(deprecated_accelerator, accelerator_data.action)); deprecated_accelerators_.insert(deprecated_accelerator); } Register(ui_accelerators, this); diff --git a/ash/accelerators/accelerator_controller_impl.h b/ash/accelerators/accelerator_controller_impl.h index 546bc2a702240c..0c28e9ccbb5bca 100644 --- a/ash/accelerators/accelerator_controller_impl.h +++ b/ash/accelerators/accelerator_controller_impl.h @@ -22,6 +22,7 @@ #include "base/gtest_prod_util.h" #include "base/macros.h" #include "ui/base/accelerators/accelerator.h" +#include "ui/base/accelerators/accelerator_map.h" namespace ui { class AcceleratorManager; @@ -230,7 +231,7 @@ class ASH_EXPORT AcceleratorControllerImpl : public ui::AcceleratorTarget, private: // A map for looking up actions from accelerators. - using AcceleratorActionMap = std::map; + using AcceleratorActionMap = ui::AcceleratorMap; // Initializes the accelerators this class handles as a target. void Init(); diff --git a/ui/base/BUILD.gn b/ui/base/BUILD.gn index d26e454c8ed7db..6a77a7a8b854bd 100644 --- a/ui/base/BUILD.gn +++ b/ui/base/BUILD.gn @@ -329,6 +329,7 @@ component("base") { "accelerators/accelerator.h", "accelerators/accelerator_manager.cc", "accelerators/accelerator_manager.h", + "accelerators/accelerator_map.h", "accelerators/media_keys_listener.cc", "accelerators/media_keys_listener.h", "accelerators/media_keys_util.cc", @@ -462,6 +463,7 @@ component("base") { # iOS does not use Chromium-specific code for event handling. public_deps += [ "//ui/base/clipboard:file_info", + "//ui/events:dom_keycode_converter", "//ui/events:events_base", "//ui/events/platform", ] @@ -966,6 +968,7 @@ test("ui_base_unittests") { } else { # !is_ios sources += [ "accelerators/accelerator_manager_unittest.cc", + "accelerators/accelerator_map_unittest.cc", "accelerators/accelerator_unittest.cc", "accelerators/menu_label_accelerator_util_unittest.cc", "models/dialog_model_unittest.cc", diff --git a/ui/base/accelerators/accelerator.cc b/ui/base/accelerators/accelerator.cc index c44f3d3752025b..d6913b15149f77 100644 --- a/ui/base/accelerators/accelerator.cc +++ b/ui/base/accelerators/accelerator.cc @@ -31,7 +31,7 @@ #include "ui/events/keycodes/keyboard_code_conversion.h" #endif -#if BUILDFLAG(IS_CHROMEOS_ASH) +#if defined(OS_CHROMEOS) #include "ui/base/ui_base_features.h" #endif @@ -108,6 +108,20 @@ Accelerator::Accelerator(KeyboardCode key_code, time_stamp_(time_stamp), interrupted_by_mouse_event_(false) {} +#if defined(OS_CHROMEOS) +Accelerator::Accelerator(KeyboardCode key_code, + DomCode code, + int modifiers, + KeyState key_state, + base::TimeTicks time_stamp) + : key_code_(key_code), + code_(code), + key_state_(key_state), + modifiers_(modifiers & kInterestingFlagsMask), + time_stamp_(time_stamp), + interrupted_by_mouse_event_(false) {} +#endif + Accelerator::Accelerator(const KeyEvent& key_event) : key_code_(key_event.key_code()), key_state_(key_event.type() == ET_KEY_PRESSED ? KeyState::PRESSED @@ -117,8 +131,15 @@ Accelerator::Accelerator(const KeyEvent& key_event) time_stamp_(key_event.time_stamp()), interrupted_by_mouse_event_(false), source_device_id_(key_event.source_device_id()) { +#if defined(OS_CHROMEOS) + if (features::IsImprovedKeyboardShortcutsEnabled()) { + code_ = key_event.code(); + } +#endif + #if BUILDFLAG(IS_CHROMEOS_ASH) if (features::IsNewShortcutMappingEnabled()) { + DCHECK(!features::IsImprovedKeyboardShortcutsEnabled()); DomKey dom_key = key_event.GetDomKey(); if (!dom_key.IsCharacter()) return; @@ -154,7 +175,11 @@ KeyEvent Accelerator::ToKeyEvent() const { return KeyEvent(key_state() == Accelerator::KeyState::PRESSED ? ET_KEY_PRESSED : ET_KEY_RELEASED, - key_code(), modifiers(), time_stamp()); + key_code(), +#if defined(OS_CHROMEOS) + code(), +#endif + modifiers(), time_stamp()); } bool Accelerator::operator<(const Accelerator& rhs) const { diff --git a/ui/base/accelerators/accelerator.h b/ui/base/accelerators/accelerator.h index 62eacbe23f949c..59008a42a14f13 100644 --- a/ui/base/accelerators/accelerator.h +++ b/ui/base/accelerators/accelerator.h @@ -21,6 +21,10 @@ #include "ui/events/event_constants.h" #include "ui/events/keycodes/keyboard_codes.h" +#if defined(OS_CHROMEOS) +#include "ui/events/keycodes/dom/dom_code.h" +#endif + namespace ui { class KeyEvent; @@ -46,6 +50,26 @@ class COMPONENT_EXPORT(UI_BASE) Accelerator { int modifiers, KeyState key_state = KeyState::PRESSED, base::TimeTicks time_stamp = base::TimeTicks()); + +#if defined(OS_CHROMEOS) + // Additional constructor that takes a |DomCode| in order to implement + // layout independent fixed position shortcuts. This is only used for + // shortcuts in Chrome OS. One such example is Alt ']'. In the US layout ']' + // is VKEY_OEM_6, in the DE layout it is VKEY_OEM_PLUS. However the key in + // that position is always DomCode::BRACKET_RIGHT regardless of what the key + // generates when pressed. When the DE layout is used and the accelerator + // is created with { VKEY_OEM_PLUS, DomCode::BRACKET_RIGHT } the custom + // accelerator map will map BRACKET_RIGHT to VKEY_OEM_6 as if in the US + // layout in order to lookup the accelerator. + // + // See accelerator_map.h for more information. + Accelerator(KeyboardCode key_code, + DomCode code, + int modifiers, + KeyState key_state = KeyState::PRESSED, + base::TimeTicks time_stamp = base::TimeTicks()); +#endif + explicit Accelerator(const KeyEvent& key_event); Accelerator(const Accelerator& accelerator); Accelerator& operator=(const Accelerator& accelerator); @@ -67,6 +91,10 @@ class COMPONENT_EXPORT(UI_BASE) Accelerator { KeyboardCode key_code() const { return key_code_; } +#if defined(OS_CHROMEOS) + DomCode code() const { return code_; } +#endif + // Sets the key state that triggers the accelerator. Default is PRESSED. void set_key_state(KeyState state) { key_state_ = state; } KeyState key_state() const { return key_state_; } @@ -107,6 +135,11 @@ class COMPONENT_EXPORT(UI_BASE) Accelerator { // The keycode (VK_...). KeyboardCode key_code_; +#if defined(OS_CHROMEOS) + // The DomCode representing a key's physical position. + DomCode code_ = DomCode::NONE; +#endif + KeyState key_state_; // The state of the Shift/Ctrl/Alt keys. This corresponds to Event::flags(). diff --git a/ui/base/accelerators/accelerator_manager.cc b/ui/base/accelerators/accelerator_manager.cc index 695f378c067da9..01d842d9716ee5 100644 --- a/ui/base/accelerators/accelerator_manager.cc +++ b/ui/base/accelerators/accelerator_manager.cc @@ -20,25 +20,24 @@ void AcceleratorManager::Register( DCHECK(target); for (const ui::Accelerator& accelerator : accelerators) { - accelerators_[accelerator].RegisterWithPriority(target, priority); + accelerators_.GetOrInsertDefault(accelerator) + .RegisterWithPriority(target, priority); } } void AcceleratorManager::Unregister(const Accelerator& accelerator, AcceleratorTarget* target) { DCHECK(target); - auto map_iter = accelerators_.find(accelerator); - DCHECK(map_iter != accelerators_.end()) - << "Unregistering non-existing accelerator"; + AcceleratorTargetInfo* target_info = accelerators_.Find(accelerator); + DCHECK(target_info) << "Unregistering non-existing accelerator"; - AcceleratorTargetInfo& target_info = map_iter->second; - const bool was_registered = target_info.Unregister(target); + const bool was_registered = target_info->Unregister(target); DCHECK(was_registered) << "Unregistering accelerator for wrong target"; // If the last target for the accelerator is removed, then erase the // entry from the map. - if (!target_info.HasTargets()) - accelerators_.erase(map_iter); + if (!target_info->HasTargets()) + accelerators_.Erase(accelerator); } void AcceleratorManager::UnregisterAll(AcceleratorTarget* target) { @@ -49,7 +48,9 @@ void AcceleratorManager::UnregisterAll(AcceleratorTarget* target) { // Unregister the target and remove the entry if it was the last target. const bool was_registered = target_info.Unregister(target); if (was_registered && !target_info.HasTargets()) { - map_iter = accelerators_.erase(map_iter); + Accelerator key_to_remove = map_iter->first; + ++map_iter; + accelerators_.Erase(key_to_remove); continue; } @@ -59,35 +60,31 @@ void AcceleratorManager::UnregisterAll(AcceleratorTarget* target) { } bool AcceleratorManager::IsRegistered(const Accelerator& accelerator) const { - auto map_iter = accelerators_.find(accelerator); + const AcceleratorTargetInfo* target_info = accelerators_.Find(accelerator); // If the accelerator is in the map, the target list should not be empty. - DCHECK(map_iter == accelerators_.end() || map_iter->second.HasTargets()); - return map_iter != accelerators_.end(); + DCHECK(!target_info || target_info->HasTargets()); + return target_info != nullptr; } bool AcceleratorManager::Process(const Accelerator& accelerator) { - auto map_iter = accelerators_.find(accelerator); - if (map_iter == accelerators_.end()) + const AcceleratorTargetInfo* target_info = accelerators_.Find(accelerator); + if (!target_info) return false; // If the accelerator is in the map, the target list should not be empty. - AcceleratorTargetInfo& target_info = map_iter->second; - DCHECK(target_info.HasTargets()); + DCHECK(target_info->HasTargets()); // We have to copy the target list here, because processing the accelerator // event handler may modify the list. - AcceleratorTargetInfo target_info_copy(target_info); + AcceleratorTargetInfo target_info_copy(*target_info); return target_info_copy.TryProcess(accelerator); } bool AcceleratorManager::HasPriorityHandler( const Accelerator& accelerator) const { - auto map_iter = accelerators_.find(accelerator); - if (map_iter == accelerators_.end()) - return false; - - return map_iter->second.HasPriorityHandler(); + const AcceleratorTargetInfo* target_info = accelerators_.Find(accelerator); + return target_info && target_info->HasPriorityHandler(); } AcceleratorManager::AcceleratorTargetInfo::AcceleratorTargetInfo() = default; diff --git a/ui/base/accelerators/accelerator_manager.h b/ui/base/accelerators/accelerator_manager.h index 0586b8cd2043dd..ef63397df2d95e 100644 --- a/ui/base/accelerators/accelerator_manager.h +++ b/ui/base/accelerators/accelerator_manager.h @@ -11,6 +11,7 @@ #include "base/component_export.h" #include "ui/base/accelerators/accelerator.h" +#include "ui/base/accelerators/accelerator_map.h" namespace ui { @@ -112,13 +113,16 @@ class COMPONENT_EXPORT(UI_BASE) AcceleratorManager { // Returns true if there are registered targets. bool HasTargets() const { return !targets_.empty(); } + // Returns the number of targets for this accelerator. + size_t size() const { return targets_.size(); } + private: std::list targets_; bool has_priority_handler_ = false; }; // The accelerators and associated targets. - std::map accelerators_; + AcceleratorMap accelerators_; }; } // namespace ui diff --git a/ui/base/accelerators/accelerator_map.h b/ui/base/accelerators/accelerator_map.h new file mode 100644 index 00000000000000..039940977124cc --- /dev/null +++ b/ui/base/accelerators/accelerator_map.h @@ -0,0 +1,175 @@ +// Copyright 2021 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef UI_BASE_ACCELERATORS_ACCELERATOR_MAP_H_ +#define UI_BASE_ACCELERATORS_ACCELERATOR_MAP_H_ + +#include +#include +#include + +#include "base/component_export.h" +#include "ui/base/accelerators/accelerator.h" + +#if defined(OS_CHROMEOS) +#include "ui/events/keycodes/dom/dom_code.h" +#include "ui/events/keycodes/dom/keycode_converter.h" +#endif + +namespace ui { + +// This is a wrapper around an internal std::map of type +// |std::map| where |V| is the mapped value. +// +// Accelerators in Chrome on all platforms are specified with the |key_code|, +// aka VKEY, however certain keys (eg. brackets, comma, period, plus, minus), +// are in different places based on the keyboard layout. In some cases the +// VKEYs don't exist, in some cases they now conflict with other shortcuts. +// +// Chrome OS uses a positional mapping for this subset of keys. Shortcuts +// based on these keys are determined by the position of the key on a US +// keyboard. This was already the case for all non-latin alphabet keyboards +// (Chinese, Japanese, Arabic, Russian, etc.). +// +// To achieve this on Chrome OS for the remaining layouts, an additional +// remapping may happen to the accelerator used for lookup based on the state +// of |use_positional_lookup_|. When false no remapping occurs. When true, +// the |code| aka DomCode (which is by definition fixed position), is used to +// find the US layout VKEY, and that VKEY is used to lookup in the map. +// +// Other non-positional keys, eg. alphanumeric, F-keys, and special keys are +// all not remapped. Alphanumeric keys always continue to follow the +// |code|/VKEY defined by the current layout as is existing behavior. +template +class COMPONENT_EXPORT(UI_BASE) AcceleratorMap { + public: + AcceleratorMap() = default; + ~AcceleratorMap() = default; + + using iterator = typename std::map::iterator; + using const_iterator = typename std::map::const_iterator; + + // Lookup an accelerator and return a pointer to the value. If the + // accelerator is not in the map, this returns nullptr. + const V* Find(const Accelerator& accelerator) const { + auto iter = FindImpl(accelerator); + return iter == map_.end() ? nullptr : &iter->second; + } + + V* Find(const Accelerator& accelerator) { + // Call the const implementation to avoid duplicating code. + return const_cast( + const_cast(this)->Find(accelerator)); + } + + // Lookup an accelerator and return a reference to the value. If the + // accelerator is not present this function will DCHECK. + const V& Get(const Accelerator& accelerator) const { + auto iter = FindImpl(accelerator); + DCHECK(iter != map_.end()); + return iter->second; + } + + V& Get(const Accelerator& accelerator) { + // Call the const implementation to avoid duplicating code. + return const_cast( + const_cast(this)->Get(accelerator)); + } + + V& GetOrInsertDefault(const Accelerator& accelerator) { +#if defined(OS_CHROMEOS) + // Cannot explicitly register an accelerator with a DomCode. + DCHECK_EQ(DomCode::NONE, accelerator.code()); +#endif + return map_[accelerator]; + } + + // Erase an accelerator from the map. + bool Erase(const Accelerator& accelerator) { + return map_.erase(accelerator) > 0; + } + + // Inserts a new accelerator and value into the map. DCHECKs if the + // 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()); +#endif + auto result = map_.insert(value); + DCHECK(result.second); + } + + // Iterators for the internal map. + iterator begin() { return map_.begin(); } + iterator end() { return map_.end(); } + + // Returns the number of items in the map. + size_t size() const { return map_.size(); } + + // Returns true if the map is empty. + bool empty() const { return map_.empty(); } + +#if defined(OS_CHROMEOS) + // When true, lookup operators on the map will remap the |key_code| of + // position-based keys based on the |code|. + void set_use_positional_lookup(bool use_positional_lookup) { + use_positional_lookup_ = use_positional_lookup; + } +#endif + + private: + std::map map_; + +#if defined(OS_CHROMEOS) + bool use_positional_lookup_ = false; + + // For the shortcuts that use positional mapping, the lookup is done based + // on the |key_code| in the US layout. The supplied |code| (DomCode) is used + // to remap the layout specific |key_code| with the US layout equivalent, + // which effectively pins the location of these keys in place regardless of + // the actual key mapping. Note that this only happens for a subset of keys + // and has no overlap with alphanumeric characters or the language equivalent + // keys that map to the alphanumeric set of VKEYS. + // + // One such example is Alt ']'. In the US layout ']' is VKEY_OEM_6, in the + // DE layout it is VKEY_OEM_PLUS. However the key in that position is always + // DomCode::BRACKET_RIGHT regardless of what the key generates when pressed. + // This function uses the DomCode to remap the VKEY back to it's US layout + // equivalent. + // + // See the design doc in crbug.com/1174326 for more information. + Accelerator RemapAcceleratorForLookup(const Accelerator& accelerator) const { + KeyboardCode lookup_key_code = accelerator.key_code(); + if (use_positional_lookup_) { + // If there's a valid remapping, use that |KeyboardCode| instead. + KeyboardCode remapped_key_code = + KeycodeConverter::MapPositionalDomCodeToUSShortcutKey( + accelerator.code()); + lookup_key_code = remapped_key_code != VKEY_UNKNOWN ? remapped_key_code + : lookup_key_code; + } + + return Accelerator(lookup_key_code, DomCode::NONE, accelerator.modifiers(), + accelerator.key_state()); + } +#endif + + const_iterator FindImpl(const Accelerator& accelerator) const { +#if defined(OS_CHROMEOS) + return map_.find(RemapAcceleratorForLookup(accelerator)); +#endif + + return map_.find(accelerator); + } + + iterator FindImpl(const Accelerator& accelerator) { + return const_cast( + const_cast(this)->FindImpl(accelerator)); + } +}; + +} // namespace ui + +#endif // UI_BASE_ACCELERATORS_ACCELERATOR_MAP_H_ diff --git a/ui/base/accelerators/accelerator_map_unittest.cc b/ui/base/accelerators/accelerator_map_unittest.cc new file mode 100644 index 00000000000000..79d11a97ffa518 --- /dev/null +++ b/ui/base/accelerators/accelerator_map_unittest.cc @@ -0,0 +1,213 @@ +// Copyright 2021 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "ui/base/accelerators/accelerator_map.h" + +#include + +#include "testing/gtest/include/gtest/gtest.h" +#include "ui/base/accelerators/accelerator.h" +#include "ui/events/keycodes/dom/dom_code.h" + +namespace ui { + +namespace { + +bool IsValidMatch(AcceleratorMap* map, + const Accelerator& pressed, + int expected) { + return nullptr != map->Find(pressed) && expected == *map->Find(pressed) && + expected == map->Get(pressed); +} + +TEST(AcceleratorMapTest, MapIsEmpty) { + AcceleratorMap m; + EXPECT_EQ(0U, m.size()); + EXPECT_TRUE(m.empty()); +} + +TEST(AcceleratorMapTest, EmptyFind) { + AcceleratorMap m; + Accelerator accelerator(VKEY_Z, EF_SHIFT_DOWN); + EXPECT_EQ(nullptr, m.Find(accelerator)); + + // Still empty after lookup. + EXPECT_TRUE(m.empty()); +} + +TEST(AcceleratorMapTest, FindExists) { + AcceleratorMap m; + Accelerator accelerator(VKEY_Z, EF_SHIFT_DOWN); + + const int expected = 77; + m.InsertNew(std::make_pair(accelerator, expected)); + EXPECT_TRUE(IsValidMatch(&m, accelerator, expected)); + + // Still single entry. + EXPECT_EQ(1U, m.size()); +} + +TEST(AcceleratorMapTest, FindDoesNotExist) { + AcceleratorMap m; + Accelerator accelerator(VKEY_Z, EF_SHIFT_DOWN); + Accelerator other(VKEY_Y, EF_SHIFT_DOWN); + + const int expected = 77; + m.InsertNew(std::make_pair(accelerator, expected)); + EXPECT_EQ(nullptr, m.Find(other)); + + // Still single entry. + EXPECT_EQ(1U, m.size()); +} + +TEST(AcceleratorMapTest, InsertDefaultCreatedNew) { + AcceleratorMap m; + Accelerator accelerator(VKEY_Z, EF_SHIFT_DOWN); + int& value_ref = m.GetOrInsertDefault(accelerator); + EXPECT_EQ(int(), value_ref); + EXPECT_EQ(1U, m.size()); +} + +TEST(AcceleratorMapTest, ChangeValueViaReturnedRef) { + AcceleratorMap m; + Accelerator accelerator(VKEY_Z, EF_SHIFT_DOWN); + int& value_ref = m.GetOrInsertDefault(accelerator); + + const int expected = 77; + value_ref = expected; + EXPECT_EQ(expected, m.Get(accelerator)); +} + +TEST(AcceleratorMapTest, SetValueDirect) { + AcceleratorMap m; + Accelerator accelerator(VKEY_Z, EF_SHIFT_DOWN); + + const int expected = 77; + m.InsertNew(std::make_pair(accelerator, expected)); + EXPECT_EQ(expected, m.Get(accelerator)); +} + +TEST(AcceleratorMapTest, Iterate) { + AcceleratorMap m; + Accelerator accelerator(VKEY_Z, EF_SHIFT_DOWN); + const int expected = 77; + m.InsertNew(std::make_pair(accelerator, expected)); + + auto iter = m.begin(); + EXPECT_NE(m.end(), iter); + EXPECT_EQ(accelerator, iter->first); + EXPECT_EQ(expected, iter->second); + ++iter; + EXPECT_EQ(m.end(), iter); +} + +// Chrome OS specific tests. +// Only Chrome OS supports positional shortcuts. +#if defined(OS_CHROMEOS) + +// Even with positional lookup enabled, if both the stored and lookup +// accelerator have no DomCode then the behavior is as if there was no +// positional lookup. +TEST(AcceleratorMapTest, PositionalLookupExistsVkeyOnly) { + AcceleratorMap m; + m.set_use_positional_lookup(true); + Accelerator accelerator(VKEY_Z, EF_SHIFT_DOWN); + EXPECT_EQ(DomCode::NONE, accelerator.code()); + + const int expected = 77; + m.InsertNew(std::make_pair(accelerator, expected)); + EXPECT_TRUE(IsValidMatch(&m, accelerator, expected)); +} + +// Both the VKEY and DomCode match, so this is always a match. This scenario +// happens when positional shortcuts are enabled, and the layout has a VKEY +// mapping consistent with the US layout. +TEST(AcceleratorMapTest, PositionalLookupExistsFullMatch) { + AcceleratorMap m; + m.set_use_positional_lookup(true); + + Accelerator registered(VKEY_OEM_6, EF_SHIFT_DOWN); + const int expected = 77; + m.InsertNew(std::make_pair(registered, expected)); + + Accelerator pressed(VKEY_OEM_6, DomCode::BRACKET_RIGHT, EF_SHIFT_DOWN); + EXPECT_TRUE(IsValidMatch(&m, pressed, expected)); +} + +// The DomCode matches, but the VKEY does not - this is a positional match. +// This scenario happens on eg. German and Spanish keyboards. +TEST(AcceleratorMapTest, PositionalLookupDomCodeMatchOnly) { + AcceleratorMap m; + m.set_use_positional_lookup(true); + + Accelerator registered(VKEY_OEM_6, EF_SHIFT_DOWN); + const int expected = 77; + m.InsertNew(std::make_pair(registered, expected)); + + Accelerator pressed(VKEY_OEM_PLUS, DomCode::BRACKET_RIGHT, EF_SHIFT_DOWN); + EXPECT_TRUE(IsValidMatch(&m, pressed, expected)); +} + +// With positional mapping enabled this first press is like the ']' key +// on a German keyboard, and it matches. When positional mapping is disabled +// it no longer matches because the VKEYs are different. +// +// Disabling positional lookup also used for special layouts like Dvorak which +// are designed to intentionally reposition certain punctuation keys. These +// layouts already work with US-like VKEY mapping, albeit to keys in different +// positions. +TEST(AcceleratorMapTest, PositionalLookupDisabled) { + AcceleratorMap m; + + // NOTE: The state of use_positional_lookup_ has no effect on insertion. + Accelerator registered(VKEY_OEM_6, EF_SHIFT_DOWN); + const int expected = 77; + m.InsertNew(std::make_pair(registered, expected)); + + // This lookup with succeed with positional lookup enabled. + m.set_use_positional_lookup(true); + Accelerator pressed(VKEY_OEM_PLUS, DomCode::BRACKET_RIGHT, EF_SHIFT_DOWN); + EXPECT_TRUE(IsValidMatch(&m, pressed, expected)); + + // Switch to a non-positional layout before testing the pressed keys and + // this lookup with fail. + m.set_use_positional_lookup(false); + EXPECT_FALSE(IsValidMatch(&m, pressed, expected)); +} + +// The VKEY matches, and both the registered and pressed accelerator supply a +// positional DomCode - this is not a match. This prevents ghost or conflicting +// shortcuts on the key that has the matching VKEY. This is a scenario on a +// Spanish keyboard. +TEST(AcceleratorMapTest, PositionalLookupVkeyMatchOnlyBothDomCodesSpecified) { + AcceleratorMap m; + m.set_use_positional_lookup(true); + Accelerator registered(VKEY_OEM_6, EF_SHIFT_DOWN); + const int expected = 77; + m.InsertNew(std::make_pair(registered, expected)); + + Accelerator pressed(VKEY_OEM_6, DomCode::EQUAL, EF_SHIFT_DOWN); + EXPECT_FALSE(IsValidMatch(&m, pressed, expected)); +} + +// The VKEY matches, and the registered accelerator has no DomCode (ie. it's +// non-positional) - this is a match. This scenario is to allow non-positional +// shortcuts to continue to work regardless of the DomCode. This is a scenario +// for the Z key on a German or other QWERTZ layout. +TEST(AcceleratorMapTest, PositionalLookupVkeyMatchOnlyRegisteredDomCodeIsNone) { + AcceleratorMap m; + m.set_use_positional_lookup(true); + Accelerator registered(VKEY_Z, EF_SHIFT_DOWN); + const int expected = 77; + m.InsertNew(std::make_pair(registered, expected)); + + Accelerator pressed(VKEY_Z, DomCode::US_Y, EF_SHIFT_DOWN); + EXPECT_TRUE(IsValidMatch(&m, pressed, expected)); +} + +#endif // defined(OS_CHROMEOS) + +} // namespace + +} // namespace ui \ No newline at end of file diff --git a/ui/base/ui_base_features.cc b/ui/base/ui_base_features.cc index 1c28cc5ce42a3f..951599c00d0085 100644 --- a/ui/base/ui_base_features.cc +++ b/ui/base/ui_base_features.cc @@ -58,14 +58,6 @@ bool IsNewShortcutMappingEnabled() { base::FeatureList::IsEnabled(kNewShortcutMapping); } -// This feature supercedes kNewShortcutMapping. -const base::Feature kImprovedKeyboardShortcuts = { - "ImprovedKeyboardShortcuts", base::FEATURE_DISABLED_BY_DEFAULT}; - -bool IsImprovedKeyboardShortcutsEnabled() { - return base::FeatureList::IsEnabled(kImprovedKeyboardShortcuts); -} - const base::Feature kDeprecateAltClick = {"DeprecateAltClick", base::FEATURE_DISABLED_BY_DEFAULT}; @@ -180,6 +172,16 @@ const base::Feature kPrecisionTouchpadLogging{ "PrecisionTouchpadLogging", base::FEATURE_DISABLED_BY_DEFAULT}; #endif // defined(OS_WIN) +#if defined(OS_CHROMEOS) +// This feature supercedes kNewShortcutMapping. +const base::Feature kImprovedKeyboardShortcuts = { + "ImprovedKeyboardShortcuts", base::FEATURE_DISABLED_BY_DEFAULT}; + +bool IsImprovedKeyboardShortcutsEnabled() { + return base::FeatureList::IsEnabled(kImprovedKeyboardShortcuts); +} +#endif // defined(OS_CHROMEOS) + #if defined(OS_WIN) || defined(OS_APPLE) || defined(OS_LINUX) || \ defined(OS_CHROMEOS) // Enables stylus appearing as touch when in contact with digitizer. diff --git a/ui/base/ui_base_features.h b/ui/base/ui_base_features.h index fbcac263e2c769..22eea521021e05 100644 --- a/ui/base/ui_base_features.h +++ b/ui/base/ui_base_features.h @@ -65,6 +65,14 @@ COMPONENT_EXPORT(UI_BASE_FEATURES) extern const base::Feature kTSFImeSupport; COMPONENT_EXPORT(UI_BASE_FEATURES) bool IsUsingWMPointerForTouch(); #endif // defined(OS_WIN) +#if defined(OS_CHROMEOS) +// This flag is intended to supercede kNewShortcutMapping. +COMPONENT_EXPORT(UI_BASE_FEATURES) +extern const base::Feature kImprovedKeyboardShortcuts; +COMPONENT_EXPORT(UI_BASE_FEATURES) +bool IsImprovedKeyboardShortcutsEnabled(); +#endif // defined(OS_CHROMEOS) + #if defined(OS_WIN) || defined(OS_APPLE) || defined(OS_LINUX) || \ defined(OS_CHROMEOS) COMPONENT_EXPORT(UI_BASE_FEATURES) @@ -110,13 +118,6 @@ extern const base::Feature kNewShortcutMapping; COMPONENT_EXPORT(UI_BASE_FEATURES) bool IsNewShortcutMappingEnabled(); -// This flag is intended to supercede kNewShortcutMapping above. -COMPONENT_EXPORT(UI_BASE_FEATURES) -extern const base::Feature kImprovedKeyboardShortcuts; - -COMPONENT_EXPORT(UI_BASE_FEATURES) -bool IsImprovedKeyboardShortcutsEnabled(); - COMPONENT_EXPORT(UI_BASE_FEATURES) extern const base::Feature kDeprecateAltClick; diff --git a/ui/content_accelerators/accelerator_util.cc b/ui/content_accelerators/accelerator_util.cc index 223df257be03bf..3d66d744447b68 100644 --- a/ui/content_accelerators/accelerator_util.cc +++ b/ui/content_accelerators/accelerator_util.cc @@ -17,7 +17,8 @@ namespace ui { ui::Accelerator GetAcceleratorFromNativeWebKeyboardEvent( const content::NativeWebKeyboardEvent& event) { #if BUILDFLAG(IS_CHROMEOS_ASH) - if (::features::IsNewShortcutMappingEnabled()) { + if (::features::IsNewShortcutMappingEnabled() || + ::features::IsImprovedKeyboardShortcutsEnabled()) { // TODO: This must be the same as below and it's simpler. // Cleanup if this change sticks. auto* os_event = static_cast(event.os_event); diff --git a/ui/events/keycodes/dom/keycode_converter.cc b/ui/events/keycodes/dom/keycode_converter.cc index c6ed4ac20a8e37..4c3881badab9de 100644 --- a/ui/events/keycodes/dom/keycode_converter.cc +++ b/ui/events/keycodes/dom/keycode_converter.cc @@ -178,6 +178,60 @@ int KeycodeConverter::DomCodeToEvdevCode(DomCode code) { } #endif +#if defined(OS_CHROMEOS) +// static +DomCode KeycodeConverter::MapUSPositionalShortcutKeyToDomCode( + KeyboardCode key_code) { + // VKEY Mapping: http://kbdlayout.info/kbdus/overview+virtualkeys + // DomCode Mapping: + // https://www.w3.org/TR/DOM-Level-3-Events-code/#writing-system-keys + switch (key_code) { + case VKEY_OEM_MINUS: + return DomCode::MINUS; + case VKEY_OEM_PLUS: + return DomCode::EQUAL; + case VKEY_OEM_2: + return DomCode::SLASH; + case VKEY_OEM_4: + return DomCode::BRACKET_LEFT; + case VKEY_OEM_6: + return DomCode::BRACKET_RIGHT; + case VKEY_OEM_COMMA: + return DomCode::COMMA; + case VKEY_OEM_PERIOD: + return DomCode::PERIOD; + default: + return DomCode::NONE; + } +} + +// static +KeyboardCode KeycodeConverter::MapPositionalDomCodeToUSShortcutKey( + DomCode code) { + // VKEY Mapping: http://kbdlayout.info/kbdus/overview+virtualkeys + // DomCode Mapping: + // https://www.w3.org/TR/DOM-Level-3-Events-code/#writing-system-keys + switch (code) { + case DomCode::MINUS: + return VKEY_OEM_MINUS; + case DomCode::EQUAL: + return VKEY_OEM_PLUS; + case DomCode::SLASH: + return VKEY_OEM_2; + case DomCode::BRACKET_LEFT: + return VKEY_OEM_4; + case DomCode::BRACKET_RIGHT: + return VKEY_OEM_6; + case DomCode::COMMA: + return VKEY_OEM_COMMA; + case DomCode::PERIOD: + return VKEY_OEM_PERIOD; + default: + return VKEY_UNKNOWN; + } +} +#endif + // static DomCode KeycodeConverter::CodeStringToDomCode(const std::string& code) { if (code.empty()) diff --git a/ui/events/keycodes/dom/keycode_converter.h b/ui/events/keycodes/dom/keycode_converter.h index b6d0f12539bbeb..032ab082894a32 100644 --- a/ui/events/keycodes/dom/keycode_converter.h +++ b/ui/events/keycodes/dom/keycode_converter.h @@ -12,6 +12,10 @@ #include "build/build_config.h" #include "ui/events/keycodes/dom/dom_key.h" +#if defined(OS_CHROMEOS) +#include "ui/events/keycodes/keyboard_codes_posix.h" +#endif + // For reference, the W3C UI Event spec is located at: // http://www.w3.org/TR/uievents/ @@ -73,6 +77,20 @@ class KeycodeConverter { static int DomCodeToEvdevCode(DomCode code); #endif +#if defined(OS_CHROMEOS) + // If |key_code| is one of the keys (plus, minus, brackets, period, comma), + // that are treated positionally for keyboard shortcuts, this returns the + // DomCode of that key in the US layout. Any other key returns + // |DomCode::NONE|. + static DomCode MapUSPositionalShortcutKeyToDomCode(KeyboardCode key_code); + + // If |code| is one of the keys (plus, minus, brackets, period, comma) that + // are treated positionally for keyboard shortcuts, this returns the + // KeyboardCode (aka VKEY) of that key in the US layout. Any other key + // returns |VKEY_UNKNOWN| + static KeyboardCode MapPositionalDomCodeToUSShortcutKey(DomCode code); +#endif + // Convert a UI Events |code| string value into a DomCode. static DomCode CodeStringToDomCode(const std::string& code);