[omnibox] Highlight the header row when the user tabs over it

This CL highlights the section header row when the user tabs to the
associated chevron button.

The implementation looks kind of big to do something so simple, but
given our exotic / bespoke focus model, I couldn't do it smaller.

Bug: 1078183, 1052522
Change-Id: Ibffd26183b5f182bf42a874797bf161187476759
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2202669
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: manuk hovanesian <manukh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#769057}

chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc

@@ -238,7 +238,13 @@ void OmniboxPopupContentsView::InvalidateLine(size_t line) {
     webui_view_->GetWebUIHandler()->InvalidateLine(line);
     return;
   }
-  result_view_at(line)->OnSelectionStateChanged();
+
+  // TODO(tommycli): This is weird, but https://crbug.com/1063071 shows that
+  // crashes like this have happened, so we add this to avoid it for now.
+  if (line >= children().size())
+    return;
+
+  static_cast<OmniboxRowView*>(children()[line])->OnSelectionStateChanged();
 }
 
 void OmniboxPopupContentsView::OnSelectionChanged(
@@ -321,7 +327,8 @@ void OmniboxPopupContentsView::UpdatePopupAppearance() {
       // memory during browser startup. https://crbug.com/1021323
       if (children().size() == i) {
         AddChildView(std::make_unique<OmniboxRowView>(
-            std::make_unique<OmniboxResultView>(this, i), pref_service));
+            i, model(), std::make_unique<OmniboxResultView>(this, i),
+            pref_service));
       }
 
       OmniboxRowView* const row_view =

chrome/browser/ui/views/omnibox/omnibox_row_view.cc

@@ -10,6 +10,7 @@
 #include "chrome/browser/ui/views/chrome_typography.h"
 #include "chrome/browser/ui/views/omnibox/omnibox_match_cell_view.h"
 #include "chrome/browser/ui/views/omnibox/omnibox_result_view.h"
+#include "components/omnibox/browser/omnibox_popup_model.h"
 #include "components/omnibox/browser/omnibox_prefs.h"
 #include "components/omnibox/browser/vector_icons.h"
 #include "components/prefs/pref_service.h"
@@ -27,7 +28,7 @@
 class OmniboxRowView::HeaderView : public views::View,
                                    public views::ButtonListener {
  public:
-  explicit HeaderView(PrefService* pref_service) : pref_service_(pref_service) {
+  explicit HeaderView(OmniboxRowView* row_view) : row_view_(row_view) {
     views::BoxLayout* layout =
         SetLayoutManager(std::make_unique<views::BoxLayout>(
             views::BoxLayout::Orientation::kHorizontal));
@@ -56,9 +57,9 @@ class OmniboxRowView::HeaderView : public views::View,
     // Moreover, it seems unusual to do case conversion in Views in general.
     header_text_->SetText(base::i18n::ToUpper(header_text));
 
-    if (pref_service_) {
+    if (row_view_->pref_service_) {
       hide_button_->SetToggled(omnibox::IsSuggestionGroupIdHidden(
-          pref_service_, suggestion_group_id_));
+          row_view_->pref_service_, suggestion_group_id_));
     }
   }
 
@@ -79,38 +80,40 @@ class OmniboxRowView::HeaderView : public views::View,
     return gfx::Insets(vertical, left_inset, vertical,
                        OmniboxMatchCellView::kMarginRight);
   }
-  void OnMouseEntered(const ui::MouseEvent& event) override {
-    UpdateUIForHoverState();
-  }
-  void OnMouseExited(const ui::MouseEvent& event) override {
-    UpdateUIForHoverState();
-  }
+  void OnMouseEntered(const ui::MouseEvent& event) override { UpdateUI(); }
+  void OnMouseExited(const ui::MouseEvent& event) override { UpdateUI(); }
   void OnThemeChanged() override {
     views::View::OnThemeChanged();
 
     // When the theme is updated, also refresh the hover-specific UI, which is
     // all of the UI.
-    UpdateUIForHoverState();
+    UpdateUI();
   }
 
   // views::ButtonListener:
   void ButtonPressed(views::Button* sender, const ui::Event& event) override {
     DCHECK_EQ(sender, hide_button_);
 
-    if (!pref_service_)
+    if (!row_view_->pref_service_)
       return;
 
-    omnibox::ToggleSuggestionGroupIdVisibility(pref_service_,
+    omnibox::ToggleSuggestionGroupIdVisibility(row_view_->pref_service_,
                                                suggestion_group_id_);
     hide_button_->SetToggled(omnibox::IsSuggestionGroupIdHidden(
-        pref_service_, suggestion_group_id_));
+        row_view_->pref_service_, suggestion_group_id_));
   }
 
- private:
-  // Some UI changes on-hover, and this function effects those changes.
-  void UpdateUIForHoverState() {
-    OmniboxPartState part_state =
-        IsMouseHovered() ? OmniboxPartState::HOVERED : OmniboxPartState::NORMAL;
+  // Updates the UI state for the new hover or selection state.
+  void UpdateUI() {
+    OmniboxPartState part_state = OmniboxPartState::NORMAL;
+    if (row_view_->popup_model_->selection() ==
+        OmniboxPopupModel::Selection(
+            row_view_->line_, OmniboxPopupModel::HEADER_BUTTON_FOCUSED)) {
+      part_state = OmniboxPartState::SELECTED;
+    } else if (IsMouseHovered()) {
+      part_state = OmniboxPartState::HOVERED;
+    }
+
     SkColor icon_color = GetOmniboxColor(GetThemeProvider(),
                                          OmniboxPart::RESULTS_ICON, part_state);
     hide_button_->set_ink_drop_base_color(icon_color);
@@ -133,9 +136,10 @@ class OmniboxRowView::HeaderView : public views::View,
     SetBackground(OmniboxResultView::GetPopupCellBackground(this, part_state));
   }
 
-  // Non-owning pointer to the preference service used for toggling headers.
-  // May be nullptr in tests.
-  PrefService* const pref_service_;
+ private:
+  // Non-owning pointer our parent row view. We access a lot of private members
+  // of our outer class. This lets us save quite a bit of state duplication.
+  OmniboxRowView* const row_view_;
 
   // The Label containing the header text. This is never nullptr.
   views::Label* header_text_;
@@ -147,11 +151,12 @@ class OmniboxRowView::HeaderView : public views::View,
   int suggestion_group_id_ = 0;
 };
 
-OmniboxRowView::OmniboxRowView(std::unique_ptr<OmniboxResultView> result_view,
+OmniboxRowView::OmniboxRowView(size_t line,
+                               OmniboxPopupModel* popup_model,
+                               std::unique_ptr<OmniboxResultView> result_view,
                                PrefService* pref_service)
-    : pref_service_(pref_service) {
+    : line_(line), popup_model_(popup_model), pref_service_(pref_service) {
   DCHECK(result_view);
-  DCHECK(pref_service);
 
   SetLayoutManager(std::make_unique<views::BoxLayout>(
       views::BoxLayout::Orientation::kVertical));
@@ -162,10 +167,8 @@ OmniboxRowView::OmniboxRowView(std::unique_ptr<OmniboxResultView> result_view,
 void OmniboxRowView::ShowHeader(int suggestion_group_id,
                                 const base::string16& header_text) {
   // Create the header (at index 0) if it doesn't exist.
-  if (header_view_ == nullptr) {
-    header_view_ =
-        AddChildViewAt(std::make_unique<HeaderView>(pref_service_), 0);
-  }
+  if (header_view_ == nullptr)
+    header_view_ = AddChildViewAt(std::make_unique<HeaderView>(this), 0);
 
   header_view_->SetHeader(suggestion_group_id, header_text);
   header_view_->SetVisible(true);
@@ -176,6 +179,12 @@ void OmniboxRowView::HideHeader() {
     header_view_->SetVisible(false);
 }
 
+void OmniboxRowView::OnSelectionStateChanged() {
+  result_view_->OnSelectionStateChanged();
+  if (header_view_ && header_view_->GetVisible())
+    header_view_->UpdateUI();
+}
+
 gfx::Insets OmniboxRowView::GetInsets() const {
   // A visible header means this is the start of a new section. Give the section
   // that just ended an extra 4dp of padding. https://crbug.com/1076646

chrome/browser/ui/views/omnibox/omnibox_row_view.h

@@ -9,6 +9,7 @@
 #include "base/strings/string16.h"
 #include "ui/views/view.h"
 
+class OmniboxPopupModel;
 class OmniboxResultView;
 class PrefService;
 
@@ -21,7 +22,9 @@ class PrefService;
 //  - It's the header for multiple matches, it's just painted above this row.
 class OmniboxRowView : public views::View {
  public:
-  OmniboxRowView(std::unique_ptr<OmniboxResultView> result_view,
+  OmniboxRowView(size_t line,
+                 OmniboxPopupModel* popup_model,
+                 std::unique_ptr<OmniboxResultView> result_view,
                  PrefService* pref_service);
 
   // Sets the header that appears above this row. Also shows the header.
@@ -33,12 +36,21 @@ class OmniboxRowView : public views::View {
   // The result view associated with this row.
   OmniboxResultView* result_view() const { return result_view_; }
 
+  // Invoked when the model's selection state has changed.
+  void OnSelectionStateChanged();
+
   // views::View:
   gfx::Insets GetInsets() const override;
 
  private:
   class HeaderView;
 
+  // Line number of this row.
+  const size_t line_;
+
+  // Non-owning pointer to the backing popup model.
+  OmniboxPopupModel* const popup_model_;
+
   // Non-owning pointer to the header view for this row. This is initially
   // nullptr, and lazily created when a header is first set for this row.
   // Lazily creating these speeds up browser startup: https://crbug.com/1021323