TableView: Fix incorrect row position for focus ring

While sorting the TableView the focus ring was in the incorrect
spot. Upon investigation, the position was always the model_index
instead of the view_index.

Added a unit test to make sure that the active row index matches
the view index being shown meanwhile the model index differs (which
tells us it has been sorted). Previously this test will fail since
the focus ring is being rendered in a different position.

AX-Relnotes: A focus ring now appears around the correct cell
in tables that are found in e.g. Chromium's Task Manager.

Bug: 1121394
Change-Id: I6921268a7274d5f41eaec8de013e2ea90e3abdb1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2373236
Commit-Queue: Mohamed Mansour <mmansour@microsoft.com>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Reviewed-by: Nektarios Paisios <nektar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#802364}

ui/views/controls/table/table_view.cc

@@ -937,7 +937,11 @@ gfx::Rect TableView::GetCellBounds(int row, int visible_column_index) const {
 }
 
 gfx::Rect TableView::GetActiveCellBounds() const {
-  return GetCellBounds(selection_model_.active(), active_visible_column_index_);
+  if (selection_model_.active() == ui::ListSelectionModel::kUnselectedIndex) {
+    return gfx::Rect();
+  }
+  return GetCellBounds(ModelToView(selection_model_.active()),
+                       active_visible_column_index_);
 }
 
 void TableView::AdjustCellBoundsForText(int visible_column_index,

ui/views/controls/table/table_view_unittest.cc

@@ -71,6 +71,14 @@ class TableViewTestHelper {
     return table_->GetVirtualAccessibilityCell(row, visible_column_index);
   }
 
+  gfx::Rect GetCellBounds(int row, int visible_column_index) const {
+    return table_->GetCellBounds(row, visible_column_index);
+  }
+
+  gfx::Rect GetActiveCellBounds() const {
+    return table_->GetActiveCellBounds();
+  }
+
   std::vector<std::vector<gfx::Rect>> GenerateExpectedBounds() {
     // Generates the expected bounds for |table_|'s rows and cells. Each vector
     // represents a row. The first entry in each child vector is the bounds for
@@ -888,6 +896,50 @@ TEST_P(TableViewTest, SortOnSpaceBar) {
   EXPECT_FALSE(table_->sort_descriptors()[1].ascending);
 }
 
+TEST_P(TableViewTest, ActiveCellBoundsFollowColumnSorting) {
+  table_->RequestFocus();
+  ASSERT_TRUE(table_->sort_descriptors().empty());
+
+  ui::ListSelectionModel new_selection;
+  new_selection.SetSelectedIndex(1);
+  helper_->SetSelectionModel(new_selection);
+
+  // Toggle the sort order of the first column. Shouldn't change the order.
+  table_->ToggleSortOrder(0);
+  ClickOnRow(0, 0);
+  EXPECT_EQ(helper_->GetCellBounds(0, 0), helper_->GetActiveCellBounds());
+  EXPECT_EQ(0, table_->ViewToModel(0));
+
+  ClickOnRow(1, 0);
+  EXPECT_EQ(helper_->GetCellBounds(1, 0), helper_->GetActiveCellBounds());
+  EXPECT_EQ(1, table_->ViewToModel(1));
+
+  ClickOnRow(2, 0);
+  EXPECT_EQ(helper_->GetCellBounds(2, 0), helper_->GetActiveCellBounds());
+  EXPECT_EQ(2, table_->ViewToModel(2));
+
+  // Toggle the sort order of the second column. The active row will stay in
+  // sync with the view index, meanwhile the model's change which shows that
+  // the list order has changed.
+  table_->ToggleSortOrder(1);
+  ClickOnRow(0, 0);
+  EXPECT_EQ(helper_->GetCellBounds(0, 0), helper_->GetActiveCellBounds());
+  EXPECT_EQ(3, table_->ViewToModel(0));
+
+  ClickOnRow(1, 0);
+  EXPECT_EQ(helper_->GetCellBounds(1, 0), helper_->GetActiveCellBounds());
+  EXPECT_EQ(0, table_->ViewToModel(1));
+
+  ClickOnRow(2, 0);
+  EXPECT_EQ(helper_->GetCellBounds(2, 0), helper_->GetActiveCellBounds());
+  EXPECT_EQ(1, table_->ViewToModel(2));
+
+  // Verifying invalid active indexes return an empty rect.
+  new_selection.Clear();
+  helper_->SetSelectionModel(new_selection);
+  EXPECT_EQ(gfx::Rect(), helper_->GetActiveCellBounds());
+}
+
 TEST_P(TableViewTest, Tooltip) {
   // Column 0 uses the TableModel's GetTooltipText override for tooltips.
   table_->SetVisibleColumnWidth(0, 10);