0

shortcuts: Split a11y switch focus shortcuts into two

These shortcuts were harded coded and did not respect aliasing
rules. This change splits them into two separate shortcuts that
support dynamic aliasing updates.

Accessibility accelerators are allowed to conflict with existing
browser shortcuts, so this change also adds that support.

Screenshot: https://screenshot.googleplex.com/3GiUgyHhA6A6QFg

Bug: b/216049298
Test: ash_webui_unittest
Change-Id: I5272efdc769c675ec6599b2747ddf68a560f5844
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5231026
Reviewed-by: Longbo Wei <longbowei@google.com>
Commit-Queue: Jimmy Gong <jimmyxgong@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1253472}
This commit is contained in:
Jimmy
2024-01-29 20:20:10 +00:00
committed by Chromium LUCI CQ
parent 64bfcce29b
commit 70f49b8196
8 changed files with 100 additions and 32 deletions

@ -4480,6 +4480,12 @@ Some features are limited to increase battery life.
<message name="IDS_AMBIENT_ACCELERATOR_DESCRIPTION_SWITCH_FOCUS" desc="Label for accelerator description - Move between bottom right corner, Launcher, address bar, bookmarks bar, webpage that's open, and downloads.">
Move between bottom right corner, Launcher, address bar, bookmarks bar, website that's open, and downloads
</message>
<message name="IDS_AMBIENT_ACCELERATOR_DESCRIPTION_SWITCH_FOCUS_FORWARDS" desc="Label for accelerator description - Move forwards between bottom right corner, Launcher, address bar, bookmarks bar, webpage that's open, and downloads.">
Move forwards between bottom right corner, Launcher, address bar, bookmarks bar, website that's open, and downloads
</message>
<message name="IDS_AMBIENT_ACCELERATOR_DESCRIPTION_SWITCH_FOCUS_BACKWARDS" desc="Label for accelerator description - Move backwards between bottom right corner, Launcher, address bar, bookmarks bar, webpage that's open, and downloads.">
Move backwards between bottom right corner, Launcher, address bar, bookmarks bar, website that's open, and downloads
</message>
<message name="IDS_AMBIENT_ACCELERATOR_DESCRIPTION_COPY" desc="Label for accelerator description - Copy selected content to clipboard.">
Copy selected content to clipboard
</message>

@ -0,0 +1 @@
552a33dc4c30101bbe14781037fee107ac423f64

@ -0,0 +1 @@
552a33dc4c30101bbe14781037fee107ac423f64

@ -554,7 +554,6 @@ AcceleratorConfigurationProvider::AcceleratorConfigurationProvider(
weak_ptr_factory_.GetWeakPtr()));
UpdateKeyboards();
InitializeNonConfigurableAccelerators(GetNonConfigurableActionsMap());
// Create LayoutInfos from kAcceleratorLayouts. LayoutInfos are static
// data that provides additional details for the app for styling.
@ -573,6 +572,10 @@ AcceleratorConfigurationProvider::AcceleratorConfigurationProvider(
accelerator_layout_lookup_[GetUuid(layout->source, layout->action_id)] =
*layout;
}
// Must initialize the non-configurable accelerators after the layout
// has been set.
InitializeNonConfigurableAccelerators(GetNonConfigurableActionsMap());
}
AcceleratorConfigurationProvider::~AcceleratorConfigurationProvider() {
@ -668,15 +671,16 @@ void AcceleratorConfigurationProvider::GetConflictAccelerator(
// Check if `accelerator` conflicts with non-configurable accelerators.
// This includes: browser, accessbility, and ambient accelerators.
const uint32_t* non_configurable_conflict_id =
non_configurable_accelerator_to_id_.Find(accelerator);
const std::vector<uint32_t> non_configurable_conflict_ids =
FindNonConfigurableIdFromAccelerator(accelerator);
// If there was a conflict with a non-configurable accelerator
if (non_configurable_conflict_id) {
if (!non_configurable_conflict_ids.empty()) {
result_data->result = AcceleratorConfigResult::kConflict;
// Get the shortcut name and add it to the return struct.
result_data->shortcut_name = l10n_util::GetStringUTF16(
accelerator_layout_lookup_[GetUuid(mojom::AcceleratorSource::kAmbient,
*non_configurable_conflict_id)]
accelerator_layout_lookup_[GetUuid(
mojom::AcceleratorSource::kAmbient,
non_configurable_conflict_ids.front())]
.description_string_id);
std::move(callback).Run(std::move(result_data));
VLOG(1) << "Attempted to add accelerator: " << accelerator.GetShortcutText()
@ -1145,11 +1149,20 @@ void AcceleratorConfigurationProvider::InitializeNonConfigurableAccelerators(
if (accelerators_details.IsStandardAccelerator()) {
DCHECK(!accelerators_details.replacements.has_value());
DCHECK(!accelerators_details.message_id.has_value());
const AcceleratorLayoutDetails& layout =
accelerator_layout_lookup_[GetUuid(mojom::AcceleratorSource::kAmbient,
ambient_action_id)];
for (const auto& accelerator :
accelerators_details.accelerators.value()) {
const uint32_t action_id = static_cast<uint32_t>(ambient_action_id);
non_configurable_accelerator_to_id_.InsertNew(
std::make_pair(accelerator, action_id));
// Store accessibility lookups separately.
if (layout.category == mojom::AcceleratorCategory::kAccessibility) {
accessibility_accelerator_to_id_.InsertNew(
std::make_pair(accelerator, action_id));
} else {
non_configurable_accelerator_to_id_.InsertNew(
std::make_pair(accelerator, action_id));
}
id_to_non_configurable_accelerators_[action_id].push_back(accelerator);
}
}
@ -1247,16 +1260,18 @@ AcceleratorConfigurationProvider::PreprocessAddAccelerator(
// Check if `accelerator` conflicts with non-configurable accelerators.
// This includes: browser, accessbility, and ambient accelerators.
const uint32_t* non_configurable_conflict_id =
non_configurable_accelerator_to_id_.Find(accelerator);
// If there was a conflict with a non-configurable accelerator
if (non_configurable_conflict_id) {
const std::vector<uint32_t> non_configurable_conflict_ids =
FindNonConfigurableIdFromAccelerator(accelerator);
// If there was a conflict with a non-configurable accelerator, return
// just one of the conflict id's.
if (!non_configurable_conflict_ids.empty()) {
pending_accelerator_.reset();
result_data->result = AcceleratorConfigResult::kConflict;
// Get the shortcut name and add it to the return struct.
result_data->shortcut_name = l10n_util::GetStringUTF16(
accelerator_layout_lookup_[GetUuid(mojom::AcceleratorSource::kAmbient,
*non_configurable_conflict_id)]
accelerator_layout_lookup_[GetUuid(
mojom::AcceleratorSource::kAmbient,
non_configurable_conflict_ids.front())]
.description_string_id);
return result_data;
}
@ -1364,6 +1379,29 @@ AcceleratorConfigurationProvider::MaybeHandleNonSearchAccelerator(
return AcceleratorConflictErrorState::kStandby;
}
std::vector<uint32_t>
AcceleratorConfigurationProvider::FindNonConfigurableIdFromAccelerator(
const ui::Accelerator& accelerator) {
std::vector<uint32_t> ids;
// Check browser/text non-configurable accelerators first.
uint32_t* non_configurable_conflict_id =
non_configurable_accelerator_to_id_.Find(accelerator);
if (non_configurable_conflict_id) {
ids.push_back(*non_configurable_conflict_id);
}
// Then check accessibility accelerators.
non_configurable_conflict_id =
accessibility_accelerator_to_id_.Find(accelerator);
if (non_configurable_conflict_id) {
ids.push_back(*non_configurable_conflict_id);
}
return ids;
}
void AcceleratorConfigurationProvider::SetLayoutDetailsMapForTesting(
const std::vector<AcceleratorLayoutDetails>& layouts) {
accelerator_layout_lookup_.clear();

@ -194,6 +194,8 @@ class AcceleratorConfigurationProvider
SetLayoutDetailsMapForTesting);
friend class AcceleratorConfigurationProviderTest;
using NonConfigAcceleratorActionMap = ui::AcceleratorMap<AcceleratorActionId>;
using AccessibilityAcceleratorActionMap =
ui::AcceleratorMap<AcceleratorActionId>;
// Represents the different states the current pending accelerator can be
// in.
@ -263,6 +265,9 @@ class AcceleratorConfigurationProvider
mojom::AcceleratorSource source,
AcceleratorActionId action_id);
std::vector<uint32_t> FindNonConfigurableIdFromAccelerator(
const ui::Accelerator& accelerator);
void SetLayoutDetailsMapForTesting(
const std::vector<AcceleratorLayoutDetails>& layouts);
@ -307,6 +312,11 @@ class AcceleratorConfigurationProvider
// standard non-configurable accelerators.
NonConfigAcceleratorActionMap non_configurable_accelerator_to_id_;
// A map from accelerators to accessibility actions, used as a reverse lookup.
// This is separate from `non_configurable_accelerator_to_id_` since shortcuts
// between browser and accessibility are allowed to overlap.
AccessibilityAcceleratorActionMap accessibility_accelerator_to_id_;
std::unique_ptr<PendingAccelerator> pending_accelerator_;
AcceleratorConflictErrorState conflict_error_state_ =

@ -461,8 +461,9 @@ class AcceleratorConfigurationProviderTest : public AshTestBase {
return accelerator_iter->second;
}
uint32_t GetNonConfigurableIdFromAccelerator(ui::Accelerator accelerator) {
return provider_->non_configurable_accelerator_to_id_.Get(accelerator);
std::vector<uint32_t> GetNonConfigurableIdFromAccelerator(
ui::Accelerator accelerator) {
return provider_->FindNonConfigurableIdFromAccelerator(accelerator);
}
std::unique_ptr<AcceleratorConfigurationProvider> provider_;
@ -1198,9 +1199,10 @@ TEST_F(AcceleratorConfigurationProviderTest, NonConfigurableReverseLookup) {
if (accelerators_details.IsStandardAccelerator()) {
for (const auto& accelerator :
accelerators_details.accelerators.value()) {
const uint32_t found_id =
std::vector<uint32_t> found_ids =
GetNonConfigurableIdFromAccelerator(accelerator);
EXPECT_EQ(ambient_action_id, found_id);
ASSERT_FALSE(found_ids.empty());
EXPECT_TRUE(base::Contains(found_ids, ambient_action_id));
}
}
}

@ -136,14 +136,6 @@ const NonConfigurableActionsMap& GetNonConfigurableActionsMap() {
TextAcceleratorPart(ui::EF_SHIFT_DOWN),
TextAcceleratorPart(ui::KeyboardCode::VKEY_L),
TextAcceleratorPart(ui::KeyboardCode::VKEY_ESCAPE)})},
{NonConfigurableActions::kAmbientSwitchFocus,
NonConfigurableAcceleratorDetails(
IDS_AMBIENT_ACCELERATOR_SWITCH_FOCUS,
{TextAcceleratorPart(ui::EF_CONTROL_DOWN),
TextAcceleratorPart(ui::KeyboardCode::VKEY_BROWSER_BACK),
TextAcceleratorPart(ui::EF_CONTROL_DOWN),
TextAcceleratorPart(ui::EF_SHIFT_DOWN),
TextAcceleratorPart(ui::KeyboardCode::VKEY_BROWSER_BACK)})},
{NonConfigurableActions::kAmbientMoveAppsInGrid,
NonConfigurableAcceleratorDetails(
IDS_AMBIENT_ACCELERATOR_MOVE_APPS_IN_GRID,
@ -416,6 +408,13 @@ const NonConfigurableActionsMap& GetNonConfigurableActionsMap() {
NonConfigurableAcceleratorDetails(
{ui::Accelerator(ui::VKEY_BROWSER_REFRESH, ui::EF_NONE),
ui::Accelerator(ui::VKEY_R, ui::EF_CONTROL_DOWN)})},
{NonConfigurableActions::kAmbientSwitchFocusForwards,
NonConfigurableAcceleratorDetails(
{ui::Accelerator(ui::VKEY_BROWSER_BACK, ui::EF_CONTROL_DOWN)})},
{NonConfigurableActions::kAmbientSwitchFocusBackwards,
NonConfigurableAcceleratorDetails(
{ui::Accelerator(ui::VKEY_BROWSER_BACK,
ui::EF_CONTROL_DOWN | ui::EF_SHIFT_DOWN)})},
});
return *nonConfigurableActionsMap;
}
@ -1723,13 +1722,21 @@ const AcceleratorLayoutMap& GetAcceleratorLayoutMap() {
mojom::AcceleratorSubcategory::kVisibility,
/*locked=*/false, mojom::AcceleratorLayoutStyle::kDefault,
mojom::AcceleratorSource::kAsh)},
{NonConfigurableActions::kAmbientSwitchFocus,
{NonConfigurableActions::kAmbientSwitchFocusForwards,
AcceleratorLayoutDetails(
NonConfigurableActions::kAmbientSwitchFocus,
IDS_AMBIENT_ACCELERATOR_DESCRIPTION_SWITCH_FOCUS,
NonConfigurableActions::kAmbientSwitchFocusForwards,
IDS_AMBIENT_ACCELERATOR_DESCRIPTION_SWITCH_FOCUS_FORWARDS,
mojom::AcceleratorCategory::kAccessibility,
mojom::AcceleratorSubcategory::kAccessibilityNavigation,
/*locked=*/true, mojom::AcceleratorLayoutStyle::kText,
/*locked=*/true, mojom::AcceleratorLayoutStyle::kDefault,
mojom::AcceleratorSource::kAmbient)},
{NonConfigurableActions::kAmbientSwitchFocusBackwards,
AcceleratorLayoutDetails(
NonConfigurableActions::kAmbientSwitchFocusBackwards,
IDS_AMBIENT_ACCELERATOR_DESCRIPTION_SWITCH_FOCUS_BACKWARDS,
mojom::AcceleratorCategory::kAccessibility,
mojom::AcceleratorSubcategory::kAccessibilityNavigation,
/*locked=*/true, mojom::AcceleratorLayoutStyle::kDefault,
mojom::AcceleratorSource::kAmbient)},
{NonConfigurableActions::kAmbientCaretBrowsing,
AcceleratorLayoutDetails(

@ -135,7 +135,7 @@ enum NonConfigurableActions {
kAmbientDisplayHiddenFiles,
kAmbientOpenRightClickMenu,
kAmbientCaretBrowsing,
kAmbientSwitchFocus,
kAmbientSwitchFocus, // DEPRECATED
kAmbientCopy,
kAmbientCut,
kAmbientPaste,
@ -155,6 +155,8 @@ enum NonConfigurableActions {
kAmbientGoToEndOfLine,
kAmbientMoveStartOfPreviousWord,
kAmbientMoveToEndOfWord,
kAmbientSwitchFocusForwards,
kAmbientSwitchFocusBackwards,
};
// Contains details for UI styling of an accelerator.
@ -478,7 +480,8 @@ inline constexpr uint32_t kAcceleratorLayouts[] = {
AcceleratorAction::kMagnifierZoomOut,
// Accessibility > Accessbility navigation
NonConfigurableActions::kAmbientSwitchFocus,
NonConfigurableActions::kAmbientSwitchFocusForwards,
NonConfigurableActions::kAmbientSwitchFocusBackwards,
NonConfigurableActions::kAmbientCaretBrowsing,
AcceleratorAction::kFocusShelf,
NonConfigurableActions::kAmbientHighlightNextItemOnShelf,