Move the close button to the Caption Bubble from the frame.

This allows us to draw it in exactly the place defined by
the spec. It will also make showing and hiding the close button
with animation easier later. In addition, when the expand button
is added it cannot be place in the BubbleFrameView, which
does not currently support buttons other than close.

Note: The spec called for the button to be 20x20, but here
I've got with 24x24 which is the default size for all
BubbleFrameView dialog bubbles. Tap targets should be as
large as possible for accessibility. The icon is still positioned
to spec with respect to the edge of the bubble view.

Bug: 1055150
Change-Id: I5949986ccc8682a778fdc215e1512b97f16345aa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2165869
Commit-Queue: Katie Dektar <katie@chromium.org>
Reviewed-by: Abigail Klein <abigailbklein@google.com>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#763580}

chrome/browser/ui/views/accessibility/caption_bubble.cc

@@ -18,8 +18,13 @@
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/geometry/insets.h"
 #include "ui/gfx/paint_vector_icon.h"
+#include "ui/strings/grit/ui_strings.h"
 #include "ui/views/bubble/bubble_dialog_delegate_view.h"
 #include "ui/views/bubble/bubble_frame_view.h"
+#include "ui/views/controls/button/button.h"
+#include "ui/views/controls/button/image_button.h"
+#include "ui/views/controls/button/image_button_factory.h"
+#include "ui/views/controls/highlight_path_generator.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/layout/box_layout.h"
@@ -35,6 +40,7 @@ static constexpr int kMaxHeightDip = kLineHeightDip * 2;
 static constexpr int kCornerRadiusDip = 8;
 static constexpr int kHorizontalMarginsDip = 6;
 static constexpr int kVerticalMarginsDip = 8;
+static constexpr int kCloseButtonMargin = 4;
 static constexpr double kPreferredAnchorWidthPercentage = 0.8;
 static constexpr int kMaxWidthDip = 548;
 static constexpr int kButtonPaddingDip = 48;
@@ -173,14 +179,29 @@ void CaptionBubble::OnWidgetBoundsChanged(views::Widget* widget,
 }
 
 void CaptionBubble::Init() {
-  auto* layout = SetLayoutManager(std::make_unique<views::FlexLayout>());
+  int content_top_bottom_margin = kHorizontalMarginsDip - kCloseButtonMargin;
+  int content_sides_margin = kVerticalMarginsDip - kCloseButtonMargin;
+
+  views::View* content_container = new views::View();
+  views::FlexLayout* layout = content_container->SetLayoutManager(
+      std::make_unique<views::FlexLayout>());
   layout->SetOrientation(views::LayoutOrientation::kVertical);
   layout->SetMainAxisAlignment(views::LayoutAlignment::kEnd);
+  layout->SetInteriorMargin(
+      gfx::Insets(content_top_bottom_margin, content_sides_margin));
   layout->SetDefault(
       views::kFlexBehaviorKey,
       views::FlexSpecification(views::MinimumFlexSizeRule::kPreferred,
                                views::MaximumFlexSizeRule::kPreferred,
                                /*adjust_height_for_width*/ true));
+  content_container->SetPreferredSize(
+      gfx::Size(kMaxWidthDip, kMaxHeightDip + kVerticalMarginsDip));
+
+  views::BoxLayout* main_layout =
+      SetLayoutManager(std::make_unique<views::BoxLayout>(
+          views::BoxLayout::Orientation::kVertical, gfx::Insets(0), 0));
+  main_layout->set_cross_axis_alignment(
+      views::BoxLayout::CrossAxisAlignment::kEnd);
 
   // TODO(crbug.com/1055150): Use system caption color scheme rather than
   // hard-coding the colors.
@@ -191,7 +212,7 @@ void CaptionBubble::Init() {
 
   auto label = std::make_unique<views::Label>();
   label->SetMultiLine(true);
-  label->SetMaximumWidth(kMaxWidthDip);
+  label->SetMaximumWidth(kMaxWidthDip - kVerticalMarginsDip);
   label->SetEnabledColor(SK_ColorWHITE);
   label->SetBackgroundColor(SK_ColorTRANSPARENT);
   label->SetHorizontalAlignment(gfx::HorizontalAlignment::ALIGN_LEFT);
@@ -228,18 +249,37 @@ void CaptionBubble::Init() {
       vector_icons::kErrorOutlineIcon, kErrorImageSizeDip, SK_ColorWHITE));
   error_icon->SetVisible(false);
 
-  SetPreferredSize(gfx::Size(kMaxWidthDip, kMaxHeightDip));
-  set_margins(gfx::Insets(kHorizontalMarginsDip, kVerticalMarginsDip));
-
-  title_ = AddChildView(std::move(title));
-  label_ = AddChildView(std::move(label));
-
-  error_icon_ = AddChildView(std::move(error_icon));
-  error_message_ = AddChildView(std::move(error_message));
+  auto close_button = views::CreateVectorImageButton(this);
+  views::SetImageFromVectorIcon(close_button.get(),
+                                vector_icons::kCloseRoundedIcon, SK_ColorWHITE);
+  // TODO(crbug.com/1055150): Use a custom string explaining we dismiss from the
+  // current tab on close, but leave the feature enabled.
+  close_button->SetTooltipText(l10n_util::GetStringUTF16(IDS_APP_CLOSE));
+  close_button->SizeToPreferredSize();
+  close_button->SetFocusForPlatform();
+  views::InstallCircleHighlightPathGenerator(close_button.get());
+
+  // TODO(crbug.com/1055150): On hover, show/hide the close button. At that
+  // time remove the height of close button size from SetPreferredSize.
+  SetPreferredSize(
+      gfx::Size(kMaxWidthDip, kMaxHeightDip + kCloseButtonMargin +
+                                  close_button->GetPreferredSize().height()));
+  set_margins(gfx::Insets(kCloseButtonMargin));
+
+  title_ = content_container->AddChildView(std::move(title));
+  label_ = content_container->AddChildView(std::move(label));
+
+  error_icon_ = content_container->AddChildView(std::move(error_icon));
+  error_message_ = content_container->AddChildView(std::move(error_message));
+
+  close_button_ = AddChildView(std::move(close_button));
+  AddChildView(content_container);
 }
 
 bool CaptionBubble::ShouldShowCloseButton() const {
-  return true;
+  // We draw our own close button so that we could show/hide it when the
+  // mouse moves, and so that in the future we can add an expand button.
+  return false;
 }
 
 views::NonClientFrameView* CaptionBubble::CreateNonClientFrameView(
@@ -253,6 +293,14 @@ views::NonClientFrameView* CaptionBubble::CreateNonClientFrameView(
   return frame;
 }
 
+void CaptionBubble::ButtonPressed(views::Button* sender,
+                                  const ui::Event& event) {
+  if (sender == close_button_) {
+    GetWidget()->CloseWithReason(
+        views::Widget::ClosedReason::kCloseButtonClicked);
+  }
+}
+
 void CaptionBubble::SetText(const std::string& text) {
   label_->SetText(base::ASCIIToUTF16(text));
   UpdateTitleVisibility();

chrome/browser/ui/views/accessibility/caption_bubble.h

@@ -8,9 +8,11 @@
 #include <string>
 
 #include "ui/views/bubble/bubble_dialog_delegate_view.h"
+#include "ui/views/controls/button/button.h"
 
 namespace views {
 class Label;
+class ImageButton;
 class ImageView;
 }
 
@@ -22,7 +24,8 @@ namespace captions {
 //  A caption bubble that floats above the BrowserView and shows automatically-
 //  generated text captions for audio and media streams from the current tab.
 //
-class CaptionBubble : public views::BubbleDialogDelegateView {
+class CaptionBubble : public views::BubbleDialogDelegateView,
+                      public views::ButtonListener {
  public:
   explicit CaptionBubble(views::View* anchor,
                          base::OnceClosure destroyed_callback);
@@ -46,6 +49,9 @@ class CaptionBubble : public views::BubbleDialogDelegateView {
   void OnWidgetBoundsChanged(views::Widget* widget,
                              const gfx::Rect& new_bounds) override;
 
+  // Views::ButtonListener:
+  void ButtonPressed(views::Button* sender, const ui::Event& event) override;
+
  private:
   friend class CaptionBubbleControllerViewsTest;
 
@@ -56,6 +62,7 @@ class CaptionBubble : public views::BubbleDialogDelegateView {
   views::Label* title_;
   views::Label* error_message_;
   views::ImageView* error_icon_;
+  views::ImageButton* close_button_;
 
   bool has_error_ = false;
 

chrome/browser/ui/views/accessibility/caption_bubble_controller_views_browsertest.cc

@@ -13,8 +13,11 @@
 #include "chrome/browser/ui/views/accessibility/caption_bubble.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/test/base/in_process_browser_test.h"
+#include "ui/events/base_event_utils.h"
+#include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
+#include "ui/views/test/widget_test.h"
 #include "ui/views/widget/widget.h"
 
 namespace captions {
@@ -54,6 +57,20 @@ class CaptionBubbleControllerViewsTest : public InProcessBrowserTest {
     return controller_ ? controller_->caption_widget_ : nullptr;
   }
 
+  void ClickCloseButton() {
+    CaptionBubble* bubble = GetBubble();
+    if (!bubble)
+      return;
+    views::test::WidgetDestroyedWaiter waiter(GetCaptionWidget());
+    bubble->close_button_->OnMousePressed(
+        ui::MouseEvent(ui::ET_MOUSE_PRESSED, gfx::Point(0, 0), gfx::Point(0, 0),
+                       ui::EventTimeForNow(), ui::EF_LEFT_MOUSE_BUTTON, 0));
+    bubble->close_button_->OnMouseReleased(ui::MouseEvent(
+        ui::ET_MOUSE_RELEASED, gfx::Point(0, 0), gfx::Point(0, 0),
+        ui::EventTimeForNow(), ui::EF_LEFT_MOUSE_BUTTON, 0));
+    waiter.Wait();
+  }
+
   // There may be some rounding errors as we do floating point math with ints.
   // Check that points are almost the same.
   void ExpectInBottomCenter(gfx::Rect anchor_bounds, gfx::Rect bubble_bounds) {
@@ -97,9 +114,11 @@ IN_PROC_BROWSER_TEST_F(CaptionBubbleControllerViewsTest, ShowsCaptionInBubble) {
 }
 
 IN_PROC_BROWSER_TEST_F(CaptionBubbleControllerViewsTest, LaysOutCaptionLabel) {
-  // A short caption is bottom-aligned with the bubble.
+  // A short caption is bottom-aligned with the bubble. The bubble bounds
+  // are inset by 4 dip of margin, add another 2 dip of margin for the label's
+  // container bounds to get 6 dip (spec).
   GetController()->OnCaptionReceived("Cats rock");
-  EXPECT_EQ(GetLabel()->GetBoundsInScreen().bottom(),
+  EXPECT_EQ(GetLabel()->GetBoundsInScreen().bottom() + 2,
             GetBubble()->GetBoundsInScreen().bottom());
 
   // Ensure overflow by using a very long caption, should still be aligned
@@ -110,7 +129,7 @@ IN_PROC_BROWSER_TEST_F(CaptionBubbleControllerViewsTest, LaysOutCaptionLabel) {
       "life, which have received widespread media coverage. At age 14, Swift "
       "became the youngest artist signed by the Sony/ATV Music publishing "
       "house and, at age 15, she signed her first record deal.");
-  EXPECT_EQ(GetLabel()->GetBoundsInScreen().bottom(),
+  EXPECT_EQ(GetLabel()->GetBoundsInScreen().bottom() + 2,
             GetBubble()->GetBoundsInScreen().bottom());
 }
 
@@ -253,7 +272,7 @@ IN_PROC_BROWSER_TEST_F(CaptionBubbleControllerViewsTest, BubblePositioning) {
 }
 
 IN_PROC_BROWSER_TEST_F(CaptionBubbleControllerViewsTest, ShowsAndHidesError) {
-  GetController()->OnCaptionReceived("Elephant's trunks average 6 feet long.");
+  GetController()->OnCaptionReceived("Elephants' trunks average 6 feet long.");
   EXPECT_TRUE(GetTitle()->GetVisible());
   EXPECT_TRUE(GetLabel()->GetVisible());
   EXPECT_FALSE(IsBubbleErrorMessageVisible());
@@ -275,4 +294,11 @@ IN_PROC_BROWSER_TEST_F(CaptionBubbleControllerViewsTest, ShowsAndHidesError) {
   EXPECT_TRUE(GetLabel()->GetVisible());
   EXPECT_FALSE(IsBubbleErrorMessageVisible());
 }
+
+IN_PROC_BROWSER_TEST_F(CaptionBubbleControllerViewsTest, CloseButtonCloses) {
+  GetController()->OnCaptionReceived("Elephants have 3-4 toenails per foot");
+  EXPECT_TRUE(GetCaptionWidget());
+  ClickCloseButton();
+  EXPECT_FALSE(GetCaptionWidget());
+}
 }  // namespace captions