[m116][omnibox][cr23] Update layout with hover fill enabled.

m116 merge. crrev.com/c/4611098 implemented hover fill. This CL updates the layout. https://screenshot.googleplex.com/64fGjkafz9dy8Dw This required redoing the view hierarchy, otherwise, 1 of the following requirements would break: - The remove suggestion 'x' should be vertically centered and 16px from the right edge of the hover fill. - The buttons should be clipped and not painted outside the hover fill when the browser window is too narrow to show the buttons completely. Out-of-bounds buttons are still keyboard focusable. - The button focus ring should not be clipped by the suggestion view above them. - The selection indicator should remain 40px high and top-aligned with the hover fill. - The 4px wide selection indicator should not indent suggestions. - The remove suggestion 'x' mouse target should remain equal to its hover circle (24x24px), rather than stretched to its parent view. Also increases the left margin of the omnibox and dropdown texts by 1px, from 55px to 56px. (cherry picked from commit e9057dc) Low-Coverage-Reason: a) existing code, b) need to merge this to m116 Bug: 1431337 Change-Id: Idc267defb72a589f2711c8cc1041893c760fd130 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4647453 Reviewed-by: Khalid Peer <khalidpeer@chromium.org> Commit-Queue: manuk hovanesian <manukh@chromium.org> Cr-Original-Commit-Position: refs/heads/main@{#1162660} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4652877 Cr-Commit-Position: refs/branch-heads/5845@{#194} Cr-Branched-From: 5a5dff6-refs/heads/main@{#1160321}
chromium · Jun 28, 2023 · 531297b · 531297b
1 parent a945c9a
commit 531297b
Show file tree

Hide file tree

Showing 6 changed files with 183 additions and 84 deletions.
diff --git a/chrome/browser/ui/views/location_bar/location_bar_view.cc b/chrome/browser/ui/views/location_bar/location_bar_view.cc
@@ -621,7 +621,7 @@ void LocationBarView::Layout() {
     icon_left = 5;
     text_left = 8;
     icon_indent = 7;
-    text_indent = 5;
+    text_indent = 6;
     icon_keyword_indent = 3;
     text_keyword_indent = -9;
   } else if (OmniboxFieldTrial::IsChromeRefreshIconsEnabled()) {

diff --git a/chrome/browser/ui/views/omnibox/omnibox_match_cell_view.cc b/chrome/browser/ui/views/omnibox/omnibox_match_cell_view.cc
@@ -242,7 +242,7 @@ OmniboxMatchCellView::~OmniboxMatchCellView() = default;
 int OmniboxMatchCellView::GetTextIndent() {
   return ui::TouchUiController::Get()->touch_ui() ||
                  OmniboxFieldTrial::IsCr23LayoutEnabled()
-             ? 51
+             ? 52
              : 47;
 }
 
@@ -398,13 +398,18 @@ void OmniboxMatchCellView::SetImage(const gfx::ImageSkia& image,
 }
 
 gfx::Insets OmniboxMatchCellView::GetInsets() const {
-  const bool single_line = layout_style_ == LayoutStyle::ONE_LINE_SUGGESTION;
-  const int vertical_margin =
-      OmniboxFieldTrial::IsUniformRowHeightEnabled()
-          ? OmniboxFieldTrial::kRichSuggestionVerticalMargin.Get()
-          : ChromeLayoutProvider::Get()->GetDistanceMetric(
-                single_line ? DISTANCE_OMNIBOX_CELL_VERTICAL_PADDING
-                            : DISTANCE_OMNIBOX_TWO_LINE_CELL_VERTICAL_PADDING);
+  int vertical_margin = 0;
+  if (OmniboxFieldTrial::IsChromeRefreshSuggestHoverFillShapeEnabled()) {
+    vertical_margin = 0;
+  } else if (OmniboxFieldTrial::IsUniformRowHeightEnabled()) {
+    vertical_margin = OmniboxFieldTrial::kRichSuggestionVerticalMargin.Get();
+  } else if (layout_style_ == LayoutStyle::ONE_LINE_SUGGESTION) {
+    vertical_margin = ChromeLayoutProvider::Get()->GetDistanceMetric(
+        DISTANCE_OMNIBOX_CELL_VERTICAL_PADDING);
+  } else {
+    vertical_margin = ChromeLayoutProvider::Get()->GetDistanceMetric(
+        DISTANCE_OMNIBOX_TWO_LINE_CELL_VERTICAL_PADDING);
+  }
   return gfx::Insets::TLBR(vertical_margin, OmniboxMatchCellView::kMarginLeft,
                            vertical_margin, OmniboxMatchCellView::kMarginRight);
 }
@@ -489,12 +494,15 @@ bool OmniboxMatchCellView::GetCanProcessEventsWithinSubtree() const {
 }
 
 gfx::Size OmniboxMatchCellView::CalculatePreferredSize() const {
-  int contentHeight = content_view_->GetLineHeight();
-  int height =
-      OmniboxFieldTrial::IsUniformRowHeightEnabled()
-          ? GetEntityImageSize() +
-                2 * OmniboxFieldTrial::kRichSuggestionVerticalMargin.Get()
-          : contentHeight + GetInsets().height();
+  int height = 0;
+  if (OmniboxFieldTrial::IsChromeRefreshSuggestHoverFillShapeEnabled()) {
+    height = GetEntityImageSize();
+  } else if (OmniboxFieldTrial::IsUniformRowHeightEnabled()) {
+    height = GetEntityImageSize() +
+             2 * OmniboxFieldTrial::kRichSuggestionVerticalMargin.Get();
+  } else {
+    height = content_view_->GetLineHeight() + GetInsets().height();
+  }
   if (layout_style_ == LayoutStyle::TWO_LINE_SUGGESTION)
     height += description_view_->GetHeightForWidth(width() - GetTextIndent());
   // Width is not calculated because it's not needed by current callers.

diff --git a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
@@ -69,7 +69,7 @@
 namespace {
 
 class OmniboxRemoveSuggestionButton : public views::ImageButton {
- public:
+public:
   METADATA_HEADER(OmniboxRemoveSuggestionButton);
   explicit OmniboxRemoveSuggestionButton(PressedCallback callback)
       : ImageButton(std::move(callback)) {
@@ -100,7 +100,7 @@ END_METADATA
 // OmniboxResultSelectionIndicator
 
 class OmniboxResultSelectionIndicator : public views::View {
- public:
+public:
   METADATA_HEADER(OmniboxResultSelectionIndicator);
 
   const bool cr2023_expanded_state_colors_enabled =
@@ -111,7 +111,10 @@ class OmniboxResultSelectionIndicator : public views::View {
 
   explicit OmniboxResultSelectionIndicator(OmniboxResultView* result_view)
       : result_view_(result_view) {
-    SetPreferredSize(gfx::Size(kStrokeThickness, 0));
+    const int height =
+        OmniboxFieldTrial::IsChromeRefreshSuggestHoverFillShapeEnabled() ? 40
+                                                                         : 0;
+    SetPreferredSize(gfx::Size(kStrokeThickness, height));
   }
 
   // views::View:
@@ -125,7 +128,7 @@ class OmniboxResultSelectionIndicator : public views::View {
     canvas->DrawPath(path, flags);
   }
 
- private:
+private:
   // Pointer to the parent view.
   const raw_ptr<OmniboxResultView> result_view_;
 
@@ -165,65 +168,131 @@ OmniboxResultView::OmniboxResultView(OmniboxPopupViewViews* popup_view,
           base::BindRepeating(&OmniboxResultView::UpdateHoverState,
                               base::Unretained(this))) {
   CHECK_GE(model_index, 0u);
-  SetLayoutManager(std::make_unique<views::FlexLayout>())
-      ->SetOrientation(views::LayoutOrientation::kVertical);
-
-  suggestion_container_ = AddChildView(std::make_unique<views::View>());
-  suggestion_container_->SetLayoutManager(
-      std::make_unique<views::FillLayout>());
-  mouse_enter_exit_handler_.ObserveMouseEnterExitOn(suggestion_container_);
-
-  // TODO(olesiamarukhno): Consider making it a decoration instead of separate
-  // view (painting it in a layer).
-  selection_indicator_ = suggestion_container_->AddChildView(
-      std::make_unique<OmniboxResultSelectionIndicator>(this));
-
-  views::View* suggestion_button_container =
-      suggestion_container_->AddChildView(std::make_unique<views::View>());
-  suggestion_button_container
-      ->SetLayoutManager(std::make_unique<views::FlexLayout>())
-      ->SetCrossAxisAlignment(views::LayoutAlignment::kCenter);
-  suggestion_button_container->SetProperty(
-      views::kFlexBehaviorKey,
-      views::FlexSpecification(views::MinimumFlexSizeRule::kScaleToZero,
-                               views::MaximumFlexSizeRule::kUnbounded));
-
-  suggestion_view_ = suggestion_button_container->AddChildView(
-      std::make_unique<OmniboxMatchCellView>(this));
-  suggestion_view_->SetProperty(
-      views::kFlexBehaviorKey,
-      views::FlexSpecification(views::MinimumFlexSizeRule::kScaleToZero,
-                               views::MaximumFlexSizeRule::kUnbounded)
-          .WithWeight(4));
-
-  const auto child_insets =
-      gfx::Insets::TLBR(0, 0, 0, OmniboxMatchCellView::kMarginRight);
-
-  remove_suggestion_button_ = suggestion_button_container->AddChildView(
-      std::make_unique<OmniboxRemoveSuggestionButton>(base::BindRepeating(
-          &OmniboxResultView::ButtonPressed, base::Unretained(this),
-          OmniboxPopupSelection::FOCUSED_BUTTON_REMOVE_SUGGESTION)));
-  remove_suggestion_button_->SetProperty(views::kMarginsKey, child_insets);
-  views::InstallCircleHighlightPathGenerator(remove_suggestion_button_);
-  remove_suggestion_button_->SetTooltipText(
-      l10n_util::GetStringUTF16(IDS_OMNIBOX_REMOVE_SUGGESTION));
-  auto* const focus_ring = views::FocusRing::Get(remove_suggestion_button_);
-  focus_ring->SetHasFocusPredicate(base::BindRepeating(
-      [](const OmniboxResultView* results, const View* view) {
-        return view->GetVisible() && results->GetMatchSelected() &&
-               (results->popup_view_->GetSelection().state ==
-                OmniboxPopupSelection::FOCUSED_BUTTON_REMOVE_SUGGESTION);
-      },
-      base::Unretained(this)));
-  focus_ring->SetColorId(kColorOmniboxResultsFocusIndicator);
-
-  button_row_ = AddChildView(std::make_unique<OmniboxSuggestionButtonRowView>(
-      popup_view_, model_, model_index));
-
-  // Quickly mouse-exiting through the suggestion button row sometimes leaves
-  // the whole row highlighted. This fixes that. It doesn't seem necessary to
-  // further observe the child controls of |button_row_|.
-  mouse_enter_exit_handler_.ObserveMouseEnterExitOn(button_row_);
+
+  if (OmniboxFieldTrial::IsChromeRefreshSuggestHoverFillShapeEnabled()) {
+    SetLayoutManager(std::make_unique<views::FillLayout>());
+
+    auto* selection_indicator_container_ =
+        AddChildView(std::make_unique<views::View>());
+    selection_indicator_container_->SetLayoutManager(
+        std::make_unique<views::FlexLayout>());
+
+    selection_indicator_ = selection_indicator_container_->AddChildView(
+        std::make_unique<OmniboxResultSelectionIndicator>(this));
+    selection_indicator_->SetProperty(views::kCrossAxisAlignmentKey,
+                                      views::LayoutAlignment::kStart);
+
+    auto* right = AddChildView(std::make_unique<views::View>());
+    right->SetLayoutManager(std::make_unique<views::FlexLayout>())
+        ->SetCrossAxisAlignment(views::LayoutAlignment::kCenter);
+
+    views::View* suggestion_and_buttons =
+        right->AddChildView(std::make_unique<views::View>());
+    suggestion_and_buttons
+        ->SetLayoutManager(std::make_unique<views::FlexLayout>())
+        ->SetOrientation(views::LayoutOrientation::kVertical);
+    suggestion_and_buttons->SetProperty(views::kMarginsKey,
+                                        gfx::Insets::VH(6, 0));
+    suggestion_and_buttons->SetProperty(
+        views::kFlexBehaviorKey,
+        views::FlexSpecification(views::MinimumFlexSizeRule::kScaleToZero,
+                                 views::MaximumFlexSizeRule::kUnbounded));
+
+    suggestion_view_ = suggestion_and_buttons->AddChildView(
+        std::make_unique<OmniboxMatchCellView>(this));
+    suggestion_view_->SetProperty(
+        views::kFlexBehaviorKey,
+        views::FlexSpecification(views::MinimumFlexSizeRule::kScaleToZero,
+                                 views::MaximumFlexSizeRule::kUnbounded)
+            .WithWeight(4));
+
+    remove_suggestion_button_ = right->AddChildView(
+        std::make_unique<OmniboxRemoveSuggestionButton>(base::BindRepeating(
+            &OmniboxResultView::ButtonPressed, base::Unretained(this),
+            OmniboxPopupSelection::FOCUSED_BUTTON_REMOVE_SUGGESTION)));
+    remove_suggestion_button_->SetProperty(views::kMarginsKey,
+                                           gfx::Insets::TLBR(0, 0, 0, 16));
+    views::InstallCircleHighlightPathGenerator(remove_suggestion_button_);
+    remove_suggestion_button_->SetTooltipText(
+        l10n_util::GetStringUTF16(IDS_OMNIBOX_REMOVE_SUGGESTION));
+    auto* const focus_ring = views::FocusRing::Get(remove_suggestion_button_);
+    focus_ring->SetHasFocusPredicate(base::BindRepeating(
+        [](const OmniboxResultView* results, const View* view) {
+          return view->GetVisible() && results->GetMatchSelected() &&
+                 (results->popup_view_->GetSelection().state ==
+                  OmniboxPopupSelection::FOCUSED_BUTTON_REMOVE_SUGGESTION);
+        },
+        base::Unretained(this)));
+    focus_ring->SetColorId(kColorOmniboxResultsFocusIndicator);
+
+    button_row_ = suggestion_and_buttons->AddChildView(
+        std::make_unique<OmniboxSuggestionButtonRowView>(popup_view_, model_,
+                                                         model_index));
+
+    mouse_enter_exit_handler_.ObserveMouseEnterExitOn(this);
+
+  } else {
+    SetLayoutManager(std::make_unique<views::FlexLayout>())
+        ->SetOrientation(views::LayoutOrientation::kVertical);
+
+    views::View* suggestion_container_ =
+        AddChildView(std::make_unique<views::View>());
+    suggestion_container_->SetLayoutManager(
+        std::make_unique<views::FillLayout>());
+    mouse_enter_exit_handler_.ObserveMouseEnterExitOn(suggestion_container_);
+
+    // TODO(olesiamarukhno): Consider making it a decoration instead of separate
+    // view (painting it in a layer).
+    selection_indicator_ = suggestion_container_->AddChildView(
+        std::make_unique<OmniboxResultSelectionIndicator>(this));
+
+    views::View* suggestion_button_container =
+        suggestion_container_->AddChildView(std::make_unique<views::View>());
+    suggestion_button_container
+        ->SetLayoutManager(std::make_unique<views::FlexLayout>())
+        ->SetCrossAxisAlignment(views::LayoutAlignment::kCenter);
+    suggestion_button_container->SetProperty(
+        views::kFlexBehaviorKey,
+        views::FlexSpecification(views::MinimumFlexSizeRule::kScaleToZero,
+                                 views::MaximumFlexSizeRule::kUnbounded));
+
+    suggestion_view_ = suggestion_button_container->AddChildView(
+        std::make_unique<OmniboxMatchCellView>(this));
+    suggestion_view_->SetProperty(
+        views::kFlexBehaviorKey,
+        views::FlexSpecification(views::MinimumFlexSizeRule::kScaleToZero,
+                                 views::MaximumFlexSizeRule::kUnbounded)
+            .WithWeight(4));
+
+    const auto child_insets =
+        gfx::Insets::TLBR(0, 0, 0, OmniboxMatchCellView::kMarginRight);
+
+    remove_suggestion_button_ = suggestion_button_container->AddChildView(
+        std::make_unique<OmniboxRemoveSuggestionButton>(base::BindRepeating(
+            &OmniboxResultView::ButtonPressed, base::Unretained(this),
+            OmniboxPopupSelection::FOCUSED_BUTTON_REMOVE_SUGGESTION)));
+    remove_suggestion_button_->SetProperty(views::kMarginsKey, child_insets);
+    views::InstallCircleHighlightPathGenerator(remove_suggestion_button_);
+    remove_suggestion_button_->SetTooltipText(
+        l10n_util::GetStringUTF16(IDS_OMNIBOX_REMOVE_SUGGESTION));
+    auto* const focus_ring = views::FocusRing::Get(remove_suggestion_button_);
+    focus_ring->SetHasFocusPredicate(base::BindRepeating(
+        [](const OmniboxResultView* results, const View* view) {
+          return view->GetVisible() && results->GetMatchSelected() &&
+                 (results->popup_view_->GetSelection().state ==
+                  OmniboxPopupSelection::FOCUSED_BUTTON_REMOVE_SUGGESTION);
+        },
+        base::Unretained(this)));
+    focus_ring->SetColorId(kColorOmniboxResultsFocusIndicator);
+
+    button_row_ = AddChildView(std::make_unique<OmniboxSuggestionButtonRowView>(
+        popup_view_, model_, model_index));
+
+    // Quickly mouse-exiting through the suggestion button row sometimes leaves
+    // the whole row highlighted. This fixes that. It doesn't seem necessary to
+    // further observe the child controls of |button_row_|.
+    mouse_enter_exit_handler_.ObserveMouseEnterExitOn(button_row_);
+  }
 }
 
 OmniboxResultView::~OmniboxResultView() {}

diff --git a/chrome/browser/ui/views/omnibox/omnibox_result_view.h b/chrome/browser/ui/views/omnibox/omnibox_result_view.h
@@ -127,9 +127,6 @@ class OmniboxResultView : public views::View {
   // Accessible name (enables to emit certain events).
   std::u16string accessible_name_;
 
-  // Container for the first row (for everything expect |button_row_|).
-  raw_ptr<views::View> suggestion_container_;
-
   // Weak pointers for easy reference.
   raw_ptr<OmniboxMatchCellView>
       suggestion_view_;  // The leading (or left) view.

diff --git a/chrome/browser/ui/views/omnibox/omnibox_suggestion_button_row_view.cc b/chrome/browser/ui/views/omnibox/omnibox_suggestion_button_row_view.cc
@@ -32,6 +32,7 @@
 #include "ui/base/window_open_disposition.h"
 #include "ui/base/window_open_disposition_utils.h"
 #include "ui/gfx/color_utils.h"
+#include "ui/gfx/geometry/skia_conversions.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/views/accessibility/view_accessibility.h"
 #include "ui/views/animation/ink_drop.h"
@@ -177,17 +178,26 @@ OmniboxSuggestionButtonRowView::OmniboxSuggestionButtonRowView(
   // included in `GetTextIndent()`.
   if (OmniboxFieldTrial::IsCr23LayoutEnabled())
     left_margin += 4;
-  int bottom_margin = ChromeLayoutProvider::Get()->GetDistanceMetric(
-      DISTANCE_OMNIBOX_CELL_VERTICAL_PADDING);
+  int top_margin =
+      OmniboxFieldTrial::IsChromeRefreshSuggestHoverFillShapeEnabled() ? 6 : 0;
+  int bottom_margin =
+      OmniboxFieldTrial::IsChromeRefreshSuggestHoverFillShapeEnabled()
+          ? 6
+          : ChromeLayoutProvider::Get()->GetDistanceMetric(
+                DISTANCE_OMNIBOX_CELL_VERTICAL_PADDING);
   SetLayoutManager(std::make_unique<views::FlexLayout>())
       ->SetCrossAxisAlignment(views::LayoutAlignment::kStart)
       .SetCollapseMargins(true)
-      .SetInteriorMargin(gfx::Insets::TLBR(0, left_margin, bottom_margin, 0))
+      .SetInteriorMargin(
+          gfx::Insets::TLBR(top_margin, left_margin, bottom_margin, 0))
       .SetDefault(
           views::kMarginsKey,
           gfx::Insets::VH(0, ChromeLayoutProvider::Get()->GetDistanceMetric(
                                  views::DISTANCE_RELATED_BUTTON_HORIZONTAL)));
   BuildViews();
+
+  if (OmniboxFieldTrial::IsChromeRefreshSuggestHoverFillShapeEnabled())
+    SetPaintToLayer(ui::LAYER_NOT_DRAWN);
 }
 
 void OmniboxSuggestionButtonRowView::BuildViews() {
@@ -241,6 +251,18 @@ void OmniboxSuggestionButtonRowView::BuildViews() {
 
 OmniboxSuggestionButtonRowView::~OmniboxSuggestionButtonRowView() = default;
 
+void OmniboxSuggestionButtonRowView::Layout() {
+  View::Layout();
+
+  if (!OmniboxFieldTrial::IsChromeRefreshSuggestHoverFillShapeEnabled())
+    return;
+
+  auto bounds = GetLocalBounds();
+  SkPath path;
+  path.addRect(RectToSkRect(bounds), SkPathDirection::kCW, 0);
+  SetClipPath(path);
+}
+
 void OmniboxSuggestionButtonRowView::UpdateFromModel() {
   if (!HasMatch()) {
     // Skip remaining code that depends on `match()`.

diff --git a/chrome/browser/ui/views/omnibox/omnibox_suggestion_button_row_view.h b/chrome/browser/ui/views/omnibox/omnibox_suggestion_button_row_view.h
@@ -33,6 +33,9 @@ class OmniboxSuggestionButtonRowView : public views::View {
       const OmniboxSuggestionButtonRowView&) = delete;
   ~OmniboxSuggestionButtonRowView() override;
 
+  // views::View:
+  void Layout() override;
+
   // Called when the theme state may have changed.
   void SetThemeState(OmniboxPartState theme_state);