ImageView: Fix CENTER alignment on ImageViews with borders.

When positioning its internal ImageSkia for painting, CENTER-aligned ImageViews
currently assume that any insets it may have are symmetrical (i.e. the
left and right or top and bottom insets are the same value). This causes the
image to be drawn in the incorrect position (e.g. it may cross the boundaries
set by the ImageView insets) when the insets are not symmetrical. Fix by
accounting for the inset width and height during positioning.

Bug: 801583
Change-Id: I65766fbea4c088ca45781a6a69cbae7b52178625
Reviewed-on: https://chromium-review.googlesource.com/917843
Commit-Queue: Patti <patricialor@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536962}

ui/views/BUILD.gn

@@ -919,6 +919,7 @@ source_set("views_unittests_sources") {
     "controls/button/radio_button_unittest.cc",
     "controls/button/toggle_button_unittest.cc",
     "controls/combobox/combobox_unittest.cc",
+    "controls/image_view_unittest.cc",
     "controls/label_unittest.cc",
     "controls/menu/menu_controller_unittest.cc",
     "controls/menu/menu_item_view_unittest.cc",

ui/views/controls/image_view.cc

@@ -106,7 +106,9 @@ gfx::Point ImageView::ComputeImageOrigin(const gfx::Size& image_size) const {
   switch (actual_horiz_alignment) {
     case LEADING:  x = insets.left();                                 break;
     case TRAILING: x = width() - insets.right() - image_size.width(); break;
-    case CENTER:   x = (width() - image_size.width()) / 2;            break;
+    case CENTER:
+      x = (width() - insets.width() - image_size.width()) / 2 + insets.left();
+      break;
     default:       NOTREACHED(); x = 0;                               break;
   }
 
@@ -114,7 +116,9 @@ gfx::Point ImageView::ComputeImageOrigin(const gfx::Size& image_size) const {
   switch (vert_alignment_) {
     case LEADING:  y = insets.top();                                     break;
     case TRAILING: y = height() - insets.bottom() - image_size.height(); break;
-    case CENTER:   y = (height() - image_size.height()) / 2;             break;
+    case CENTER:
+      y = (height() - insets.height() - image_size.height()) / 2 + insets.top();
+      break;
     default:       NOTREACHED(); y = 0;                                  break;
   }
 

ui/views/controls/image_view.h

@@ -82,6 +82,8 @@ class VIEWS_EXPORT ImageView : public View {
   views::PaintInfo::ScaleType GetPaintScaleType() const override;
 
  private:
+  friend class ImageViewTest;
+
   void OnPaintImage(gfx::Canvas* canvas);
 
   // Returns true if |img| is the same as the last image we painted. This is

ui/views/controls/image_view_unittest.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ui/views/controls/image_view.h"
+
+#include "base/i18n/rtl.h"
+#include "base/memory/ptr_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/skia/include/core/SkBitmap.h"
+#include "third_party/skia/include/core/SkColor.h"
+#include "ui/gfx/geometry/insets.h"
+#include "ui/gfx/geometry/point.h"
+#include "ui/gfx/geometry/size.h"
+#include "ui/gfx/image/image_skia.h"
+#include "ui/views/border.h"
+#include "ui/views/layout/box_layout.h"
+#include "ui/views/test/views_test_base.h"
+#include "ui/views/widget/widget.h"
+
+namespace {
+
+enum class Axis {
+  kHorizontal,
+  kVertical,
+};
+
+// A test utility function to set the application default text direction.
+void SetRTL(bool rtl) {
+  // Override the current locale/direction.
+  base::i18n::SetICUDefaultLocale(rtl ? "he" : "en");
+  EXPECT_EQ(rtl, base::i18n::IsRTL());
+}
+
+}  // namespace
+
+namespace views {
+
+class ImageViewTest : public ViewsTestBase,
+                      public ::testing::WithParamInterface<Axis> {
+ public:
+  ImageViewTest() {}
+
+  // ViewsTestBase:
+  void SetUp() override {
+    ViewsTestBase::SetUp();
+
+    Widget::InitParams params =
+        CreateParams(Widget::InitParams::TYPE_WINDOW_FRAMELESS);
+    params.bounds = gfx::Rect(200, 200);
+    params.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+    widget_.Init(params);
+    View* container = new View();
+    // Make sure children can take up exactly as much space as they require.
+    BoxLayout::Orientation orientation = GetParam() == Axis::kHorizontal
+                                             ? BoxLayout::kHorizontal
+                                             : BoxLayout::kVertical;
+    container->SetLayoutManager(std::make_unique<BoxLayout>(orientation));
+    widget_.SetContentsView(container);
+
+    image_view_ = new ImageView();
+    container->AddChildView(image_view_);
+
+    widget_.Show();
+  }
+
+  void TearDown() override {
+    widget_.Close();
+    ViewsTestBase::TearDown();
+  }
+
+  int CurrentImageOriginForParam() {
+    gfx::Point origin =
+        image_view()->ComputeImageOrigin(image_view()->GetImageSize());
+    return GetParam() == Axis::kHorizontal ? origin.x() : origin.y();
+  }
+
+ protected:
+  ImageView* image_view() { return image_view_; }
+  Widget* widget() { return &widget_; }
+
+ private:
+  ImageView* image_view_ = nullptr;
+  Widget widget_;
+
+  DISALLOW_COPY_AND_ASSIGN(ImageViewTest);
+};
+
+// Test the image origin of the internal ImageSkia is correct when it is
+// center-aligned (both horizontally and vertically).
+TEST_P(ImageViewTest, CenterAlignment) {
+  image_view()->SetHorizontalAlignment(ImageView::CENTER);
+
+  constexpr int kImageSkiaSize = 4;
+  SkBitmap bitmap;
+  bitmap.allocN32Pixels(kImageSkiaSize, kImageSkiaSize);
+  gfx::ImageSkia image_skia = gfx::ImageSkia::CreateFrom1xBitmap(bitmap);
+  image_view()->SetImage(image_skia);
+  widget()->GetContentsView()->Layout();
+  EXPECT_NE(gfx::Size(), image_skia.size());
+
+  // With no changes to the size / padding of |image_view|, the origin of
+  // |image_skia| is the same as the origin of |image_view|.
+  EXPECT_EQ(0, CurrentImageOriginForParam());
+
+  // Test insets are always respected in LTR and RTL.
+  constexpr int kInset = 5;
+  image_view()->SetBorder(CreateEmptyBorder(gfx::Insets(kInset)));
+  widget()->GetContentsView()->Layout();
+  EXPECT_EQ(kInset, CurrentImageOriginForParam());
+
+  SetRTL(true);
+  widget()->GetContentsView()->Layout();
+  EXPECT_EQ(kInset, CurrentImageOriginForParam());
+
+  // Check this still holds true when the insets are asymmetrical.
+  constexpr int kLeadingInset = 4;
+  constexpr int kTrailingInset = 6;
+  image_view()->SetBorder(CreateEmptyBorder(
+      gfx::Insets(/*top=*/kLeadingInset, /*left=*/kLeadingInset,
+                  /*bottom=*/kTrailingInset, /*right=*/kTrailingInset)));
+  widget()->GetContentsView()->Layout();
+  EXPECT_EQ(kLeadingInset, CurrentImageOriginForParam());
+
+  SetRTL(false);
+  widget()->GetContentsView()->Layout();
+  EXPECT_EQ(kLeadingInset, CurrentImageOriginForParam());
+}
+
+INSTANTIATE_TEST_CASE_P(,
+                        ImageViewTest,
+                        ::testing::Values(Axis::kHorizontal, Axis::kVertical));
+
+}  // namespace views