Add HighlightPathGenerator to FocusRing

Replaces OnBoundsChanged + FocusRing::SetPath() with either installing a
HighlightPathGenerator for the view or installing an overriding one on
the FocusRing. It also removes an InkDropView::GetInkDropMask override.

This change also adds a PillHighlightPathGenerator which was used both
for the location bar and a button inside the Omnibox.

Bug: chromium:1007546
Change-Id: I163291c5ee4f8f361d3181b30d0e81f310bb4eab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1842115
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Charlene Yan <cyan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#703138}

chrome/browser/ui/views/location_bar/location_bar_view.cc

@@ -116,6 +116,7 @@
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/button/image_button_factory.h"
 #include "ui/views/controls/focus_ring.h"
+#include "ui/views/controls/highlight_path_generator.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/widget/widget.h"
 
@@ -157,6 +158,21 @@ LocationBarView::LocationBarView(Browser* browser,
       profile_(profile),
       delegate_(delegate),
       is_popup_mode_(is_popup_mode) {
+  if (!is_popup_mode_) {
+    focus_ring_ = views::FocusRing::Install(this);
+    focus_ring_->SetHasFocusPredicate([](View* view) -> bool {
+      DCHECK_EQ(view->GetClassName(), LocationBarView::kViewClassName);
+      auto* v = static_cast<LocationBarView*>(view);
+
+      // Show focus ring when the Omnibox is visibly focused and the popup is
+      // closed.
+      return v->omnibox_view_->model()->is_caret_visible() &&
+             !v->GetOmniboxPopupView()->IsOpen();
+    });
+
+    focus_ring_->SetPathGenerator(
+        std::make_unique<views::PillHighlightPathGenerator>());
+  }
   md_observer_.Add(ui::MaterialDesignController::GetInstance());
 }
 
@@ -868,37 +884,6 @@ void LocationBarView::RefreshClearAllButtonIcon() {
       GetLayoutInsets(LOCATION_BAR_ICON_INTERIOR_PADDING)));
 }
 
-void LocationBarView::RefreshFocusRing() {
-  // Popup windows should not display a focus ring.
-  if (is_popup_mode_)
-    return;
-
-  // We may not have a usable path during initialization before first layout.
-  // This will be called again once there is a usable path, so early exit.
-  const int radius = GetBorderRadius();
-  SkPath path;
-  path.addRRect(
-      SkRRect::MakeRectXY(RectToSkRect(GetLocalBounds()), radius, radius));
-  if (!views::FocusRing::IsPathUseable(path))
-    return;
-
-  // Install a focus ring if it's not already there.
-  if (!focus_ring_) {
-    focus_ring_ = views::FocusRing::Install(this);
-    focus_ring_->SetHasFocusPredicate([](View* view) -> bool {
-      auto* v = static_cast<LocationBarView*>(view);
-
-      // Show focus ring when the Omnibox is visibly focused and the popup is
-      // closed.
-      return v->omnibox_view_->model()->is_caret_visible() &&
-             !v->GetOmniboxPopupView()->IsOpen();
-    });
-  }
-
-  // Update the focus ring path.
-  focus_ring_->SetPath(path);
-}
-
 bool LocationBarView::ShouldShowKeywordBubble() const {
   return omnibox_view_->model()->is_keyword_selected();
 }
@@ -1014,7 +999,6 @@ const char* LocationBarView::GetClassName() const {
 
 void LocationBarView::OnBoundsChanged(const gfx::Rect& previous_bounds) {
   RefreshBackground();
-  RefreshFocusRing();
 }
 
 bool LocationBarView::GetNeedsNotificationWhenVisibleBoundsChange() const {

chrome/browser/ui/views/location_bar/location_bar_view.h

@@ -260,9 +260,6 @@ class LocationBarView : public LocationBar,
   // Updates the color of the icon for the "clear all" button.
   void RefreshClearAllButtonIcon();
 
-  // Updates the focus ring.
-  void RefreshFocusRing();
-
   // Returns true if a keyword is selected in the model.
   bool ShouldShowKeywordBubble() const;
 

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

@@ -20,6 +20,7 @@
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/views/accessibility/view_accessibility.h"
 #include "ui/views/animation/ink_drop_mask.h"
+#include "ui/views/controls/highlight_path_generator.h"
 
 bool OmniboxTabSwitchButton::calculated_widths_ = false;
 size_t OmniboxTabSwitchButton::icon_only_width_;
@@ -41,6 +42,7 @@ OmniboxTabSwitchButton::OmniboxTabSwitchButton(
       hint_(hint),
       hint_short_(hint_short),
       theme_provider_(theme_provider) {
+  views::InstallPillHighlightPathGenerator(this);
   SetBgColorOverride(GetBackgroundColor());
   SetImage(STATE_NORMAL, gfx::CreateVectorIcon(
                              icon, GetLayoutConstant(LOCATION_BAR_ICON_SIZE),
@@ -79,17 +81,6 @@ gfx::Size OmniboxTabSwitchButton::CalculatePreferredSize() const {
   return size;
 }
 
-void OmniboxTabSwitchButton::OnBoundsChanged(const gfx::Rect& previous_bounds) {
-  MdTextButton::OnBoundsChanged(previous_bounds);
-  focus_ring()->SetPath(GetFocusRingPath());
-}
-
-std::unique_ptr<views::InkDropMask> OmniboxTabSwitchButton::CreateInkDropMask()
-    const {
-  return std::make_unique<views::RoundRectInkDropMask>(
-      size(), gfx::Insets(), CalculatePreferredSize().height() / 2.f);
-}
-
 void OmniboxTabSwitchButton::AnimationProgressed(
     const gfx::Animation* animation) {
   if (animation != animation_.get()) {

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

@@ -27,8 +27,6 @@ class OmniboxTabSwitchButton : public views::MdTextButton {
 
   // views::MdTextButton:
   gfx::Size CalculatePreferredSize() const override;
-  void OnBoundsChanged(const gfx::Rect& previous_bounds) override;
-  std::unique_ptr<views::InkDropMask> CreateInkDropMask() const override;
   void AnimationProgressed(const gfx::Animation* animation) override;
   void StateChanged(ButtonState old_state) override;
 

ui/views/controls/button/checkbox.cc

@@ -21,6 +21,7 @@
 #include "ui/views/animation/ink_drop_ripple.h"
 #include "ui/views/controls/button/label_button_border.h"
 #include "ui/views/controls/focus_ring.h"
+#include "ui/views/controls/highlight_path_generator.h"
 #include "ui/views/layout/layout_provider.h"
 #include "ui/views/metadata/metadata_impl_macros.h"
 #include "ui/views/painter.h"
@@ -31,6 +32,18 @@
 
 namespace views {
 
+class Checkbox::FocusRingHighlightPathGenerator
+    : public views::HighlightPathGenerator {
+ public:
+  SkPath GetHighlightPath(const views::View* view) override {
+    SkPath path;
+    auto* checkbox = static_cast<const views::Checkbox*>(view);
+    if (checkbox->image()->bounds().IsEmpty())
+      return path;
+    return checkbox->GetFocusRingPath();
+  }
+};
+
 Checkbox::Checkbox(const base::string16& label, ButtonListener* listener)
     : LabelButton(listener, label), checked_(false), label_ax_id_(0) {
   SetHorizontalAlignment(gfx::ALIGN_LEFT);
@@ -155,12 +168,6 @@ std::unique_ptr<LabelButtonBorder> Checkbox::CreateDefaultBorder() const {
   return border;
 }
 
-void Checkbox::Layout() {
-  LabelButton::Layout();
-  if (focus_ring() && !image()->bounds().IsEmpty())
-    focus_ring()->SetPath(GetFocusRingPath());
-}
-
 SkPath Checkbox::GetFocusRingPath() const {
   SkPath path;
   gfx::Rect bounds = image()->GetMirroredBounds();

ui/views/controls/button/checkbox.h

@@ -56,7 +56,6 @@ class VIEWS_EXPORT Checkbox : public LabelButton {
   SkColor GetInkDropBaseColor() const override;
   gfx::ImageSkia GetImage(ButtonState for_state) const override;
   std::unique_ptr<LabelButtonBorder> CreateDefaultBorder() const override;
-  void Layout() override;
 
   // Gets the vector icon to use based on the current state of |checked_|.
   virtual const gfx::VectorIcon& GetVectorIcon() const;
@@ -65,7 +64,7 @@ class VIEWS_EXPORT Checkbox : public LabelButton {
   virtual SkPath GetFocusRingPath() const;
 
  private:
-  friend class IconFocusRing;
+  class FocusRingHighlightPathGenerator;
 
   // Bitmask constants for GetIconImageColor.
   enum IconState { CHECKED = 0b1, ENABLED = 0b10 };

ui/views/controls/focus_ring.cc

@@ -14,6 +14,11 @@ namespace views {
 
 namespace {
 
+bool IsPathUsable(const SkPath& path) {
+  return !path.isEmpty() && (path.isRect(nullptr) || path.isOval(nullptr) ||
+                             path.isRRect(nullptr));
+}
+
 ui::NativeTheme::ColorId ColorIdForValidity(bool valid) {
   return valid ? ui::NativeTheme::kColorId_FocusedBorderColor
                : ui::NativeTheme::kColorId_AlertSeverityHigh;
@@ -27,6 +32,7 @@ double GetCornerRadius() {
 SkPath GetHighlightPathInternal(const View* view) {
   HighlightPathGenerator* path_generator =
       view->GetProperty(kHighlightPathGeneratorKey);
+
   if (path_generator)
     return path_generator->GetHighlightPath(view);
 
@@ -36,10 +42,8 @@ SkPath GetHighlightPathInternal(const View* view) {
     return *highlight_path;
 
   const double corner_radius = GetCornerRadius();
-  SkPath path;
-  path.addRRect(SkRRect::MakeRectXY(RectToSkRect(view->GetLocalBounds()),
-                                    corner_radius, corner_radius));
-  return path;
+  return SkPath().addRRect(SkRRect::MakeRectXY(
+      RectToSkRect(view->GetLocalBounds()), corner_radius, corner_radius));
 }
 
 }  // namespace
@@ -54,14 +58,9 @@ std::unique_ptr<FocusRing> FocusRing::Install(View* parent) {
   return ring;
 }
 
-// static
-bool FocusRing::IsPathUseable(const SkPath& path) {
-  return !path.isEmpty() && (path.isRect(nullptr) || path.isOval(nullptr) ||
-                             path.isRRect(nullptr));
-}
-
-void FocusRing::SetPath(const SkPath& path) {
-  path_ = IsPathUseable(path) ? path : SkPath();
+void FocusRing::SetPathGenerator(
+    std::unique_ptr<HighlightPathGenerator> generator) {
+  path_generator_ = std::move(generator);
   SchedulePaint();
 }
 
@@ -119,13 +118,16 @@ void FocusRing::OnPaint(gfx::Canvas* canvas) {
   paint.setStyle(cc::PaintFlags::kStroke_Style);
   paint.setStrokeWidth(PlatformStyle::kFocusHaloThickness);
 
-  SkPath path = path_;
-  // Focus rings flip the canvas if RTL is enabled for it's parent, so
-  // we need to always get non-mirrored highlight path for focus rings.
-  if (path.isEmpty())
+  SkPath path;
+  if (path_generator_)
+    path = path_generator_->GetHighlightPath(parent());
+
+  // If there's no path generator or the generated path is unusable, fall back
+  // to the default.
+  if (!IsPathUsable(path))
     path = GetHighlightPathInternal(parent());
 
-  DCHECK(IsPathUseable(path));
+  DCHECK(IsPathUsable(path));
   DCHECK_EQ(flip_canvas_on_paint_for_rtl_ui(),
             parent()->flip_canvas_on_paint_for_rtl_ui());
   SkRect bounds;

ui/views/controls/focus_ring.h

@@ -14,6 +14,8 @@
 
 namespace views {
 
+class HighlightPathGenerator;
+
 // FocusRing is a View that is designed to act as an indicator of focus for its
 // parent. It is a stand-alone view that paints to a layer which extends beyond
 // the bounds of its parent view.
@@ -48,20 +50,12 @@ class VIEWS_EXPORT FocusRing : public View, public ViewObserver {
   // |parent|.
   static std::unique_ptr<FocusRing> Install(View* parent);
 
-  // Returns whether this class can draw a focus ring from |path|. Not all paths
-  // are useable since not all paths can be easily outset. If a FocusRing is
-  // configured to use an unuseable path, it will fall back to the default focus
-  // ring path.
-  static bool IsPathUseable(const SkPath& path);
-
-  // Sets the path to draw this FocusRing around. This path is in the parent
-  // view's coordinate system, *not* in the FocusRing's coordinate system. Note
-  // that this path will not be mirrored in RTL, so your View's computation of
-  // it should take RTL into account.
+  // Sets the HighlightPathGenerator to draw this FocusRing around.
   // Note: This method should only be used if the focus ring needs to differ
-  // from the highlight shape used for inkdrops. Otherwise set kHighlightPathKey
-  // on the parent and FocusRing will use it as well.
-  void SetPath(const SkPath& path);
+  // from the highlight shape used for InkDrops.
+  // Otherwise install a HighlightPathGenerator on the parent and FocusRing will
+  // use it as well.
+  void SetPathGenerator(std::unique_ptr<HighlightPathGenerator> generator);
 
   // Sets whether the FocusRing should show an invalid state for the View it
   // encloses.
@@ -97,9 +91,8 @@ class VIEWS_EXPORT FocusRing : public View, public ViewObserver {
   SkRRect RingRectFromPathRect(const SkRect& rect) const;
   SkRRect RingRectFromPathRect(const SkRRect& rect) const;
 
-  // The path to draw this focus ring around. IsPathUseable(path_) is always
-  // true.
-  SkPath path_;
+  // The path generator used to draw this focus ring.
+  std::unique_ptr<HighlightPathGenerator> path_generator_;
 
   // Whether the enclosed View is in an invalid state, which controls whether
   // the focus ring shows an invalid appearance (usually a different color).

ui/views/controls/highlight_path_generator.cc

@@ -20,9 +20,7 @@ void HighlightPathGenerator::Install(
 }
 
 SkPath RectHighlightPathGenerator::GetHighlightPath(const View* view) {
-  SkPath path;
-  path.addRect(gfx::RectToSkRect(view->GetLocalBounds()));
-  return path;
+  return SkPath().addRect(gfx::RectToSkRect(view->GetLocalBounds()));
 }
 
 void InstallRectHighlightPathGenerator(View* view) {
@@ -34,9 +32,7 @@ SkPath CircleHighlightPathGenerator::GetHighlightPath(const View* view) {
   const SkRect rect = gfx::RectToSkRect(view->GetLocalBounds());
   const SkScalar radius = SkScalarHalf(std::min(rect.width(), rect.height()));
 
-  SkPath path;
-  path.addCircle(rect.centerX(), rect.centerY(), radius);
-  return path;
+  return SkPath().addCircle(rect.centerX(), rect.centerY(), radius);
 }
 
 void InstallCircleHighlightPathGenerator(View* view) {
@@ -44,4 +40,17 @@ void InstallCircleHighlightPathGenerator(View* view) {
       view, std::make_unique<CircleHighlightPathGenerator>());
 }
 
+SkPath PillHighlightPathGenerator::GetHighlightPath(const View* view) {
+  const SkRect rect = gfx::RectToSkRect(view->GetLocalBounds());
+  const SkScalar radius = SkScalarHalf(std::min(rect.width(), rect.height()));
+
+  return SkPath().addRoundRect(gfx::RectToSkRect(view->GetLocalBounds()),
+                               radius, radius);
+}
+
+void InstallPillHighlightPathGenerator(View* view) {
+  HighlightPathGenerator::Install(
+      view, std::make_unique<PillHighlightPathGenerator>());
+}
+
 }  // namespace views

ui/views/controls/highlight_path_generator.h

@@ -62,6 +62,21 @@ class VIEWS_EXPORT CircleHighlightPathGenerator
 
 void VIEWS_EXPORT InstallCircleHighlightPathGenerator(View* view);
 
+// Sets a pill-shaped highlight path.
+class VIEWS_EXPORT PillHighlightPathGenerator : public HighlightPathGenerator {
+ public:
+  PillHighlightPathGenerator() = default;
+
+  PillHighlightPathGenerator(const PillHighlightPathGenerator&) = delete;
+  PillHighlightPathGenerator& operator=(const PillHighlightPathGenerator&) =
+      delete;
+
+  // HighlightPathGenerator:
+  SkPath GetHighlightPath(const View* view) override;
+};
+
+void VIEWS_EXPORT InstallPillHighlightPathGenerator(View* view);
+
 }  // namespace views
 
 #endif  // UI_VIEWS_CONTROLS_HIGHLIGHT_PATH_GENERATOR_H_