Make tab group color picker accessible

This improves on the color picker's keyboard and screenreader behavior.

Bug: 989174
Change-Id: I3be7227afe3616636a311c66b3393683be65bd79
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1754273
Commit-Queue: Bret Sepulveda <bsep@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#687359}

chrome/browser/ui/views/tabs/color_picker_view.cc

@@ -9,6 +9,7 @@
 #include "base/bind.h"
 #include "base/callback.h"
 #include "chrome/browser/ui/views/chrome_layout_provider.h"
+#include "ui/accessibility/ax_node_data.h"
 #include "ui/gfx/canvas.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/button/button.h"
@@ -29,6 +30,7 @@ class ColorPickerElementView : public views::Button,
         color_name_(color_name) {
     DCHECK(selected_callback_);
 
+    SetFocusBehavior(views::View::FocusBehavior::ALWAYS);
     SetAccessibleName(color_name);
 
     SetBorder(
@@ -52,6 +54,23 @@ class ColorPickerElementView : public views::Button,
   bool selected() const { return selected_; }
 
   // views::Button:
+  bool IsGroupFocusTraversable() const override {
+    // Tab should only focus the selected element.
+    return false;
+  }
+
+  views::View* GetSelectedViewForGroup(int group) override {
+    DCHECK(parent());
+    return parent()->GetSelectedViewForGroup(group);
+  }
+
+  void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
+    views::Button::GetAccessibleNodeData(node_data);
+    node_data->role = ax::mojom::Role::kRadioButton;
+    node_data->SetCheckedState(selected() ? ax::mojom::CheckedState::kTrue
+                                          : ax::mojom::CheckedState::kFalse);
+  }
+
   base::string16 GetTooltipText(const gfx::Point& p) const override {
     return color_name_;
   }
@@ -118,6 +137,13 @@ ColorPickerView::ColorPickerView(
         color.first, color.second)));
   }
 
+  // Our children should take keyboard focus, not us.
+  SetFocusBehavior(views::View::FocusBehavior::NEVER);
+  for (View* view : elements_) {
+    // Group the colors so they can be navigated with arrows.
+    view->SetGroup(0);
+  }
+
   const int element_spacing = ChromeLayoutProvider::Get()->GetDistanceMetric(
       views::DISTANCE_RELATED_BUTTON_HORIZONTAL);
 
@@ -142,6 +168,14 @@ base::Optional<SkColor> ColorPickerView::GetSelectedColor() const {
   return base::nullopt;
 }
 
+views::View* ColorPickerView::GetSelectedViewForGroup(int group) {
+  for (ColorPickerElementView* element : elements_) {
+    if (element->selected())
+      return element;
+  }
+  return nullptr;
+}
+
 views::Button* ColorPickerView::GetElementAtIndexForTesting(int index) {
   DCHECK_GE(index, 0);
   DCHECK_LT(index, static_cast<int>(elements_.size()));

chrome/browser/ui/views/tabs/color_picker_view.h

@@ -32,6 +32,9 @@ class ColorPickerView : public views::View {
 
   base::Optional<SkColor> GetSelectedColor() const;
 
+  // views::View:
+  views::View* GetSelectedViewForGroup(int group) override;
+
   views::Button* GetElementAtIndexForTesting(int index);
 
  private:

chrome/browser/ui/views/tabs/color_picker_view_unittest.cc

@@ -15,8 +15,11 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/events/event.h"
+#include "ui/events/keycodes/dom/dom_codes.h"
+#include "ui/events/keycodes/keyboard_code_conversion.h"
 #include "ui/gfx/canvas.h"
 #include "ui/views/controls/button/button.h"
+#include "ui/views/focus/focus_manager.h"
 #include "ui/views/test/widget_test.h"
 #include "ui/views/widget/widget.h"
 
@@ -46,8 +49,7 @@ class ColorPickerViewTest : public ChromeViewsTestBase {
     ChromeViewsTestBase::TearDown();
   }
 
-  void ClickColorAtIndex(int index) {
-    views::Button* element = color_picker_->GetElementAtIndexForTesting(index);
+  void ClickColorElement(views::Button* element) {
     gfx::Point center = element->GetLocalBounds().CenterPoint();
     gfx::Point root_center = center;
     views::View::ConvertPointToWidget(color_picker_, &root_center);
@@ -63,6 +65,10 @@ class ColorPickerViewTest : public ChromeViewsTestBase {
     element->OnMouseReleased(released_event);
   }
 
+  void ClickColorAtIndex(int index) {
+    ClickColorElement(color_picker_->GetElementAtIndexForTesting(index));
+  }
+
   ColorPickerView* color_picker_;
 
  private:
@@ -94,3 +100,29 @@ TEST_F(ColorPickerViewTest, ClickingTwiceDeselects) {
   ClickColorAtIndex(0);
   EXPECT_FALSE(color_picker_->GetSelectedColor().has_value());
 }
+
+TEST_F(ColorPickerViewTest, KeyboardFocusBehavesLikeRadioButtons) {
+  views::FocusManager* focus_manager = color_picker_->GetFocusManager();
+
+  // When no color is selected, focus should start on the first.
+  focus_manager->AdvanceFocus(false);
+  EXPECT_EQ(color_picker_->GetElementAtIndexForTesting(0),
+            focus_manager->GetFocusedView());
+
+  // Pressing arrow keys should cycle through the elements.
+  ui::KeyEvent arrow_event(
+      ui::EventType::ET_KEY_PRESSED,
+      ui::DomCodeToUsLayoutKeyboardCode(ui::DomCode::ARROW_RIGHT),
+      ui::DomCode::ARROW_RIGHT, ui::EF_NONE);
+  EXPECT_FALSE(focus_manager->OnKeyEvent(arrow_event));
+  EXPECT_EQ(color_picker_->GetElementAtIndexForTesting(1),
+            focus_manager->GetFocusedView());
+
+  focus_manager->ClearFocus();
+  ClickColorAtIndex(1);
+
+  // Re-entering should restore focus to the currently selected color.
+  focus_manager->AdvanceFocus(false);
+  EXPECT_EQ(color_picker_->GetElementAtIndexForTesting(1),
+            focus_manager->GetFocusedView());
+}