CrOS MD Menu - Some more user row tweaks.

ash/common/system/user/user_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 "ash/common/system/user/user_view.h"
 
 #include <algorithm>
 #include <utility>
 

[tdanderson, 2016/11/28 20:55:30]

Also specified in the bug is "'Sign-out' left and right padding should be 20dp"
- was that fixed in another CL / are you fixing that in a follow-up CL?

[Evan Stade, 2016/11/29 02:30:32]

I didn't actually notice that part of the bug --- guess it wasn't part of the
email thread? The change to the user name text was something I noticed
incidentally when trying to figure out the font styles for the add user label.

For some reason I changed the sign out padding from 20 to 8 in
https://codereview.chromium.org/2498293003 but I can't dig up the reasoning for
why I did that, so I've reverted that change (and fixed the height of the button
as well).

 #include "ash/common/material_design/material_design_controller.h"
 #include "ash/common/multi_profile_uma.h"
 #include "ash/common/popup_message.h"
 #include "ash/common/session/session_state_delegate.h"
 #include "ash/common/shell_delegate.h"
 #include "ash/common/system/tray/system_tray.h"
 #include "ash/common/system/tray/system_tray_controller.h"
 #include "ash/common/system/tray/system_tray_delegate.h"
 #include "ash/common/system/tray/tray_constants.h"
+#include "ash/common/system/tray/tray_popup_item_style.h"
 #include "ash/common/system/tray/tray_popup_label_button.h"
 #include "ash/common/system/tray/tray_popup_label_button_border.h"
 #include "ash/common/system/tray/tray_popup_utils.h"
 #include "ash/common/system/user/button_from_view.h"
 #include "ash/common/system/user/login_status.h"
 #include "ash/common/system/user/rounded_image_view.h"
 #include "ash/common/system/user/user_card_view.h"
 #include "ash/common/wm_lookup.h"
 #include "ash/common/wm_root_window_controller.h"
 #include "ash/common/wm_shell.h"
@@ skipping 68 matching lines @@
       MultiProfileUMA::SWITCH_ACTIVE_USER_BY_TRAY);
   delegate->SwitchActiveUser(delegate->GetUserInfo(user_index)->GetAccountId());
 }
 
 bool IsMultiProfileSupportedAndUserActive() {
   return WmShell::Get()->delegate()->IsMultiProfilesEnabled() &&
          !WmShell::Get()->GetSessionStateDelegate()->IsUserSessionBlocked();
 }
 
 // Creates the view shown in the user switcher popup ("AddUserMenuOption").
-views::View* CreateAddUserView(AddUserSessionPolicy policy) {
+views::View* CreateAddUserView(AddUserSessionPolicy policy,
+                               views::ButtonListener* listener) {
   DCHECK(UseMd());
   auto view = new views::View;
-  auto layout = new views::BoxLayout(
-      views::BoxLayout::kHorizontal, (kMenuButtonSize - kMenuIconSize) / 2,
-      kMenuSeparatorVerticalPadding, kTrayPopupPaddingBetweenItems);
+  const int icon_padding = (kMenuButtonSize - kMenuIconSize) / 2;
+  auto layout =
+      new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0,
+                           kTrayPopupLabelHorizontalPadding + icon_padding);
   layout->set_minimum_cross_axis_size(
-      policy == AddUserSessionPolicy::ALLOWED
-          ? GetTrayConstant(TRAY_POPUP_ITEM_MIN_HEIGHT)
-          : 56);
+      GetTrayConstant(TRAY_POPUP_ITEM_MIN_HEIGHT));
   view->SetLayoutManager(layout);
   view->set_background(
       views::Background::CreateSolidBackground(kBackgroundColor));
 
   int message_id = 0;
   switch (policy) {
     case AddUserSessionPolicy::ALLOWED: {
       message_id = IDS_ASH_STATUS_TRAY_SIGN_IN_ANOTHER_ACCOUNT;
 
       auto icon = new views::ImageView();
       icon->SetImage(
           gfx::CreateVectorIcon(kSystemMenuNewUserIcon, kMenuIconColor));
       view->AddChildView(icon);
       break;
     }
     case AddUserSessionPolicy::ERROR_NOT_ALLOWED_PRIMARY_USER:
       message_id = IDS_ASH_STATUS_TRAY_MESSAGE_NOT_ALLOWED_PRIMARY_USER;
       break;
     case AddUserSessionPolicy::ERROR_MAXIMUM_USERS_REACHED:
       message_id = IDS_ASH_STATUS_TRAY_MESSAGE_CANNOT_ADD_USER;
       break;
     case AddUserSessionPolicy::ERROR_NO_ELIGIBLE_USERS:
       message_id = IDS_ASH_STATUS_TRAY_MESSAGE_OUT_OF_USERS;
       break;
   }
 
   auto command_label = new views::Label(l10n_util::GetStringUTF16(message_id));
   command_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   command_label->SetMultiLine(true);
+
+  TrayPopupItemStyle label_style(
+      TrayPopupItemStyle::FontStyle::DETAILED_VIEW_LABEL);
+  int vertical_padding = kMenuSeparatorVerticalPadding;
+  if (policy != AddUserSessionPolicy::ALLOWED) {
+    label_style.set_font_style(TrayPopupItemStyle::FontStyle::CAPTION);
+    label_style.set_color_style(TrayPopupItemStyle::ColorStyle::INACTIVE);
+    vertical_padding += kMenuSeparatorVerticalPadding;

[tdanderson, 2016/11/28 20:55:30]

From the bug, Sebastien has specified "the entire additional row ("sign in
another user" or "All available...) need to be moved down 8dp" but here it looks
like you're giving 4dp of extra padding for "Sign in another user" but 8dp extra
padding to "All available...". Just wanted to double check this is intentional.

[Evan Stade, 2016/11/29 02:30:33]

I added 4 with add_user_padding (because that 4 is not supposed to be part of
the ripple). I added 4 here for "sign in another", making 8. I added 4 more top
and bottom for "all available" which is supposed to be 8dp taller (not
explicitly mentioned in the bug, but this is where the previous 56 came from).

+  }
+  label_style.SetupLabel(command_label);
   view->AddChildView(command_label);
+  view->SetBorder(views::CreateEmptyBorder(vertical_padding, icon_padding,
+                                           vertical_padding,
+                                           kTrayPopupLabelHorizontalPadding));
+  if (policy == AddUserSessionPolicy::ALLOWED) {
+    auto button =
+        new ButtonFromView(view, listener, TrayPopupInkDropStyle::INSET_BOUNDS,
+                           false, gfx::Insets());
+    button->SetAccessibleName(
+        l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_SIGN_IN_ANOTHER_ACCOUNT));
+    return button;

[tdanderson, 2016/11/28 20:55:30]

Thanks for making this a button only when it needs to be.

[Evan Stade, 2016/11/29 02:30:33]

Acknowledged.

+  }
+
   return view;
 }
 
 class UserViewMouseWatcherHost : public views::MouseWatcherHost {
  public:
   explicit UserViewMouseWatcherHost(const gfx::Rect& screen_area)
       : screen_area_(screen_area) {}
   ~UserViewMouseWatcherHost() override {}
 
   // Implementation of MouseWatcherHost.
@@ skipping 445 matching lines @@
           &params);
   add_menu_option_->Init(params);
 
   const SessionStateDelegate* delegate =
       WmShell::Get()->GetSessionStateDelegate();
   const AddUserSessionPolicy add_user_policy =
       delegate->GetAddUserSessionPolicy();
   add_user_enabled_ = add_user_policy == AddUserSessionPolicy::ALLOWED;
 
   if (UseMd()) {
-    ButtonFromView* button = new ButtonFromView(
-        CreateAddUserView(add_user_policy), add_user_enabled_ ? this : nullptr,
-        IsActiveUser() ? TrayPopupInkDropStyle::INSET_BOUNDS
-                       : TrayPopupInkDropStyle::FILL_BOUNDS,
-        false, gfx::Insets());
-    button->SetAccessibleName(
-        l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_SIGN_IN_ANOTHER_ACCOUNT));
-    button->ForceBorderVisible(true);
-
     // Position the widget on top of the user card view (which is still in the
     // system menu). The top half of the widget will be transparent to allow
     // the active user to show through.
     gfx::Rect bounds = user_card_view_->GetBoundsInScreen();
     bounds.set_width(bounds.width() + kSeparatorWidth);
     int row_height = bounds.height();
 
     views::View* container = new AddUserWidgetContents(
         base::Bind(&UserView::RemoveAddUserMenuOption, base::Unretained(this)));
     container->SetBorder(views::CreatePaddedBorder(
         views::CreateSolidSidedBorder(0, 0, 0, kSeparatorWidth,
                                       kBackgroundColor),
         gfx::Insets(row_height, 0, 0, 0)));
+    views::View* add_user_padding = new views::View();
+    add_user_padding->SetBorder(views::CreateSolidSidedBorder(
+        kMenuSeparatorVerticalPadding, 0, 0, 0, kBackgroundColor));
+    views::View* add_user_view = CreateAddUserView(add_user_policy, this);
+    add_user_padding->AddChildView(add_user_view);
+    add_user_padding->SetLayoutManager(new views::FillLayout());
+    container->AddChildView(add_user_padding);
     container->SetLayoutManager(new views::FillLayout());
-    container->AddChildView(button);
     add_menu_option_->SetContentsView(container);
 
     bounds.set_height(container->GetPreferredSize().height());
     add_menu_option_->SetBounds(bounds);
 
     // Show the content.
     add_menu_option_->SetAlwaysOnTop(true);
     add_menu_option_->Show();
 
     // We activate the entry automatically if invoked with focus.
     if (add_user_enabled_ && user_card_view_->HasFocus()) {
-      button->GetFocusManager()->SetFocusedView(button);
-      user_card_view_->GetFocusManager()->SetFocusedView(button);
+      add_user_view->GetFocusManager()->SetFocusedView(add_user_view);
+      user_card_view_->GetFocusManager()->SetFocusedView(add_user_view);
     }
   } else {
     AddUserView* add_user_view =
         new AddUserView(static_cast<ButtonFromView*>(user_card_view_));
     ButtonFromView* button = new ButtonFromView(
         add_user_view, add_user_enabled_ ? this : nullptr,
         // Ignored in non-md.
         TrayPopupInkDropStyle::INSET_BOUNDS, add_user_enabled_, gfx::Insets(1));
     button->SetAccessibleName(
         l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_SIGN_IN_ANOTHER_ACCOUNT));
@@ skipping 64 matching lines @@
   focus_manager_ = nullptr;
   if (user_card_view_->GetFocusManager())
     user_card_view_->GetFocusManager()->ClearFocus();
   popup_message_.reset();
   mouse_watcher_.reset();
   add_menu_option_.reset();
 }
 
 }  // namespace tray
 }  // namespace ash