[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..54612bd745884d12a476d06b8071dd274b33dd23 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/image_button.h"
 #include "ui/views/controls/button/label_button.h"
+#include "ui/views/controls/button/toggle_button.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;
+const int kKeyboardRowSeparatorThickness = 1;
+const int kFocusBorderInset = 1;
+
+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,15 +85,23 @@ 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 {
+// 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.
+class ImeListItemView : public ActionableView {
  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);
+  ImeListItemView(SystemTrayItem* owner,
+                  ImeListView* list_view,
+                  const base::string16& id,
+                  const base::string16& label,
+                  bool selected,
+                  const SkColor button_color)
+      : ActionableView(owner), ime_list_view_(list_view) {
+    TriView* tri_view = TrayPopupUtils::CreateDefaultRowView();

[tdanderson, 2016/11/02 18:44:36]

Thanks for making use of TriView here, too.

+    AddChildView(tri_view);
+    SetLayoutManager(new views::FillLayout);
 
-    // TODO(azurewei): Use TrayPopupItemStyle for |id_button|.
+    // The id button shows the IME short name.
     views::LabelButton* id_button = new views::LabelButton(nullptr, id);

[tdanderson, 2016/11/02 18:44:36]

optional as follow on work in another CL: consider using just a label instead of
a LabelButton. The entire row is one click target, so I don't think it needs to
be a button.

Similarly, |check_button| can probably just be replaced by an image.

[Azure Wei, 2016/11/03 02:06:44]

Done. Updated |id_button | as a Label and |check_button| as a ImageView.

     ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
     id_button->SetFontList(rb.GetFontList(ui::ResourceBundle::MediumBoldFont));
@@ -90,44 +111,18 @@ class ImeInfoView : public views::View {
     const int button_padding = (kMenuButtonSize - kMenuIconSize) / 2;
     id_button->SetBorder(views::Border::CreateEmptyBorder(
         button_padding, button_padding, button_padding, button_padding));
-    AddChildView(id_button);
+    tri_view->AddView(TriView::Container::START, id_button);
 
-    views::Label* text_label = new views::Label(text);
+    // 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);
-    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.
-class ImeListItemView : public ActionableView {
- public:
-  ImeListItemView(SystemTrayItem* owner,
-                  ImeListView* list_view,
-                  const base::string16& id,
-                  const base::string16& label,
-                  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);
-
-    ImeInfoView* info_view = new ImeInfoView(id, label);
-    AddChildView(info_view);
-    box_layout->SetFlexForView(info_view, 1);
+    tri_view->AddView(TriView::Container::CENTER, text_label);
 
     if (selected) {
+      // The checked button indicates the IME is selected.
       views::ImageButton* check_button = new views::ImageButton(nullptr);
       gfx::ImageSkia icon_image = gfx::CreateVectorIcon(
           gfx::VectorIconId::CHECK_CIRCLE, kMenuIconSize, button_color);
@@ -135,7 +130,7 @@ class ImeListItemView : public ActionableView {
       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);
+      tri_view->AddView(TriView::Container::END, check_button);
     }
   }
 
@@ -154,6 +149,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) {
+    Init();
+    SetKeyboardStatusEnabled(enabled);
+  }
+
+  ~MaterialKeyboardStatusRowView() override {}
+
+  void SetKeyboardStatusEnabled(bool enabled) {
+    toggle_->SetIsOn(enabled, true);
+  }
+
+  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.
+    ui::NativeTheme* theme = GetNativeTheme();
+    views::ImageButton* image_button = new views::ImageButton(nullptr);
+    gfx::ImageSkia image = CreateVectorIcon(kImeMenuOnScreenKeyboardIcon,
+                                            kMenuIconSize, kMenuIconColor);
+    image_button->SetImage(views::CustomButton::STATE_NORMAL, &image);
+    const int horizontal_padding = (kMenuButtonSize - image.width()) / 2;
+    const int vertical_padding = (kMenuButtonSize - image.height()) / 2;
+    image_button->SetBorder(views::Border::CreateEmptyBorder(
+        gfx::Insets(vertical_padding, horizontal_padding)));

[tdanderson, 2016/11/02 18:44:36]

For lines 192-198, I don't think this needs to be an ImageButton since it is not
clickable. I also think you should be able to re-use some of the layout code in
TrayPopupUtils instead of doing the computations yourself. Can you try the
following (similar to what is done in TrayItemMore) and see if it works / looks
correct?

ImageView* image = TrayPopupUtils::CreateMainImageView();
image->SetImage(CreateVectorIcon(...));
tri_view->AddView(TriView::Container::START, image);

[Azure Wei, 2016/11/03 02:06:44]

Done. Updated as using CreateMainImageView() and it works well.

+    const SkColor focus_color =
+        theme->GetSystemColor(ui::NativeTheme::kColorId_FocusedBorderColor);
+    image_button->SetFocusForPlatform();

[tdanderson, 2016/11/02 18:44:36]

The image button shouldn't need to have system focus. The only thing that should
have focus is the toggle button, which you have done on line 216.

[Azure Wei, 2016/11/03 02:06:44]

Done. Removed this line.

+    image_button->SetFocusPainter(views::Painter::CreateSolidFocusPainter(
+        focus_color, gfx::Insets(kFocusBorderInset)));
+    tri_view->AddView(TriView::Container::START, image_button);
+
+    // The on-screen keyboard label.
+    views::Label* label = new views::Label(

[tdanderson, 2016/11/02 18:44:36]

I think you will also need to use the TrayPopupItemStyle and a call to
style.SetupLabel(label) similar to what is done on line 118 in order for this
label to have the correct size and font style. In the mocks it looks like it
should have the same style as the labels used in the individual rows.

[Azure Wei, 2016/11/03 02:06:44]

Done. Used TrayPopupItemStyle to update the label's style.

+        ui::ResourceBundle::GetSharedInstance().GetLocalizedString(
+            IDS_ASH_STATUS_TRAY_ACCESSIBILITY_VIRTUAL_KEYBOARD));
+    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(l10n_util::GetStringUTF16(
+        IDS_ASH_STATUS_TRAY_ACCESSIBILITY_VIRTUAL_KEYBOARD));

[tdanderson, 2016/11/02 18:44:36]

This lg for the time being, thanks for adding it. There may need to be a small
tweak to this in the near future though depending on the outcome of the
discussion in issue crbug.com/652677. If so I will either let you know by filing
a separate bug or make the change myself.

[Azure Wei, 2016/11/03 02:06:44]

Sorry, this should be 'Enable/Disable on-screen keyboard'. Updated. 

+    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)
@@ -173,6 +247,11 @@ void ImeListView::Update(const IMEInfoList& list,
                          bool show_keyboard_toggle,
                          SingleImeBehavior single_ime_behavior) {
   Reset();
+  if (show_keyboard_toggle &&
+      MaterialDesignController::IsSystemTrayMenuMaterial()) {

[tdanderson, 2016/11/02 18:44:36]

Should you also include a check that |material_keyboard_status_view_| hasn't
already been created? (i.e., && !material_keyboard_status_view_| ?

[Azure Wei, 2016/11/03 02:06:44]

Done. Added the check for |material_keyboard_status_view_| in
AppendMaterialKeyboardStatus().

+    AppendMaterialKeyboardStatus();
+  }
+
   ime_map_.clear();
   property_map_.clear();
   CreateScrollableList();
@@ -188,7 +267,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();
@@ -265,6 +345,14 @@ void ImeListView::AppendKeyboardStatus() {
   keyboard_status_ = container;
 }
 
+void ImeListView::AppendMaterialKeyboardStatus() {
+  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 +377,12 @@ 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() &&
+      !material_keyboard_statuts_view_->is_toggled()) {

[tdanderson, 2016/11/02 18:44:36]

Isn't there also a call you need to make when
material_keyboard_status_view_->is_toggled() is true?

[Azure Wei, 2016/11/03 02:06:44]

Removed the check for material_keyboard_status_view_->is_toggled().
Actually the toggled should always be on. Otherwise the toggle won't be shown.

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

I went into non-MD mode on canary, plugged in an external keyboard, and entered
touchview (maximize mode). When I go into the IME detailed view I see a "disable
on-screen keyboard" button, and when I tap it it turns into an "enable on-screen
keyboard" button. So the toggle row needs to be shown regardless of whether the
toggle state is on or off. What you have in patch set 5 on lines ~255 and
ImeListView::ButtonPressed() looks to be doing the correct thing.

+    WmShell::Get()->ToggleIgnoreExternalKeyboard();
+  }
+}
+
 }  // namespace ash