[ash-md] Updated layout of Brightness and Audio system menu rows to the material design spec.

ash/common/system/audio/volume_view.cc

 // Copyright 2014 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 <algorithm>
+
 #include "ash/common/system/audio/volume_view.h"
 
-#include "ash/common/ash_constants.h"
 #include "ash/common/material_design/material_design_controller.h"
 #include "ash/common/metrics/user_metrics_action.h"
 #include "ash/common/system/audio/tray_audio.h"
 #include "ash/common/system/audio/tray_audio_delegate.h"
+#include "ash/common/system/tray/actionable_view.h"
 #include "ash/common/system/tray/system_tray_item.h"
 #include "ash/common/system/tray/tray_constants.h"
 #include "ash/common/system/tray/tray_popup_item_container.h"
+#include "ash/common/system/tray/tray_popup_utils.h"
+#include "ash/common/system/tray/tri_view.h"
 #include "ash/common/wm_shell.h"
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "grit/ash_resources.h"
 #include "grit/ash_strings.h"
 #include "ui/accessibility/ax_node_data.h"
 #include "ui/base/resource/resource_bundle.h"
-#include "ui/gfx/canvas.h"
 #include "ui/gfx/image/image_skia_operations.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/gfx/vector_icon_types.h"
 #include "ui/views/background.h"
 #include "ui/views/border.h"
-#include "ui/views/controls/button/image_button.h"
+#include "ui/views/controls/button/custom_button.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/separator.h"
 #include "ui/views/controls/slider.h"
 #include "ui/views/layout/box_layout.h"
-#include "ui/views/painter.h"
+#include "ui/views/layout/fill_layout.h"
 
 namespace {
 const int kVolumeImageWidth = 25;
 const int kVolumeImageHeight = 25;
 const int kSeparatorSize = 3;
 const int kSeparatorVerticalInset = 8;
-const int kSliderRightPaddingToVolumeViewEdge = 17;
-const int kExtraPaddingBetweenBarAndMore = 10;
-const int kExtraPaddingBetweenIconAndSlider = 8;
 const int kBoxLayoutPadding = 2;
 
 // IDR_AURA_UBER_TRAY_VOLUME_LEVELS contains 5 images,
 // The one for mute is at the 0 index and the other
 // four are used for ascending volume levels.
 const int kVolumeLevels = 4;
 
 const gfx::VectorIcon* const kVolumeLevelIcons[] = {
     &ash::kSystemMenuVolumeMuteIcon,    // Muted.
     &ash::kSystemMenuVolumeLowIcon,     // Low volume.
     &ash::kSystemMenuVolumeMediumIcon,  // Medium volume.
     &ash::kSystemMenuVolumeHighIcon,    // High volume.
     &ash::kSystemMenuVolumeHighIcon,    // Full volume.
 };
 
 }  // namespace
 
 namespace ash {
 namespace tray {
 
-class VolumeButton : public views::ToggleImageButton {
+class VolumeButton : public ButtonListenerActionableView {
  public:
-  VolumeButton(views::ButtonListener* listener,
+  VolumeButton(SystemTrayItem* owner,
+               views::ButtonListener* listener,
                system::TrayAudioDelegate* audio_delegate)
-      : views::ToggleImageButton(listener),
+      : ButtonListenerActionableView(owner, listener),
         audio_delegate_(audio_delegate),
+        image_(TrayPopupUtils::CreateMainImageView()),
         image_index_(-1) {
+    TrayPopupUtils::ConfigureContainer(TriView::Container::START, this);
     SetFocusBehavior(FocusBehavior::ALWAYS);
-    SetFocusPainter(views::Painter::CreateSolidFocusPainter(

[tdanderson, 2016/11/09 01:38:33]

Looks like you're inheriting the focus painter from ActionableView, right? (did
you double check tabbing through this row in non-MD and MD?)

[bruthig, 2016/11/10 02:54:54]

Yes the Mute button maintains the correct keyboard focus and ChromeVox feedback.

I did notice that with my change that the DeviceType/More button was now
focus-able when it was not before.  I have fixed this change to make it not
focus-able but created http://crbug.com/663767 assuming that it should be.

Good catch :)

-        kFocusBorderColor, gfx::Insets(1, 1, 1, 1)));
-    SetImageAlignment(ALIGN_CENTER, ALIGN_MIDDLE);
-    if (!MaterialDesignController::IsSystemTrayMenuMaterial()) {
-      image_ = ui::ResourceBundle::GetSharedInstance().GetImageNamed(
-          IDR_AURA_UBER_TRAY_VOLUME_LEVELS);
-    }
+    AddChildView(image_);
+    if (MaterialDesignController::IsSystemTrayMenuMaterial())
+      SetInkDropMode(InkDropMode::ON);
     Update();
+
+    set_notify_enter_exit_on_child(true);
   }
 
   ~VolumeButton() override {}
 
   void Update() {
     float level =
         static_cast<float>(audio_delegate_->GetOutputVolumeLevel()) / 100.0f;
     int volume_levels = MaterialDesignController::IsSystemTrayMenuMaterial()
                             ? arraysize(kVolumeLevelIcons) - 1
                             : kVolumeLevels;
     int image_index =
         audio_delegate_->IsOutputAudioMuted()
             ? 0
             : (level == 1.0 ? volume_levels
                             : std::max(1, static_cast<int>(std::ceil(
                                               level * (volume_levels - 1)))));
     if (image_index != image_index_) {
       gfx::ImageSkia image_skia;
       if (MaterialDesignController::IsSystemTrayMenuMaterial()) {
         image_skia = gfx::CreateVectorIcon(*kVolumeLevelIcons[image_index],
                                            kMenuIconColor);
       } else {
         gfx::Rect region(0, image_index * kVolumeImageHeight, kVolumeImageWidth,
                          kVolumeImageHeight);
+        gfx::Image image =
+            ui::ResourceBundle::GetSharedInstance().GetImageNamed(
+                IDR_AURA_UBER_TRAY_VOLUME_LEVELS);
         image_skia = gfx::ImageSkiaOperations::ExtractSubset(
-            *(image_.ToImageSkia()), region);
+            *(image.ToImageSkia()), region);
       }
-      SetImage(views::CustomButton::STATE_NORMAL, &image_skia);
+      image_->SetImage(&image_skia);
       image_index_ = image_index;
     }
   }
 
  private:
   // views::View:
-  gfx::Size GetPreferredSize() const override {
-    gfx::Size size = views::ToggleImageButton::GetPreferredSize();
-    size.set_height(GetTrayConstant(TRAY_POPUP_ITEM_HEIGHT));
-    return size;
-  }
-
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
     ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance();
     node_data->SetName(
         bundle.GetLocalizedString(IDS_ASH_STATUS_TRAY_VOLUME_MUTE));
     node_data->role = ui::AX_ROLE_TOGGLE_BUTTON;
     if (audio_delegate_->IsOutputAudioMuted())
       node_data->AddStateFlag(ui::AX_STATE_PRESSED);
   }
 
   // views::CustomButton:
   void StateChanged() override {
     if (state() == STATE_HOVERED || state() == STATE_PRESSED) {
-      set_background(
-          views::Background::CreateSolidBackground(kHoverBackgroundColor));
+      if (!MaterialDesignController::IsSystemTrayMenuMaterial()) {
+        set_background(
+            views::Background::CreateSolidBackground(kHoverBackgroundColor));
+      }
     } else {
       set_background(nullptr);
     }
   }
 
   system::TrayAudioDelegate* audio_delegate_;
-  gfx::Image image_;
+  views::ImageView* image_;
   int image_index_;
 
   DISALLOW_COPY_AND_ASSIGN(VolumeButton);
 };
 
 VolumeView::VolumeView(SystemTrayItem* owner,
                        system::TrayAudioDelegate* audio_delegate,
                        bool is_default_view)
-    : ActionableView(owner),
+    : owner_(owner),
+      tri_view_(TrayPopupUtils::CreateMultiTargetRowView()),
       audio_delegate_(audio_delegate),
-      icon_(NULL),
-      slider_(NULL),
-      device_type_(NULL),
-      more_(NULL),
+      more_button_(nullptr),
+      icon_(nullptr),
+      slider_(nullptr),
+      separator_(nullptr),
+      device_type_(nullptr),
       is_default_view_(is_default_view) {
   SetFocusBehavior(FocusBehavior::NEVER);
-  views::BoxLayout* box_layout = new views::BoxLayout(
-      views::BoxLayout::kHorizontal, 0, 0, kBoxLayoutPadding);
-  box_layout->SetDefaultFlex(0);
-  SetLayoutManager(box_layout);
+  SetLayoutManager(new views::FillLayout);
+  AddChildView(tri_view_);
 
-  icon_ = new VolumeButton(this, audio_delegate_);
-  icon_->SetBorder(views::CreateEmptyBorder(0, kTrayPopupPaddingHorizontal, 0,
-                                            kExtraPaddingBetweenIconAndSlider));
-  AddChildView(icon_);
-  slider_ = views::Slider::CreateSlider(
-      ash::MaterialDesignController::IsSystemTrayMenuMaterial(), this);
+  icon_ = new VolumeButton(owner, this, audio_delegate_);
+  tri_view_->AddView(TriView::Container::START, icon_);
 
-  if (ash::MaterialDesignController::IsSystemTrayMenuMaterial()) {
-    slider_->SetBorder(views::CreateEmptyBorder(
-        gfx::Insets(0, kTrayPopupSliderPaddingMD) + slider_->GetInsets()));
-  } else {
-    slider_->SetBorder(
-        views::CreateEmptyBorder(0, 0, 0, kTrayPopupPaddingBetweenItems));
-  }
-
-  slider_->set_focus_border_color(kFocusBorderColor);
+  slider_ = TrayPopupUtils::CreateSlider(this);
   slider_->SetValue(
       static_cast<float>(audio_delegate_->GetOutputVolumeLevel()) / 100.0f);
   slider_->SetAccessibleName(
       ui::ResourceBundle::GetSharedInstance().GetLocalizedString(
           IDS_ASH_STATUS_TRAY_VOLUME));
-  AddChildView(slider_);
-  box_layout->SetFlexForView(slider_, 1);
+  tri_view_->AddView(TriView::Container::CENTER, slider_);
 
-  separator_ = new views::Separator(views::Separator::VERTICAL);
-  separator_->SetColor(kButtonStrokeColor);
-  separator_->SetPreferredSize(kSeparatorSize);
-  separator_->SetBorder(views::CreateEmptyBorder(
-      kSeparatorVerticalInset, 0, kSeparatorVerticalInset, kBoxLayoutPadding));
+  more_button_ = new ButtonListenerActionableView(owner_, this);
+  TrayPopupUtils::ConfigureContainer(TriView::Container::END, more_button_);
 
-  more_region_ = new TrayPopupItemContainer(separator_, true);
-  more_region_->SetBorder(
-      views::CreateEmptyBorder(0, 0, 0, kTrayPopupPaddingBetweenItems));
-  AddChildView(more_region_);
+  device_type_ = TrayPopupUtils::CreateMoreImageView();
+  more_button_->AddChildView(device_type_);
 
-  device_type_ = new views::ImageView;
-  more_region_->AddChildView(device_type_);
-
-  more_ = new views::ImageView;
-  more_->EnableCanvasFlippingForRTLUI(true);
+  views::ImageView* more_arrow = TrayPopupUtils::CreateMoreImageView();
+  if (MaterialDesignController::IsSystemTrayMenuMaterial()) {
+    more_arrow->SetImage(
+        gfx::CreateVectorIcon(kSystemMenuArrowRightIcon, kMenuIconColor));

[tdanderson, 2016/11/09 01:38:33]

The arrow color may possibly change if the row is disabled (e.g., Bluetooth,
Network).

[bruthig, 2016/11/10 02:54:54]

I will defer that work but I have updated the spreadsheet accordingly.

+  } else {
+    more_arrow->SetImage(ui::ResourceBundle::GetSharedInstance()
+                             .GetImageNamed(IDR_AURA_UBER_TRAY_MORE)
+                             .ToImageSkia());
+  }
+  more_button_->AddChildView(more_arrow);
 
   if (MaterialDesignController::IsSystemTrayMenuMaterial()) {
-    more_->SetImage(
-        gfx::CreateVectorIcon(kSystemMenuArrowRightIcon, kMenuIconColor));
+    more_button_->SetInkDropMode(views::InkDropHostView::InkDropMode::ON);
+    tri_view_->AddView(TriView::Container::END, more_button_);
   } else {
-    more_->SetImage(ui::ResourceBundle::GetSharedInstance()
-                        .GetImageNamed(IDR_AURA_UBER_TRAY_MORE)
-                        .ToImageSkia());
+    separator_ = new views::Separator(views::Separator::VERTICAL);
+    separator_->SetColor(kButtonStrokeColor);
+    separator_->SetPreferredSize(kSeparatorSize);
+    separator_->SetBorder(views::CreateEmptyBorder(kSeparatorVerticalInset, 0,
+                                                   kSeparatorVerticalInset,
+                                                   kBoxLayoutPadding));
+
+    TrayPopupItemContainer* more_container_ =

[tdanderson, 2016/11/09 01:38:33]

If you mean for this to be local, remove the trailing _

[bruthig, 2016/11/10 02:54:54]

Done.

+        new TrayPopupItemContainer(separator_, true);
+    more_container_->SetLayoutManager(
+        new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, 0));
+    more_container_->AddChildView(more_button_);
+    tri_view_->AddView(TriView::Container::END, more_container_);
   }
 
-  more_region_->AddChildView(more_);
+  if (!is_default_view_)

[tdanderson, 2016/11/09 01:38:33]

Consider moving this earlier (before line 171) as an early return to avoid doing
all of the creation of the more button and icons if they're not even going to be
visible. i.e.,

set_background(views::Background::CreateSolidBackground(kBackgroundColor));
if (!is_default_view_) {
  tri_view_->SetContainerVisible(END, false);
  Update();
  return;
}

// ... lines 170-205

Update();

[bruthig, 2016/11/10 02:54:53]

Done.

+    tri_view_->SetContainerVisible(TriView::Container::END, false);
 
   set_background(views::Background::CreateSolidBackground(kBackgroundColor));
 
   Update();
 }
 
 VolumeView::~VolumeView() {}
 
 void VolumeView::Update() {
   icon_->Update();
@@ skipping 13 matching lines @@
   // It is possible that the volume was (un)muted, but the actual volume level
   // did not change. In that case, setting the value of the slider won't
   // trigger an update. So explicitly trigger an update.
   Update();
   slider_->set_enable_accessibility_events(true);
 }
 
 void VolumeView::UpdateDeviceTypeAndMore() {
   bool show_more = is_default_view_ && TrayAudio::ShowAudioDeviceMenu() &&
                    audio_delegate_->HasAlternativeSources();
-  if (!ash::MaterialDesignController::IsSystemTrayMenuMaterial()) {
-    slider_->SetBorder(views::CreateEmptyBorder(
-        0, 0, 0, show_more ? kTrayPopupPaddingBetweenItems
-                           : kSliderRightPaddingToVolumeViewEdge));
-  }
-  if (!show_more) {
-    more_region_->SetVisible(false);
+
+  if (!show_more)
     return;
-  }
 
   // Show output device icon if necessary.
   device_type_->SetVisible(false);
   if (MaterialDesignController::IsSystemTrayMenuMaterial()) {
     const gfx::VectorIcon& device_icon =
         audio_delegate_->GetActiveOutputDeviceVectorIcon();
     if (!device_icon.is_empty()) {
       device_type_->SetImage(
           gfx::CreateVectorIcon(device_icon, kMenuIconColor));
       device_type_->SetVisible(true);
     }
   } else {
     int device_icon = audio_delegate_->GetActiveOutputDeviceIconId();
     if (device_icon != system::TrayAudioDelegate::kNoAudioDeviceIcon) {
       device_type_->SetImage(ui::ResourceBundle::GetSharedInstance()
                                  .GetImageNamed(device_icon)
                                  .ToImageSkia());
       device_type_->SetVisible(true);
     }
   }
-  int spacing = kTrayPopupPaddingBetweenItems;
-  if (!device_type_->visible())
-    spacing += kExtraPaddingBetweenBarAndMore;
-  more_region_->SetLayoutManager(
-      new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, spacing));
-  more_region_->SetVisible(true);
 }
 
 void VolumeView::HandleVolumeUp(float level) {
   audio_delegate_->SetOutputVolumeLevel(level);
   if (audio_delegate_->IsOutputAudioMuted() &&
       level > audio_delegate_->GetOutputDefaultVolumeMuteLevel()) {
     audio_delegate_->SetOutputAudioIsMuted(false);
   }
 }
 
 void VolumeView::HandleVolumeDown(float level) {
   audio_delegate_->SetOutputVolumeLevel(level);
   if (!audio_delegate_->IsOutputAudioMuted() &&
       level <= audio_delegate_->GetOutputDefaultVolumeMuteLevel()) {
     audio_delegate_->SetOutputAudioIsMuted(true);
   } else if (audio_delegate_->IsOutputAudioMuted() &&
              level > audio_delegate_->GetOutputDefaultVolumeMuteLevel()) {
     audio_delegate_->SetOutputAudioIsMuted(false);
   }
 }
 
 void VolumeView::ButtonPressed(views::Button* sender, const ui::Event& event) {
-  if (sender != icon_) {
-    ActionableView::ButtonPressed(sender, event);
-    return;
+  if (sender == icon_) {
+    bool mute_on = !audio_delegate_->IsOutputAudioMuted();
+    audio_delegate_->SetOutputAudioIsMuted(mute_on);
+    if (!mute_on)
+      audio_delegate_->AdjustOutputVolumeToAudibleLevel();
+    icon_->Update();
+  } else if (sender == more_button_) {
+    if (!more_button_->visible())

[tdanderson, 2016/11/09 01:38:33]

Consider restructuring as 'else if (sender == more && more button visible) - or
dropping the visibility check entirely since I don't see how that could ever be
applicable

[bruthig, 2016/11/10 02:54:54]

Good point, dropped the visible() condition.

+      return;
+    owner_->TransitionDetailedView();
   }

[tdanderson, 2016/11/09 01:38:33]

optional nit: else NOTREACHED() ?

[bruthig, 2016/11/10 02:54:54]

Done.

-
-  bool mute_on = !audio_delegate_->IsOutputAudioMuted();
-  audio_delegate_->SetOutputAudioIsMuted(mute_on);
-  if (!mute_on)
-    audio_delegate_->AdjustOutputVolumeToAudibleLevel();
-  icon_->Update();
 }
 
 void VolumeView::SliderValueChanged(views::Slider* sender,
                                     float value,
                                     float old_value,
                                     views::SliderChangeReason reason) {
   if (reason == views::VALUE_CHANGED_BY_USER) {
     float new_volume = value * 100.0f;
     float current_volume = audio_delegate_->GetOutputVolumeLevel();
     // Do not call change audio volume if the difference is less than
     // 1%, which is beyond cras audio api's granularity for output volume.
     if (std::abs(new_volume - current_volume) < 1.0f)
       return;
     WmShell::Get()->RecordUserMetricsAction(
         is_default_view_ ? UMA_STATUS_AREA_CHANGED_VOLUME_MENU
                          : UMA_STATUS_AREA_CHANGED_VOLUME_POPUP);
     if (new_volume > current_volume)
       HandleVolumeUp(new_volume);
     else
       HandleVolumeDown(new_volume);
   }
   icon_->Update();
 }
 
-bool VolumeView::PerformAction(const ui::Event& event) {
-  if (!more_region_->visible())
-    return false;
-  owner()->TransitionDetailedView();
-  return true;
-}
-
 void VolumeView::OnBoundsChanged(const gfx::Rect& previous_bounds) {
   // Separator's prefered size is based on set bounds. When an empty bounds is
   // set on first layout this causes BoxLayout to ignore the separator. Reset
   // its height on each bounds change so that it is laid out properly.
-  separator_->SetSize(gfx::Size(kSeparatorSize, bounds().height()));
-}
-
-void VolumeView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
-  // Intentionally overrides ActionableView, leaving |state| unset. A slider
-  // childview exposes accessibility data.
+  if (separator_)
+    separator_->SetSize(gfx::Size(kSeparatorSize, bounds().height()));
 }
 
 }  // namespace tray
 }  // namespace ash