[ash-md] Add on-screen keyboard toggle row in IME menu view.

ash/common/system/chromeos/ime_menu/ime_list_view.cc

Index: ash/common/system/chromeos/ime_menu/ime_list_view.cc
diff --git a/ash/common/system/chromeos/ime_menu/ime_list_view.cc b/ash/common/system/chromeos/ime_menu/ime_list_view.cc
index d3f13487349fddf83ca07b882259621ecbb82116..db245cde1068e9a602525c28d3607412a52928ae 100644
--- a/ash/common/system/chromeos/ime_menu/ime_list_view.cc
+++ b/ash/common/system/chromeos/ime_menu/ime_list_view.cc
@@ -7,11 +7,14 @@
 #include "ash/common/material_design/material_design_controller.h"
 #include "ash/common/system/tray/hover_highlight_view.h"
 #include "ash/common/system/tray/ime_info.h"
+#include "ash/common/system/tray/system_menu_button.h"
 #include "ash/common/system/tray/system_tray_delegate.h"
 #include "ash/common/system/tray/tray_constants.h"
 #include "ash/common/system/tray/tray_details_view.h"
 #include "ash/common/system/tray/tray_popup_header_button.h"
 #include "ash/common/system/tray/tray_popup_item_style.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 "grit/ash_resources.h"
 #include "grit/ash_strings.h"
@@ -23,15 +26,25 @@
 #include "ui/gfx/vector_icons_public.h"
 #include "ui/keyboard/keyboard_util.h"
 #include "ui/views/border.h"
-#include "ui/views/controls/button/label_button.h"
+#include "ui/views/controls/button/toggle_button.h"
+#include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/separator.h"
-#include "ui/views/layout/box_layout.h"
+#include "ui/views/layout/fill_layout.h"
+#include "ui/views/painter.h"
+#include "ui/views/view.h"
 #include "ui/views/widget/widget.h"
 
 namespace ash {
 namespace {
 
+const int kKeyboardRowkVerticalInset = 4;

[tdanderson, 2016/11/03 21:17:06]

nit: 'kKeyboardRowVerticalInset' instead of 'kKeyboardRowkVerticalInset' (no
extra k)

[Azure Wei, 2016/11/04 01:28:34]

Done.

+const int kKeyboardRowSeparatorThickness = 1;
+// const int kFocusBorderInset = 1;

[tdanderson, 2016/11/03 21:17:06]

Don't forget to delete this line if it's no longer needed.

[Azure Wei, 2016/11/04 01:28:34]

Done. Sorry about the silly mistake.

+const int kMaxReduceFontSize = -10;

[tdanderson, 2016/11/03 21:17:05]

nit: I would instead call this something with 'min' in it instead of 'max' for
clarity. Consider "kMinFontSizeDelta" and add a brief explanation.

[Azure Wei, 2016/11/04 01:28:34]

Done.

+
+const SkColor kKeyboardRowSeparatorColor = SkColorSetA(SK_ColorBLACK, 0x1F);
+
 // Creates a separator that will be used between the IME list items.
 views::Separator* CreateListItemSeparator() {
   views::Separator* separator =
@@ -72,41 +85,6 @@ class SelectableHoverHighlightView : public HoverHighlightView {
   DISALLOW_COPY_AND_ASSIGN(SelectableHoverHighlightView);
 };
 
-// The view that contains IME short name and the IME label.
-class ImeInfoView : public views::View {
- public:
-  ImeInfoView(const base::string16& id, const base::string16& text) {
-    views::BoxLayout* box_layout =
-        new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, 0);
-    SetLayoutManager(box_layout);
-
-    // TODO(azurewei): Use TrayPopupItemStyle for |id_button|.
-    views::LabelButton* id_button = new views::LabelButton(nullptr, id);
-    ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
-    id_button->SetFontList(rb.GetFontList(ui::ResourceBundle::MediumBoldFont));
-    id_button->SetTextColor(views::Button::STATE_NORMAL, kMenuIconColor);
-    id_button->SetMaxSize(gfx::Size(kMenuButtonSize, kMenuButtonSize));
-    id_button->SetMinSize(gfx::Size(kMenuButtonSize, kMenuButtonSize));
-    const int button_padding = (kMenuButtonSize - kMenuIconSize) / 2;
-    id_button->SetBorder(views::Border::CreateEmptyBorder(
-        button_padding, button_padding, button_padding, button_padding));
-    AddChildView(id_button);
-
-    views::Label* text_label = new views::Label(text);
-    TrayPopupItemStyle style(
-        GetNativeTheme(), TrayPopupItemStyle::FontStyle::DETAILED_VIEW_LABEL);
-    style.SetupLabel(text_label);
-    text_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-    box_layout->set_cross_axis_alignment(
-        views::BoxLayout::CROSS_AXIS_ALIGNMENT_CENTER);
-    AddChildView(text_label);
-    box_layout->SetFlexForView(text_label, 1);
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(ImeInfoView);
-};
-
 // The IME list item view used in the material design. It contains IME info
 // (name and label) and a check button if the item is selected. It's also used
 // for IME property item, which has no name but label and a gray checked icon.
@@ -119,23 +97,42 @@ class ImeListItemView : public ActionableView {
                   bool selected,
                   const SkColor button_color)
       : ActionableView(owner), ime_list_view_(list_view) {
-    views::BoxLayout* box_layout =
-        new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, 0);
-    SetLayoutManager(box_layout);
+    TriView* tri_view = TrayPopupUtils::CreateDefaultRowView();
+    AddChildView(tri_view);
+    SetLayoutManager(new views::FillLayout);
+
+    // The id button shows the IME short name.
+    views::Label* id_label = new views::Label(id);
+    ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+    const gfx::FontList& base_font_list =
+        rb.GetFontList(ui::ResourceBundle::MediumBoldFont);
+    id_label->SetFontList(base_font_list);
+    id_label->SizeToFit(kMenuIconSize);
+    id_label->SetMaximumWidth(kMenuIconSize);
+    int size_deta = -1;

[tdanderson, 2016/11/03 21:17:05]

nit: 'size_delta' instead of 'size_deta'

[Azure Wei, 2016/11/04 01:28:34]

Done.

+    while (id_label->GetPreferredSize().width() > kMenuIconSize) {

[tdanderson, 2016/11/03 21:17:05]

nit: please include a comment to explain what you're doing here, e.g., "Shrink
the font size until it fits within the bounds"

[Azure Wei, 2016/11/04 01:28:34]

Done.

+      id_label->SetFontList(base_font_list.DeriveWithSizeDelta(size_deta));
+      --size_deta;
+      // In case we don't run into endless loop.
+      if (size_deta < kMaxReduceFontSize)

[tdanderson, 2016/11/03 21:17:06]

Instead of this break, can you instead just put it in the while, i.e.,

while (width > kMenuIconSize && size_delta >= kMaxReduceFontSize) {
  --size_delta;
}

?

[Azure Wei, 2016/11/04 01:28:34]

Done.

+        break;
+    }
+    tri_view->AddView(TriView::Container::START, id_label);
 
-    ImeInfoView* info_view = new ImeInfoView(id, label);
-    AddChildView(info_view);
-    box_layout->SetFlexForView(info_view, 1);
+    // The label shows the IME name.
+    views::Label* text_label = new views::Label(label);
+    TrayPopupItemStyle style(
+        GetNativeTheme(), TrayPopupItemStyle::FontStyle::DETAILED_VIEW_LABEL);
+    style.SetupLabel(text_label);
+    text_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+    tri_view->AddView(TriView::Container::CENTER, text_label);
 
     if (selected) {
-      views::ImageButton* check_button = new views::ImageButton(nullptr);
-      gfx::ImageSkia icon_image = gfx::CreateVectorIcon(
-          gfx::VectorIconId::CHECK_CIRCLE, kMenuIconSize, button_color);
-      check_button->SetImage(views::CustomButton::STATE_NORMAL, &icon_image);
-      const int button_padding = (kMenuButtonSize - icon_image.width()) / 2;
-      check_button->SetBorder(views::Border::CreateEmptyBorder(
-          button_padding, button_padding, button_padding, button_padding));
-      AddChildView(check_button);
+      // The checked button indicates the IME is selected.
+      views::ImageView* checked_image = TrayPopupUtils::CreateMainImageView();
+      checked_image->SetImage(gfx::CreateVectorIcon(
+          gfx::VectorIconId::CHECK_CIRCLE, kMenuIconSize, button_color));
+      tri_view->AddView(TriView::Container::END, checked_image);
     }
   }
 
@@ -154,6 +151,85 @@ class ImeListItemView : public ActionableView {
 
 }  // namespace
 
+// The view that contains a |KeyboardButtonView| and a toggle button.
+class ImeListView::MaterialKeyboardStatusRowView : public views::View {
+ public:
+  MaterialKeyboardStatusRowView(views::ButtonListener* listener, bool enabled)
+      : views::View(), listener_(listener), toggle_(nullptr) {

[tdanderson, 2016/11/03 21:17:06]

I don't know if you need the call up to views::View().

[Azure Wei, 2016/11/04 01:28:34]

Should be unnecessary. Removed.

+    Init();
+    SetKeyboardStatusEnabled(enabled);
+  }
+
+  ~MaterialKeyboardStatusRowView() override {}
+
+  void SetKeyboardStatusEnabled(bool enabled) {
+    toggle_->SetIsOn(enabled, true);
+    SetKeyboardToggleText();
+  }
+
+  void SetKeyboardToggleText() {
+    toggle_->SetTooltipText(
+        toggle_->is_on()
+            ? l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_DISABLE_KEYBOARD)

[tdanderson, 2016/11/03 21:17:06]

This is fine to land, but FYI we will eventually want to change this to
"on-screen keyboard, on switch" and "on-screen keyboard, off switch" (see
discussion about toggle button a11y text in crbug.com/652677).

[Azure Wei, 2016/11/04 01:28:34]

The 'on/off switch' will be automatically appended to ToggleButton items, right?
I've added a TODO here.

[tdanderson, 2016/11/04 19:01:04]

Yes, that's right. The change actually just landed today
(https://codereview.chromium.org/2472303002). Thanks for adding the TODO.

+            : l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_ENABLE_KEYBOARD));
+  }
+
+  const views::Button* toggle() const { return toggle_; }
+  bool is_toggled() const { return toggle_->is_on(); }
+
+ protected:
+  // views::View:
+  gfx::Size GetPreferredSize() const override {
+    gfx::Size size = views::View::GetPreferredSize();
+    size.set_height(kMenuButtonSize + kKeyboardRowkVerticalInset * 2);
+    return size;
+  }
+
+  int GetHeightForWidth(int w) const override {
+    return GetPreferredSize().height();
+  }
+
+ private:
+  void Init() {
+    SetBorder(views::Border::CreateSolidSidedBorder(
+        kKeyboardRowSeparatorThickness, 0, 0, 0, kKeyboardRowSeparatorColor));
+    TriView* tri_view = TrayPopupUtils::CreateDefaultRowView();
+    AddChildView(tri_view);
+    SetLayoutManager(new views::FillLayout);
+
+    // The on-screen keyboard image button.
+    views::ImageView* keyboard_image = TrayPopupUtils::CreateMainImageView();
+    keyboard_image->SetImage(gfx::CreateVectorIcon(
+        kImeMenuOnScreenKeyboardIcon, kMenuIconSize, kMenuIconColor));
+    tri_view->AddView(TriView::Container::START, keyboard_image);
+
+    // The on-screen keyboard label.
+    views::Label* label = new views::Label(
+        ui::ResourceBundle::GetSharedInstance().GetLocalizedString(
+            IDS_ASH_STATUS_TRAY_ACCESSIBILITY_VIRTUAL_KEYBOARD));
+    TrayPopupItemStyle style(

[tdanderson, 2016/11/03 21:17:05]

Sorry, I should have mentioned something in a previous review cycle. Take a look
at the class-level documentation of TrayPoupupItemStyle. The correct way to use
it is to make |label| a member variable, then move lines 210-212 into a private
helper function UpdateStyle(). Then have this class override
OnNativeThemeChanged(), which should call UpdateStyle().

Same comment for lines 124-126.

[Azure Wei, 2016/11/04 01:28:34]

Updated this and lines 124-126 by using UpdateStyle().

+        GetNativeTheme(), TrayPopupItemStyle::FontStyle::DETAILED_VIEW_LABEL);
+    style.SetupLabel(label);
+    label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+    tri_view->AddView(TriView::Container::CENTER, label);
+
+    // The on-screen keyboard toggle button.
+    toggle_ = new views::ToggleButton(listener_);
+    toggle_->SetFocusForPlatform();
+    toggle_->SetTooltipText(

[tdanderson, 2016/11/03 21:17:05]

I think you want to call into SetKeyboardToggleText() here instead.

[Azure Wei, 2016/11/04 01:28:34]

Done.

+        l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_DISABLE_KEYBOARD));
+    tri_view->AddView(TriView::Container::END, toggle_);
+  }
+
+  // ButtonListener to notify when |toggle_| is clicked.
+  views::ButtonListener* listener_;
+
+  // ToggleButton to toggle keyboard on or off.
+  views::ToggleButton* toggle_;
+
+  DISALLOW_COPY_AND_ASSIGN(MaterialKeyboardStatusRowView);
+};
+
 ImeListView::ImeListView(SystemTrayItem* owner,
                          bool show_keyboard_toggle,
                          SingleImeBehavior single_ime_behavior)
@@ -166,13 +242,21 @@ ImeListView::ImeListView(SystemTrayItem* owner,
   Update(list, property_list, show_keyboard_toggle, single_ime_behavior);
 }
 
-ImeListView::~ImeListView() {}
+ImeListView::~ImeListView() {
+  material_keyboard_statuts_view_ = nullptr;
+  keyboard_status_ = nullptr;

[tdanderson, 2016/11/03 21:17:06]

Can you clarify why you are setting these to null in the destructor?

[Azure Wei, 2016/11/04 01:28:34]

Nulling the two items are only needed in ResetImeListView(). Reverted these
unnecessary ops.

+}
 
 void ImeListView::Update(const IMEInfoList& list,
                          const IMEPropertyInfoList& property_list,
                          bool show_keyboard_toggle,
                          SingleImeBehavior single_ime_behavior) {
-  Reset();
+  ResetImeListView();
+  if (show_keyboard_toggle &&
+      MaterialDesignController::IsSystemTrayMenuMaterial()) {
+    AppendMaterialKeyboardStatus();
+  }
+
   ime_map_.clear();
   property_map_.clear();
   CreateScrollableList();
@@ -188,7 +272,8 @@ void ImeListView::Update(const IMEInfoList& list,
     }
   }
 
-  if (show_keyboard_toggle) {
+  if (show_keyboard_toggle &&
+      !MaterialDesignController::IsSystemTrayMenuMaterial()) {
     if (list.size() > 1 || !property_list.empty())
       AddScrollSeparator();
     AppendKeyboardStatus();

[tdanderson, 2016/11/03 21:17:05]

Can you add a DCHECK at the top of AppendKeyboardStatus() to check
!IsSystemTrayMenuMaterial()?

[Azure Wei, 2016/11/04 01:28:34]

Done.
The previous change should be bug. Shouldn't add check for MD before adding
scroll separator.

@@ -198,6 +283,12 @@ void ImeListView::Update(const IMEInfoList& list,
   SchedulePaint();
 }
 
+void ImeListView::ResetImeListView() {
+  Reset();
+  material_keyboard_statuts_view_ = nullptr;

[tdanderson, 2016/11/03 21:17:05]

optional nit: When I first saw this it seemed as though you were nulling out
these two members but not actually freeing the memory anywhere. But once I
followed Reset() I see that it calls RemoveAllChildViews(true) which will free
the memory. Consider adding a comment here saying something like "Note that the
children are removed from the view hierarchy and deleted in Reset()."

[Azure Wei, 2016/11/04 01:28:33]

Done.

+  keyboard_status_ = nullptr;
+}
+
 void ImeListView::AppendIMEList(const IMEInfoList& list) {
   DCHECK(ime_map_.empty());
   for (size_t i = 0; i < list.size(); i++) {
@@ -265,6 +356,16 @@ void ImeListView::AppendKeyboardStatus() {
   keyboard_status_ = container;
 }
 
+void ImeListView::AppendMaterialKeyboardStatus() {

[tdanderson, 2016/11/03 21:17:05]

Can you add a DCHECK at the top of this function to check that
IsSystemTrayMenuMaterial() ?

[Azure Wei, 2016/11/04 01:28:34]

Done.

+  if (material_keyboard_statuts_view_)
+    return;
+  MaterialKeyboardStatusRowView* view =
+      new MaterialKeyboardStatusRowView(this, keyboard::IsKeyboardEnabled());
+  view->SetKeyboardStatusEnabled(keyboard::IsKeyboardEnabled());
+  AddChildView(view);
+  material_keyboard_statuts_view_ = view;
+}
+
 void ImeListView::HandleViewClicked(views::View* view) {
   if (view == keyboard_status_) {
     WmShell::Get()->ToggleIgnoreExternalKeyboard();
@@ -289,4 +390,13 @@ void ImeListView::HandleViewClicked(views::View* view) {
   GetWidget()->Close();
 }
 
+void ImeListView::ButtonPressed(views::Button* sender, const ui::Event& event) {
+  if (material_keyboard_statuts_view_ &&
+      sender == material_keyboard_statuts_view_->toggle()) {
+    WmShell::Get()->ToggleIgnoreExternalKeyboard();
+    // Set the toggle's a11y text.
+    material_keyboard_statuts_view_->SetKeyboardToggleText();
+  }
+}
+
 }  // namespace ash