[ash-md] Reduces dimensions of texture layers in overview mode

ash/common/wm/overview/window_selector_item.cc

Index: ash/common/wm/overview/window_selector_item.cc
diff --git a/ash/common/wm/overview/window_selector_item.cc b/ash/common/wm/overview/window_selector_item.cc
index c225ef9fd2ba94c1681a74a851a2efb17dc6e4b6..465fea291f3f150307d96786fd7f05ba56cd3be3 100644
--- a/ash/common/wm/overview/window_selector_item.cc
+++ b/ash/common/wm/overview/window_selector_item.cc
@@ -76,9 +76,6 @@ static const SkColor kLabelExitColor = SkColorSetARGB(255, 90, 90, 90);
 // Corner radius for the selection tiles.
 static int kLabelBackgroundRadius = 2;
 
-// Vertical padding for the label, on top of it.
-static const int kVerticalLabelPadding = 20;
-
 // Horizontal padding for the label, on both sides.
 static const int kHorizontalLabelPadding = 8;
 
@@ -125,6 +122,51 @@ void SetupFadeInAfterLayout(views::Widget* widget) {
   window->SetOpacity(1.0f);
 }
 
+// A Button that has a listener and listens to mouse clicks on the visible part
+// of an overview window.
+class ShieldButton : public views::CustomButton {
+ public:
+  ShieldButton(views::ButtonListener* listener, const base::string16& name)
+      : views::CustomButton(listener) {
+    SetAccessibleName(name);
+  }
+  ~ShieldButton() override {}
+
+  // Resets the listener so that the listener can go out of scope.
+  void ResetListener() { listener_ = nullptr; }

[tdanderson, 2017/01/16 23:43:29]

I know this is re-use of existing code, but I'm not entirely convinced why we
need to have the ability to reset the listener (i.e., why we need to call
ResetListener() on line 446), and I don't find the comment on line 135 helpful.
Can you give a quick explanation for what I'm missing?

[varkha, 2017/01/17 17:39:05]

Done (edited comment for clarity).

+
+ protected:
+  // views::View:
+  const char* GetClassName() const override { return "ShieldButton"; }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ShieldButton);
+};
+
+// A Label used to display caption text above the windows in overview mode.
+class OverviewLabel : public views::Label {

[tdanderson, 2017/01/16 23:43:29]

I wonder if introducing this class is really necessary since it does not have
any of its own members and the only method it overrides is GetClassName().
Instead, consider instantiating an object as a views::Label followed immediately
by lines 150-157.

[varkha, 2017/01/17 17:39:05]

Done.

+ public:
+  OverviewLabel(const base::string16& text) : views::Label(text) {

[tdanderson, 2017/01/16 23:43:29]

nit: explicit

[varkha, 2017/01/17 17:39:05]

Acknowledged.

+    SetHorizontalAlignment(gfx::ALIGN_LEFT);
+    SetAutoColorReadabilityEnabled(false);
+    SetEnabledColor(kLabelColor);
+    // Tell the label what color it will be drawn onto. It will use whether the
+    // background color is opaque or transparent to decide whether to use
+    // subpixel
+    // rendering. Does not actually set the label's background color.

[tdanderson, 2017/01/16 23:43:29]

nit: line breaks in this comment block

[varkha, 2017/01/17 17:39:05]

Done.

+    SetBackgroundColor(kLabelBackgroundColor);
+  }
+
+  ~OverviewLabel() override {}
+
+ protected:
+  // views::Label:
+  const char* GetClassName() const override { return "OverviewLabel"; }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(OverviewLabel);
+};
+
 }  // namespace
 
 WindowSelectorItem::OverviewCloseButton::OverviewCloseButton(
@@ -169,7 +211,10 @@ class WindowSelectorItem::RoundedContainerView
         target_color_(background),
         current_value_(0),
         layer_(nullptr),
-        animation_(new gfx::SlideAnimation(this)) {}
+        animation_(new gfx::SlideAnimation(this)) {
+    SetPaintToLayer(true);
+    layer()->SetFillsBoundsOpaquely(false);
+  }
 
   ~RoundedContainerView() override { StopObservingLayerAnimations(); }
 
@@ -254,6 +299,8 @@ class WindowSelectorItem::RoundedContainerView
     canvas->DrawColor(target_color);
   }
 
+  const char* GetClassName() const override { return "RoundedContainerView"; }

[tdanderson, 2017/01/16 23:43:29]

Thanks for adding this.

[varkha, 2017/01/17 17:39:05]

Acknowledged.

+
  private:
   // gfx::AnimationDelegate:
   void AnimationEnded(const gfx::Animation* animation) override {
@@ -313,39 +360,25 @@ class WindowSelectorItem::RoundedContainerView
   DISALLOW_COPY_AND_ASSIGN(RoundedContainerView);
 };
 
-WindowSelectorItem::OverviewLabelButton::OverviewLabelButton(
-    views::ButtonListener* listener,
-    const base::string16& text)
-    : LabelButton(listener, text) {}
-
-WindowSelectorItem::OverviewLabelButton::~OverviewLabelButton() {}
-
-void WindowSelectorItem::OverviewLabelButton::SetBackgroundColorHint(
-    SkColor color) {
-  // Tell the label what color it will be drawn onto. It will use whether the
-  // background color is opaque or transparent to decide whether to use subpixel
-  // rendering. Does not actually set the label's background color.
-  label()->SetBackgroundColor(color);
-}
-
-gfx::Rect WindowSelectorItem::OverviewLabelButton::GetChildAreaBounds() {
-  gfx::Rect bounds = GetLocalBounds();
-  bounds.Inset(padding_ + gfx::Insets(0, kHorizontalLabelPadding));
-  return bounds;
-}
-
 // Container View that has an item label and a close button as children.

[tdanderson, 2017/01/16 23:43:29]

This could use some clarification about what CaptionContainerView owns: the
label, close button, as well as the window region. With the comment as-is, line
392 makes it seem like |listener_button_| is only covering the label and close
button.

[varkha, 2017/01/17 17:39:05]

Done.

 class WindowSelectorItem::CaptionContainerView : public views::View {
  public:
-  CaptionContainerView(WindowSelectorItem::OverviewLabelButton* label,
+  CaptionContainerView(ButtonListener* listener,
+                       views::Label* label,
                        views::ImageButton* close_button,
                        WindowSelectorItem::RoundedContainerView* background)
-      : label_(label), close_button_(close_button), background_(background) {
-    AddChildView(background_);
-    AddChildView(label_);
-    AddChildView(close_button_);
+      : listener_button_(new ShieldButton(listener, label->text())),
+        background_(background),
+        label_(label),
+        close_button_(close_button) {
+    background_->AddChildView(label_);
+    background_->AddChildView(close_button_);

[tdanderson, 2017/01/16 23:43:29]

optional: as you explained in person, it may be worthwhile providing a comment
that the close button has a higher z-order than the listener button (i.e., is
not obscured by the listener button) and is thus still able to receive its own
events.

[varkha, 2017/01/17 17:39:05]

Done.

+    listener_button_->AddChildView(background_);
+    AddChildView(listener_button_);
   }
 
+  ShieldButton* listener_button() { return listener_button_; }
+
  protected:
   // views::View:
   void Layout() override {
@@ -356,25 +389,31 @@ class WindowSelectorItem::CaptionContainerView : public views::View {
     // events from reaching the transformed window in overview.
     gfx::Rect bounds(GetLocalBounds());
     bounds.Inset(kWindowSelectorMargin, kWindowSelectorMargin);
-    gfx::Rect background_bounds(bounds);
-    background_bounds.set_height(close_button_->GetPreferredSize().height());
-    background_->SetBoundsRect(background_bounds);
+    listener_button_->SetBoundsRect(bounds);
 
     const int visible_height = close_button_->GetPreferredSize().height();
-    gfx::Insets label_padding(0, 0, bounds.height() - visible_height,
-                              visible_height);
-    label_->set_padding(label_padding);
+    gfx::Rect background_bounds(gfx::Rect(bounds.size()));
+    background_bounds.set_height(visible_height);
+    background_->SetBoundsRect(background_bounds);
+
+    bounds = background_bounds;
+    bounds.Inset(kHorizontalLabelPadding, 0,
+                 kHorizontalLabelPadding + visible_height, 0);
     label_->SetBoundsRect(bounds);
-    bounds.set_x(bounds.right() - visible_height);
+
+    bounds = background_bounds;
+    bounds.set_x(bounds.width() - visible_height);
     bounds.set_width(visible_height);
-    bounds.set_height(visible_height);
     close_button_->SetBoundsRect(bounds);
   }
 
+  const char* GetClassName() const override { return "CaptionContainerView"; }
+
  private:
-  WindowSelectorItem::OverviewLabelButton* label_;
-  views::ImageButton* close_button_;
+  ShieldButton* listener_button_;
   WindowSelectorItem::RoundedContainerView* background_;
+  views::Label* label_;
+  views::ImageButton* close_button_;
 
   DISALLOW_COPY_AND_ASSIGN(CaptionContainerView);
 };
@@ -387,7 +426,7 @@ WindowSelectorItem::WindowSelectorItem(WmWindow* window,
       in_bounds_update_(false),
       selected_(false),
       caption_container_view_(nullptr),
-      window_label_button_view_(nullptr),
+      window_label_view_(nullptr),
       close_button_(new OverviewCloseButton(this)),
       window_selector_(window_selector),
       background_view_(nullptr) {
@@ -404,7 +443,7 @@ WmWindow* WindowSelectorItem::GetWindow() {
 }
 
 void WindowSelectorItem::RestoreWindow() {
-  window_label_button_view_->ResetListener();
+  caption_container_view_->listener_button()->ResetListener();
   close_button_->ResetListener();
   transform_window_.RestoreWindow();
   if (background_view_) {
@@ -486,8 +525,8 @@ void WindowSelectorItem::SetSelected(bool selected) {
 }
 
 void WindowSelectorItem::SendAccessibleSelectionEvent() {
-  window_label_button_view_->NotifyAccessibilityEvent(ui::AX_EVENT_SELECTION,
-                                                      true);
+  caption_container_view_->listener_button()->NotifyAccessibilityEvent(
+      ui::AX_EVENT_SELECTION, true);
 }
 
 void WindowSelectorItem::CloseWindow() {
@@ -528,7 +567,7 @@ void WindowSelectorItem::ButtonPressed(views::Button* sender,
     CloseWindow();
     return;
   }
-  CHECK(sender == window_label_button_view_);
+  CHECK(sender == caption_container_view_->listener_button());
   window_selector_->SelectWindow(transform_window_.window());
 }
 
@@ -540,8 +579,8 @@ void WindowSelectorItem::OnWindowDestroying(WmWindow* window) {
 void WindowSelectorItem::OnWindowTitleChanged(WmWindow* window) {
   // TODO(flackr): Maybe add the new title to a vector of titles so that we can
   // filter any of the titles the window had while in the overview session.
-  window_label_button_view_->SetText(window->GetTitle());
-  UpdateCloseButtonAccessibilityName();
+  window_label_view_->SetText(window->GetTitle());
+  UpdateButtonAccessibilityName();
 }
 
 float WindowSelectorItem::GetItemScale(const gfx::Size& size) {
@@ -587,28 +626,6 @@ void WindowSelectorItem::SetOpacity(float opacity) {
   transform_window_.SetOpacity(opacity);
 }
 
-void WindowSelectorItem::UpdateWindowLabel(
-    const gfx::Rect& window_bounds,
-    OverviewAnimationType animation_type) {
-  if (!window_label_->IsVisible()) {
-    window_label_->Show();
-    SetupFadeInAfterLayout(window_label_.get());
-  }
-
-  gfx::Rect label_bounds = root_window_->ConvertRectFromScreen(window_bounds);
-  window_label_button_view_->set_padding(
-      gfx::Insets(label_bounds.height() - kVerticalLabelPadding, 0, 0, 0));
-  std::unique_ptr<ScopedOverviewAnimationSettings> animation_settings =
-      ScopedOverviewAnimationSettingsFactory::Get()
-          ->CreateOverviewAnimationSettings(
-              animation_type,
-              WmLookup::Get()->GetWindowForWidget(window_label_.get()));
-
-  WmWindow* window_label_window =
-      WmLookup::Get()->GetWindowForWidget(window_label_.get());
-  window_label_window->SetBounds(label_bounds);
-}
-
 void WindowSelectorItem::CreateWindowLabel(const base::string16& title) {
   background_view_ = new RoundedContainerView(this, transform_window_.window(),
                                               kLabelBackgroundRadius,
@@ -619,6 +636,7 @@ void WindowSelectorItem::CreateWindowLabel(const base::string16& title) {
   params_label.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
   params_label.opacity = views::Widget::InitParams::TRANSLUCENT_WINDOW;
   params_label.visible_on_all_workspaces = true;
+  params_label.layer_type = ui::LAYER_NOT_DRAWN;
   params_label.name = "OverviewModeLabel";
   params_label.activatable =
       views::Widget::InitParams::Activatable::ACTIVATABLE_DEFAULT;
@@ -631,10 +649,7 @@ void WindowSelectorItem::CreateWindowLabel(const base::string16& title) {
           &params_label);
   window_label_->set_focus_on_creation(false);
   window_label_->Init(params_label);
-  window_label_button_view_ = new OverviewLabelButton(this, title);
-  window_label_button_view_->SetBorder(views::NullBorder());
-  window_label_button_view_->SetEnabledTextColors(kLabelColor);
-  window_label_button_view_->set_animate_on_state_change(false);
+  window_label_view_ = new OverviewLabel(title);
   WmWindow* label_window =
       WmLookup::Get()->GetWindowForWidget(window_label_.get());
   if (transform_window_.GetTopInset()) {
@@ -648,14 +663,10 @@ void WindowSelectorItem::CreateWindowLabel(const base::string16& title) {
     label_window->GetParent()->StackChildBelow(label_window,
                                                transform_window_.window());
   }
-  window_label_button_view_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-  // Hint at the background color that the label will be drawn onto (for
-  // subpixel antialiasing). Does not actually set the background color.
-  window_label_button_view_->SetBackgroundColorHint(kLabelBackgroundColor);
   caption_container_view_ = new CaptionContainerView(
-      window_label_button_view_, close_button_, background_view_);
+      this, window_label_view_, close_button_, background_view_);
   window_label_->SetContentsView(caption_container_view_);
-  window_label_button_view_->SetVisible(false);
+  window_label_view_->SetVisible(false);
   window_label_->SetOpacity(0);
   window_label_->Show();
 
@@ -704,8 +715,8 @@ void WindowSelectorItem::UpdateHeaderLayout(
       background_view_->set_color(kLabelExitColor);
     }
   }
-  if (!window_label_button_view_->visible()) {
-    window_label_button_view_->SetVisible(true);
+  if (!window_label_view_->visible()) {
+    window_label_view_->SetVisible(true);
     SetupFadeInAfterLayout(window_label_.get());
   }
   WmWindow* window_label_window =
@@ -752,10 +763,9 @@ void WindowSelectorItem::AnimateOpacity(float opacity,
   window_label_window->SetOpacity(header_opacity);
 }
 
-void WindowSelectorItem::UpdateCloseButtonAccessibilityName() {
-  close_button_->SetAccessibleName(l10n_util::GetStringFUTF16(
-      IDS_ASH_OVERVIEW_CLOSE_ITEM_BUTTON_ACCESSIBLE_NAME,
-      GetWindow()->GetTitle()));
+void WindowSelectorItem::UpdateButtonAccessibilityName() {

[tdanderson, 2017/01/16 23:43:29]

I see you are removing IDS_ASH_OVERVIEW_CLOSE_ITEM_BUTTON_ACCESSIBLE_NAME which
contained the string "Close". Does this mean that a ChromeVox user has no way of
closing windows in overview mode (i.e., they can just traverse the windows and
select one of them)?

[varkha, 2017/01/17 17:39:05]

This was already the case. I feel that the Ctrl+W provides adequate albeit not
discoverable way to close windows in overview using keyboard and having a tab
stop for close button would hinder overview mode usability.
IDS_ASH_OVERVIEW_CLOSE_ITEM_BUTTON_ACCESSIBLE_NAME was essentially unused for a
while so I have removed it.

+  caption_container_view_->listener_button()->SetAccessibleName(
+      GetWindow()->GetTitle());
 }
 
 void WindowSelectorItem::FadeOut(std::unique_ptr<views::Widget> widget) {