Skip to content

Commit

Permalink
Revert "Allow chromevox selection in Chrome OS launcher search"
Browse files Browse the repository at this point in the history
This reverts commit 6547ab3.

Reason for revert: failing on linux-chromeos-chrome.
  See https://crbug.com/1281791

Original change's description:
> Allow chromevox selection in Chrome OS launcher search
>
> Chrome OS launcher search results UI uses selection to verbalize the
> selected launcher search result (similar to omnibox).
>
> BUG=1276247
>
> AX-Relnotes: Fix verbalization for results in Chrome OS launcher search
>
> Change-Id: I4264e5940a0a3469ffd23df83441420eb0ba8fda
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3346381
> Reviewed-by: Rachel Wong <wrong@chromium.org>
> Reviewed-by: David Tseng <dtseng@chromium.org>
> Reviewed-by: Yulun Wu <yulunwu@chromium.org>
> Commit-Queue: Toni Barzic <tbarzic@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#953073}

Bug: 1276247, 1281791
Change-Id: Ieee01713ddca22b251bf9b674016ae405d0cfaf1
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3347019
Auto-Submit: Austin Tankiang <austinct@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Austin Tankiang <austinct@chromium.org>
Owners-Override: Austin Tankiang <austinct@chromium.org>
Cr-Commit-Position: refs/heads/main@{#953137}
  • Loading branch information
Austin Tankiang authored and Chromium LUCI CQ committed Dec 21, 2021
1 parent b2998a0 commit 6e05cca
Show file tree
Hide file tree
Showing 12 changed files with 43 additions and 504 deletions.
19 changes: 10 additions & 9 deletions ash/app_list/views/productivity_launcher_search_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
#include "base/notreached.h"
#include "base/strings/string_number_conversions.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
#include "ui/accessibility/platform/ax_unique_id.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/base/metadata/metadata_impl_macros.h"
#include "ui/gfx/geometry/rect.h"
Expand Down Expand Up @@ -252,20 +251,22 @@ void ProductivityLauncherSearchView::MaybeNotifySelectedResultChanged() {
if (ignore_result_changes_for_a11y_)
return;

if (!result_selection_controller_->selected_result()) {
search_box_view_->SetA11yActiveDescendant(absl::nullopt);
// Ignore result selection change if the focus moved away from the search box
// textfield, for example to the close button.
if (!search_box_view_->search_box()->HasFocus())
return;

if (!result_selection_controller_->selected_result())
return;
}

views::View* selected_view =
result_selection_controller_->selected_result()->GetSelectedView();
if (!selected_view) {
search_box_view_->SetA11yActiveDescendant(absl::nullopt);
if (!selected_view)
return;
}

search_box_view_->SetA11yActiveDescendant(
selected_view->GetViewAccessibility().GetUniqueId().Get());
selected_view->NotifyAccessibilityEvent(ax::mojom::Event::kSelection, true);
NotifyAccessibilityEvent(ax::mojom::Event::kSelectedChildrenChanged, true);
search_box_view_->set_a11y_selection_on_search_result(true);
}

bool ProductivityLauncherSearchView::CanSelectSearchResults() {
Expand Down
12 changes: 6 additions & 6 deletions ash/app_list/views/productivity_launcher_search_view_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -518,22 +518,22 @@ TEST_P(ProductivityLauncherSearchViewTest, SearchResultA11y) {
// Pressing down should not generate a selection accessibility event because
// A11Y announcements are delayed since the results list just changed.
PressAndReleaseKey(ui::VKEY_DOWN);
EXPECT_EQ(0, ax_counter.GetCount(ax::mojom::Event::kActiveDescendantChanged));
EXPECT_EQ(0, ax_counter.GetCount(ax::mojom::Event::kSelection));
// Advance time to fire the timer to stop ignoring A11Y announcements.
task_environment()->FastForwardBy(base::Milliseconds(5000));

// A selection event is generated when the timer fires.
EXPECT_EQ(1, ax_counter.GetCount(ax::mojom::Event::kActiveDescendantChanged));
EXPECT_EQ(1, ax_counter.GetCount(ax::mojom::Event::kSelection));

// Successive up/down key presses should generate additional selection events.
PressAndReleaseKey(ui::VKEY_DOWN);
EXPECT_EQ(2, ax_counter.GetCount(ax::mojom::Event::kActiveDescendantChanged));
EXPECT_EQ(2, ax_counter.GetCount(ax::mojom::Event::kSelection));
PressAndReleaseKey(ui::VKEY_UP);
EXPECT_EQ(3, ax_counter.GetCount(ax::mojom::Event::kActiveDescendantChanged));
EXPECT_EQ(3, ax_counter.GetCount(ax::mojom::Event::kSelection));
PressAndReleaseKey(ui::VKEY_DOWN);
EXPECT_EQ(4, ax_counter.GetCount(ax::mojom::Event::kActiveDescendantChanged));
EXPECT_EQ(4, ax_counter.GetCount(ax::mojom::Event::kSelection));
PressAndReleaseKey(ui::VKEY_DOWN);
EXPECT_EQ(5, ax_counter.GetCount(ax::mojom::Event::kActiveDescendantChanged));
EXPECT_EQ(5, ax_counter.GetCount(ax::mojom::Event::kSelection));
}

TEST_P(ProductivityLauncherSearchViewTest, SearchPageA11y) {
Expand Down
27 changes: 8 additions & 19 deletions ash/app_list/views/search_box_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -144,14 +144,6 @@ void SearchBoxView::ResetForShow() {
}
}

void SearchBoxView::UpdateSearchTextfieldAccessibleNodeData(
ui::AXNodeData* node_data) {
if (a11y_active_descendant_) {
node_data->AddIntAttribute(ax::mojom::IntAttribute::kActivedescendantId,
*a11y_active_descendant_);
}
}

void SearchBoxView::ClearSearch() {
SearchBoxViewBase::ClearSearch();
current_query_.clear();
Expand Down Expand Up @@ -605,8 +597,10 @@ void SearchBoxView::ClearAutocompleteText() {
}

void SearchBoxView::OnBeforeUserAction(views::Textfield* sender) {
if (a11y_active_descendant_)
SetA11yActiveDescendant(absl::nullopt);
if (a11y_selection_on_search_result_) {
a11y_selection_on_search_result_ = false;
search_box()->NotifyAccessibilityEvent(ax::mojom::Event::kSelection, true);
}
}

void SearchBoxView::ContentsChanged(views::Textfield* sender,
Expand Down Expand Up @@ -679,18 +673,11 @@ void SearchBoxView::ClearSearchAndDeactivateSearchBox() {
if (!is_search_box_active())
return;

SetA11yActiveDescendant(absl::nullopt);
a11y_selection_on_search_result_ = false;
ClearSearch();
SetSearchBoxActive(false, ui::ET_UNKNOWN);
}

void SearchBoxView::SetA11yActiveDescendant(
const absl::optional<int32_t>& active_descendant) {
a11y_active_descendant_ = active_descendant;
search_box()->NotifyAccessibilityEvent(
ax::mojom::Event::kActiveDescendantChanged, true);
}

bool SearchBoxView::HandleKeyEvent(views::Textfield* sender,
const ui::KeyEvent& key_event) {
DCHECK(result_selection_controller_);
Expand Down Expand Up @@ -812,7 +799,9 @@ bool SearchBoxView::HandleKeyEvent(views::Textfield* sender,

DCHECK(close_button()->GetVisible());
close_button()->RequestFocus();
SetA11yActiveDescendant(absl::nullopt);
close_button()->NotifyAccessibilityEvent(ax::mojom::Event::kSelection,
true);
a11y_selection_on_search_result_ = false;
break;
case ResultSelectionController::MoveResult::kResultChanged:
UpdateSearchBoxTextForSelectedResult(
Expand Down
21 changes: 6 additions & 15 deletions ash/app_list/views/search_box_view.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
#ifndef ASH_APP_LIST_VIEWS_SEARCH_BOX_VIEW_H_
#define ASH_APP_LIST_VIEWS_SEARCH_BOX_VIEW_H_

#include <stdint.h>

#include <vector>

#include "ash/app_list/app_list_model_provider.h"
Expand All @@ -17,7 +15,6 @@
#include "ash/public/cpp/app_list/app_list_types.h"
#include "ash/search_box/search_box_view_base.h"
#include "base/scoped_observation.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

namespace views {
class Textfield;
Expand Down Expand Up @@ -65,8 +62,6 @@ class ASH_EXPORT SearchBoxView : public SearchBoxViewBase,

// Overridden from SearchBoxViewBase:
void Init(const InitParams& params) override;
void UpdateSearchTextfieldAccessibleNodeData(
ui::AXNodeData* node_data) override;
void ClearSearch() override;
void HandleSearchBoxEvent(ui::LocatedEvent* located_event) override;
void UpdateKeyboardVisibility() override;
Expand Down Expand Up @@ -120,18 +115,15 @@ class ASH_EXPORT SearchBoxView : public SearchBoxViewBase,
// Clears the search query and de-activate the search box.
void ClearSearchAndDeactivateSearchBox();

// Sets the view accessibility ID of the search box's active descendant.
// The active descendant should be the currently selected result view in the
// search results list.
// `nullopt` indicates no active descendant, i.e. that no result is selected.
void SetA11yActiveDescendant(
const absl::optional<int32_t>& active_descendant);

void set_contents_view(ContentsView* contents_view) {
contents_view_ = contents_view;
}
ContentsView* contents_view() { return contents_view_; }

void set_a11y_selection_on_search_result(bool value) {
a11y_selection_on_search_result_ = value;
}

ResultSelectionController* result_selection_controller_for_test() {
return result_selection_controller_;
}
Expand Down Expand Up @@ -218,9 +210,8 @@ class ASH_EXPORT SearchBoxView : public SearchBoxViewBase,
bool is_tablet_mode_;

// Set by SearchResultPageView when the accessibility selection moves to a
// search result view - the value is the ID of the currently selected result
// view.
absl::optional<int32_t> a11y_active_descendant_;
// search result view.
bool a11y_selection_on_search_result_ = false;

// Owned by SearchResultPageView (for fullscreen launcher) or
// ProductivityLauncherSearchPage (for bubble launcher).
Expand Down
22 changes: 11 additions & 11 deletions ash/app_list/views/search_result_page_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
#include "base/memory/ptr_util.h"
#include "base/strings/string_number_conversions.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
#include "ui/accessibility/platform/ax_unique_id.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/base/metadata/metadata_header_macros.h"
#include "ui/base/metadata/metadata_impl_macros.h"
Expand Down Expand Up @@ -432,25 +431,26 @@ void SearchResultPageView::NotifyA11yResultsChanged() {
void SearchResultPageView::NotifySelectedResultChanged() {
// Result selection should be handled by |productivity_launcher_search_page_|.
DCHECK(!features::IsProductivityLauncherEnabled());
if (ignore_result_changes_for_a11y_)
if (ignore_result_changes_for_a11y_ ||
!result_selection_controller_->selected_location_details() ||
!result_selection_controller_->selected_result()) {
return;
}

SearchBoxView* search_box = AppListPage::contents_view()->GetSearchBoxView();
if (!result_selection_controller_->selected_location_details() ||
!result_selection_controller_->selected_result()) {
search_box->SetA11yActiveDescendant(absl::nullopt);
// Ignore result selection change if the focus moved away from the search boc
// textfield, for example to the close button.
if (!search_box->search_box()->HasFocus())
return;
}

views::View* selected_view =
result_selection_controller_->selected_result()->GetSelectedView();
if (!selected_view) {
search_box->SetA11yActiveDescendant(absl::nullopt);
if (!selected_view)
return;
}

search_box->SetA11yActiveDescendant(
selected_view->GetViewAccessibility().GetUniqueId().Get());
selected_view->NotifyAccessibilityEvent(ax::mojom::Event::kSelection, true);
NotifyAccessibilityEvent(ax::mojom::Event::kSelectedChildrenChanged, true);
search_box->set_a11y_selection_on_search_result(true);
}

void SearchResultPageView::OnActiveAppListModelsChanged(
Expand Down
1 change: 1 addition & 0 deletions ash/app_list/views/search_result_tile_item_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ void SearchResultTileItemView::GetAccessibleNodeData(

// The tile is a list item in the search result page's result list.
node_data->role = ax::mojom::Role::kListBoxOption;
node_data->AddBoolAttribute(ax::mojom::BoolAttribute::kSelected, selected());
node_data->SetDefaultActionVerb(ax::mojom::DefaultActionVerb::kClick);

// Specify |ax::mojom::StringAttribute::kDescription| with an empty string, so
Expand Down
1 change: 1 addition & 0 deletions ash/app_list/views/search_result_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,7 @@ void SearchResultView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
// button are child button of SearchResultView), which is not supported by
// ChromeVox. see details in crbug.com/924776.
node_data->role = ax::mojom::Role::kListBoxOption;
node_data->AddBoolAttribute(ax::mojom::BoolAttribute::kSelected, selected());
node_data->SetDefaultActionVerb(ax::mojom::DefaultActionVerb::kClick);
node_data->SetName(GetAccessibleName());
}
Expand Down
8 changes: 0 additions & 8 deletions ash/search_box/search_box_view_base.cc
Original file line number Diff line number Diff line change
Expand Up @@ -251,11 +251,6 @@ class SearchBoxTextfield : public views::Textfield {
}
}

void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
views::Textfield::GetAccessibleNodeData(node_data);
search_box_view_->UpdateSearchTextfieldAccessibleNodeData(node_data);
}

private:
SearchBoxViewBase* const search_box_view_;
};
Expand Down Expand Up @@ -526,9 +521,6 @@ bool SearchBoxViewBase::IsSearchBoxTrimmedQueryEmpty() const {
return trimmed_query.empty();
}

void SearchBoxViewBase::UpdateSearchTextfieldAccessibleNodeData(
ui::AXNodeData* node_data) {}

void SearchBoxViewBase::ClearSearch() {
// Avoid setting |search_box_| text to empty if it is already empty.
if (search_box_->GetText() == std::u16string())
Expand Down
3 changes: 0 additions & 3 deletions ash/search_box/search_box_view_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,6 @@ class SearchBoxViewBase : public views::View,
// Whether the trimmed query in the search box is empty.
bool IsSearchBoxTrimmedQueryEmpty() const;

virtual void UpdateSearchTextfieldAccessibleNodeData(
ui::AXNodeData* node_data);

virtual void ClearSearch();

// Called when the search box active state changes.
Expand Down
Loading

0 comments on commit 6e05cca

Please sign in to comment.