Skip to content

Commit

Permalink
Allow to inject accessibility information into PopupCellView.
Browse files Browse the repository at this point in the history 
Since a11y information in views is not exposed via setters/getters,
but instead is passed by overriding GetAccessibleNodeData(),
this CL introduces a simple delegate for filling out node data.

It will be used to differentiate a11y data between Autofill items
and control elements.

Bug: 1417187
Change-Id: I1c533b2b36a8105e8fbfc98d3e1e4810200e131a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4264711
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Jan Keitel <jkeitel@google.com>
Cr-Commit-Position: refs/heads/main@{#1107609}
  • Loading branch information
Jan Keitel authored and Chromium LUCI CQ committed Feb 21, 2023
1 parent 0f53601 commit 672dd9d
Show file tree
Hide file tree
Showing 9 changed files with 270 additions and 136 deletions.
32 changes: 7 additions & 25 deletions chrome/browser/ui/views/autofill/popup/popup_cell_view.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@

#include "chrome/browser/ui/views/autofill/popup/popup_cell_view.h"

#include <string>
#include <memory>
#include <utility>

#include "base/functional/callback.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
Expand Down Expand Up @@ -42,16 +43,9 @@ void PopupCellView::SetSelected(bool selected) {
}
}

void PopupCellView::SetVoiceOverString(std::u16string voice_over) {
voice_over_ = std::move(voice_over);
}

void PopupCellView::SetSetSizeForAccessibility(absl::optional<int> set_size) {
set_size_ = set_size;
}

void PopupCellView::SetSetIndexForAccessibility(absl::optional<int> set_index) {
set_index_ = set_index;
void PopupCellView::SetAccessibilityDelegate(
std::unique_ptr<AccessibilityDelegate> a11y_delegate) {
a11y_delegate_ = std::move(a11y_delegate);
}

void PopupCellView::SetOnEnteredCallback(base::RepeatingClosure callback) {
Expand Down Expand Up @@ -157,17 +151,8 @@ bool PopupCellView::HandleAccessibleAction(
}

void PopupCellView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
// Options are selectable.
node_data->role = ax::mojom::Role::kListBoxOption;
node_data->AddBoolAttribute(ax::mojom::BoolAttribute::kSelected,
GetSelected());
node_data->SetNameChecked(GetVoiceOverString());

if (set_size_) {
node_data->AddIntAttribute(ax::mojom::IntAttribute::kSetSize, *set_size_);
}
if (set_index_) {
node_data->AddIntAttribute(ax::mojom::IntAttribute::kPosInSet, *set_index_);
if (a11y_delegate_) {
a11y_delegate_->GetAccessibleNodeData(GetSelected(), node_data);
}
}

Expand Down Expand Up @@ -203,9 +188,6 @@ void PopupCellView::RefreshStyle() {

BEGIN_METADATA(PopupCellView, views::View)
ADD_PROPERTY_METADATA(bool, Selected)
ADD_PROPERTY_METADATA(std::u16string, VoiceOverString)
ADD_PROPERTY_METADATA(absl::optional<int>, SetSizeForAccessibility)
ADD_PROPERTY_METADATA(absl::optional<int>, SetIndexForAccessibility)
ADD_PROPERTY_METADATA(base::RepeatingClosure, OnEnteredCallback)
ADD_PROPERTY_METADATA(base::RepeatingClosure, OnExitedCallback)
ADD_PROPERTY_METADATA(base::RepeatingClosure, OnAcceptedCallback)
Expand Down
46 changes: 22 additions & 24 deletions chrome/browser/ui/views/autofill/popup/popup_cell_view.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#ifndef CHROME_BROWSER_UI_VIEWS_AUTOFILL_POPUP_POPUP_CELL_VIEW_H_
#define CHROME_BROWSER_UI_VIEWS_AUTOFILL_POPUP_POPUP_CELL_VIEW_H_

#include <memory>
#include <string>

#include "base/functional/callback.h"
Expand All @@ -28,6 +29,19 @@ namespace autofill {
// information.
class PopupCellView : public views::View {
public:
// Interface for injecting accessibility data into `PopupCellView`. This
// allows to have `PopupCellViews` with different a11y roles without needing
// to subclass them.
class AccessibilityDelegate {
public:
virtual ~AccessibilityDelegate() = default;

// Sets the a11y information in `node_data` based on whether the cell in
// question `is_selected` or not.
virtual void GetAccessibleNodeData(bool is_selected,
ui::AXNodeData* node_data) const = 0;
};

METADATA_HEADER(PopupCellView);

PopupCellView();
Expand All @@ -39,16 +53,10 @@ class PopupCellView : public views::View {
bool GetSelected() const { return selected_; }
void SetSelected(bool selected);

// Gets and sets the string announced by VoiceOver.
const std::u16string& GetVoiceOverString() const { return voice_over_; }
void SetVoiceOverString(std::u16string voice_over);

// Gets and sets additional (optional) accessibility information. See the
// member definition for more information.
absl::optional<int> GetSetSizeForAccessibility() const { return set_size_; }
void SetSetSizeForAccessibility(absl::optional<int> set_size);
absl::optional<int> GetSetIndexForAccessibility() const { return set_index_; }
void SetSetIndexForAccessibility(absl::optional<int> set_index);
// Sets the accessibility delegate that is consulted when providing accessible
// node data.
void SetAccessibilityDelegate(
std::unique_ptr<AccessibilityDelegate> a11y_delegate);

// Gets and sets the callback that is run when the cell is entered (via mouse
// or gesture event).
Expand Down Expand Up @@ -106,17 +114,8 @@ class PopupCellView : public views::View {

// The selection state.
bool selected_ = false;
// The string announced by VoiceOver.
std::u16string voice_over_;

// Additional information set for a11y purposes. The `set_size_` is the number
// of non-separator suggestions and `set_index_` is this element's (1-indexed)
// position in it.
// TODO(crbug.com/1411172): Move to subclasses once `PopupStrategy` exists
// and uses them since in the future not every `PopupCellView` may be a
// `ListBoxOption`.
absl::optional<int> set_index_;
absl::optional<int> set_size_;
// The accessibility delegate.
std::unique_ptr<AccessibilityDelegate> a11y_delegate_;

base::RepeatingClosure on_entered_callback_;
base::RepeatingClosure on_exited_callback_;
Expand All @@ -137,9 +136,8 @@ class PopupCellView : public views::View {
};

BEGIN_VIEW_BUILDER(/* no export*/, PopupCellView, views::View)
VIEW_BUILDER_PROPERTY(std::u16string, VoiceOverString)
VIEW_BUILDER_PROPERTY(absl::optional<int>, SetSizeForAccessibility)
VIEW_BUILDER_PROPERTY(absl::optional<int>, SetIndexForAccessibility)
VIEW_BUILDER_PROPERTY(std::unique_ptr<PopupCellView::AccessibilityDelegate>,
AccessibilityDelegate)
VIEW_BUILDER_PROPERTY(base::RepeatingClosure, OnEnteredCallback)
VIEW_BUILDER_PROPERTY(base::RepeatingClosure, OnExitedCallback)
VIEW_BUILDER_PROPERTY(base::RepeatingClosure, OnAcceptedCallback)
Expand Down
41 changes: 22 additions & 19 deletions chrome/browser/ui/views/autofill/popup/popup_cell_view_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "base/memory/raw_ptr.h"
#include "base/test/mock_callback.h"
#include "build/build_config.h"
#include "chrome/browser/ui/views/autofill/popup/test_popup_row_strategy.h"
#include "chrome/test/views/chrome_views_test_base.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
Expand Down Expand Up @@ -78,32 +79,22 @@ class PopupCellViewTest : public ChromeViewsTestBase {
};

TEST_F(PopupCellViewTest, AccessibleNodeData) {
constexpr char16_t kVoiceOverName[] = u"Sample voice over name";
constexpr absl::optional<int> kSetSize{5};
constexpr absl::optional<int> kSetIndex{3};
ShowView(views::Builder<PopupCellView>()
.SetVoiceOverString(kVoiceOverName)
.SetSetSizeForAccessibility(kSetSize)
.SetSetIndexForAccessibility(kSetIndex)
.SetAccessibilityDelegate(
std::make_unique<TestAccessibilityDelegate>())
.Build());

ui::AXNodeData node_data;
view().GetAccessibleNodeData(&node_data);

EXPECT_EQ(ax::mojom::Role::kListBoxOption, node_data.role);
EXPECT_FALSE(view().GetSelected());
EXPECT_FALSE(node_data.GetBoolAttribute(ax::mojom::BoolAttribute::kSelected));
EXPECT_EQ(kVoiceOverName,
EXPECT_EQ(TestAccessibilityDelegate::kVoiceOverName,
node_data.GetString16Attribute(ax::mojom::StringAttribute::kName));
EXPECT_EQ(kSetSize,
node_data.GetIntAttribute(ax::mojom::IntAttribute::kSetSize));
EXPECT_EQ(kSetIndex,
node_data.GetIntAttribute(ax::mojom::IntAttribute::kPosInSet));
}

TEST_F(PopupCellViewTest, SetSelectedUpdatesBackground) {
ShowView(views::Builder<PopupCellView>()
.SetVoiceOverString(u"Dummy name")
.SetAccessibilityDelegate(
std::make_unique<TestAccessibilityDelegate>())
.Build());

// The unselected background.
Expand All @@ -123,7 +114,10 @@ TEST_F(PopupCellViewTest, SetSelectedUpdatesBackground) {

TEST_F(PopupCellViewTest, SetSelectedUpdatesTrackedLabels) {
std::unique_ptr<PopupCellView> cell =
views::Builder<PopupCellView>().SetVoiceOverString(u"Dummy name").Build();
views::Builder<PopupCellView>()
.SetAccessibilityDelegate(
std::make_unique<TestAccessibilityDelegate>())
.Build();
views::Label* tracked_label =
cell->AddChildView(std::make_unique<views::Label>(
u"Label text 1", views::style::CONTEXT_DIALOG_BODY_TEXT,
Expand Down Expand Up @@ -163,7 +157,10 @@ TEST_F(PopupCellViewTest, SetSelectedUpdatesTrackedLabels) {

TEST_F(PopupCellViewTest, MouseEvents) {
std::unique_ptr<PopupCellView> cell =
views::Builder<PopupCellView>().SetVoiceOverString(u"Dummy name").Build();
views::Builder<PopupCellView>()
.SetAccessibilityDelegate(
std::make_unique<TestAccessibilityDelegate>())
.Build();
views::Label* label =
cell->AddChildView(std::make_unique<views::Label>(u"Label text"));
ShowView(std::move(cell));
Expand Down Expand Up @@ -196,7 +193,10 @@ TEST_F(PopupCellViewTest, MouseEvents) {
#if !BUILDFLAG(IS_MAC)
TEST_F(PopupCellViewTest, GestureEvents) {
std::unique_ptr<PopupCellView> cell =
views::Builder<PopupCellView>().SetVoiceOverString(u"Dummy name").Build();
views::Builder<PopupCellView>()
.SetAccessibilityDelegate(
std::make_unique<TestAccessibilityDelegate>())
.Build();
views::Label* label =
cell->AddChildView(std::make_unique<views::Label>(u"Label text"));
ShowView(std::move(cell));
Expand All @@ -217,7 +217,10 @@ TEST_F(PopupCellViewTest, GestureEvents) {

TEST_F(PopupCellViewTest, IgnoreClickIfMouseWasNotOutsideBefore) {
std::unique_ptr<PopupCellView> cell =
views::Builder<PopupCellView>().SetVoiceOverString(u"Dummy name").Build();
views::Builder<PopupCellView>()
.SetAccessibilityDelegate(
std::make_unique<TestAccessibilityDelegate>())
.Build();
views::Label* label =
cell->AddChildView(std::make_unique<views::Label>(u"Label text"));
ShowView(std::move(cell));
Expand Down
111 changes: 77 additions & 34 deletions chrome/browser/ui/views/autofill/popup/popup_row_strategy.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#include "components/omnibox/browser/vector_icons.h"
#include "components/strings/grit/components_strings.h"
#include "components/vector_icons/vector_icons.h"
#include "ui/accessibility/ax_enums.mojom.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/base/models/image_model.h"
#include "ui/color/color_id.h"
Expand Down Expand Up @@ -435,44 +436,80 @@ std::u16string GetVoiceOverStringFromSuggestion(const Suggestion& suggestion) {
return base::JoinString(text, u" ");
}

// Adds all relevant accessibility information to a content view.
void AddAccessibilityInformationToContentView(
// Adds the callbacks for the content area to `content_view`.
void AddCallbacksToContentView(
base::WeakPtr<AutofillPopupController> controller,
int line_number,
PopupCellView& content_view) {
content_view.SetOnSelectedCallback(base::BindRepeating(
&AutofillPopupController::SelectSuggestion, controller, line_number));
content_view.SetOnUnselectedCallback(base::BindRepeating(
&AutofillPopupController::SelectSuggestion, controller, absl::nullopt));
content_view.SetOnAcceptedCallback(base::BindRepeating(
&AutofillPopupController::AcceptSuggestion, controller, line_number,
/*show_threshold=*/kIgnoreEarlyClicksOnPopupDuration));
}

// ********************* AccessibilityDelegate implementations *****************

class ContentItemAccessibilityDelegate
: public PopupCellView::AccessibilityDelegate {
public:
// Creates an a11y delegate for the `line_number`. `controller` must not be
// null.
ContentItemAccessibilityDelegate(
base::WeakPtr<AutofillPopupController> controller,
int line_number);
~ContentItemAccessibilityDelegate() override = default;

void GetAccessibleNodeData(bool is_selected,
ui::AXNodeData* node_data) const override;

private:
// The string announced via VoiceOver.
std::u16string voice_over_string_;
// The number of suggestions in the popup and the (1-based) index of the
// suggestion this delegate belongs to.
int set_index_ = 0;
int set_size_ = 0;
};

ContentItemAccessibilityDelegate::ContentItemAccessibilityDelegate(
base::WeakPtr<AutofillPopupController> controller,
int line_number) {
DCHECK(controller);
content_view.SetVoiceOverString(GetVoiceOverStringFromSuggestion(
controller->GetSuggestionAt(line_number)));

int set_size = 0;
int set_index = line_number + 1;
voice_over_string_ = GetVoiceOverStringFromSuggestion(
controller->GetSuggestionAt(line_number));

set_size_ = 0;
set_index_ = line_number + 1;
for (int i = 0; i < controller->GetLineCount(); ++i) {
if (controller->GetSuggestionAt(i).frontend_id == POPUP_ITEM_ID_SEPARATOR) {
if (i < line_number) {
--set_index;
--set_index_;
}
} else {
++set_size;
++set_size_;
}
}
content_view.SetSetIndexForAccessibility(set_index);
content_view.SetSetSizeForAccessibility(set_size);
}

// Adds the callbacks for the content area to `content_view`.
void AddCallbacksToContentView(
base::WeakPtr<AutofillPopupController> controller,
int line_number,
PopupCellView& content_view) {
content_view.SetOnSelectedCallback(base::BindRepeating(
&AutofillPopupController::SelectSuggestion, controller, line_number));
content_view.SetOnUnselectedCallback(base::BindRepeating(
&AutofillPopupController::SelectSuggestion, controller, absl::nullopt));
content_view.SetOnAcceptedCallback(base::BindRepeating(
&AutofillPopupController::AcceptSuggestion, controller, line_number,
/*show_threshold=*/kIgnoreEarlyClicksOnPopupDuration));
void ContentItemAccessibilityDelegate::GetAccessibleNodeData(
bool is_selected,
ui::AXNodeData* node_data) const {
DCHECK(node_data);
// Options are selectable.
node_data->role = ax::mojom::Role::kListBoxOption;
node_data->AddBoolAttribute(ax::mojom::BoolAttribute::kSelected, is_selected);
node_data->SetNameChecked(voice_over_string_);

node_data->AddIntAttribute(ax::mojom::IntAttribute::kPosInSet, set_index_);
node_data->AddIntAttribute(ax::mojom::IntAttribute::kSetSize, set_size_);
}

// TODO(crbug.com/1417187): Add an a11y delegate for control buttons.

} // namespace

/**************************** PopupRowBaseStrategy ****************************/
Expand Down Expand Up @@ -506,11 +543,12 @@ std::unique_ptr<PopupCellView> PopupSuggestionStrategy::CreateContent() {
}
const Suggestion& kSuggestion =
GetController()->GetSuggestionAt(GetLineNumber());
auto view = std::make_unique<PopupCellView>();

// Prepare the a11y information.
AddAccessibilityInformationToContentView(GetController(), GetLineNumber(),
*view);
std::unique_ptr<PopupCellView> view =
views::Builder<PopupCellView>()
.SetAccessibilityDelegate(
std::make_unique<ContentItemAccessibilityDelegate>(
GetController(), GetLineNumber()))
.Build();

// Add the actual views.
std::unique_ptr<views::Label> main_text_label = CreateMainTextLabel(
Expand Down Expand Up @@ -606,10 +644,12 @@ PopupPasswordSuggestionStrategy::CreateContent() {

const Suggestion& kSuggestion =
GetController()->GetSuggestionAt(GetLineNumber());
auto view = std::make_unique<PopupCellView>();

AddAccessibilityInformationToContentView(GetController(), GetLineNumber(),
*view);
std::unique_ptr<PopupCellView> view =
views::Builder<PopupCellView>()
.SetAccessibilityDelegate(
std::make_unique<ContentItemAccessibilityDelegate>(
GetController(), GetLineNumber()))
.Build();

// Add the actual views.
std::unique_ptr<views::Label> main_text_label = CreateMainTextLabel(
Expand Down Expand Up @@ -682,9 +722,12 @@ std::unique_ptr<PopupCellView> PopupFooterStrategy::CreateContent() {

const Suggestion& kSuggestion =
GetController()->GetSuggestionAt(GetLineNumber());
auto view = std::make_unique<PopupCellView>();
AddAccessibilityInformationToContentView(GetController(), GetLineNumber(),
*view);
std::unique_ptr<PopupCellView> view =
views::Builder<PopupCellView>()
.SetAccessibilityDelegate(
std::make_unique<ContentItemAccessibilityDelegate>(
GetController(), GetLineNumber()))
.Build();

views::BoxLayout* layout_manager =
view->SetLayoutManager(std::make_unique<views::BoxLayout>(
Expand Down

0 comments on commit 672dd9d

Please sign in to comment.