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

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

 // Copyright 2016 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/chromeos/ime_menu/ime_list_view.h"
 
 #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"
 #include "ui/accessibility/ax_view_state.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/paint_vector_icon.h"
 #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 =
       new views::Separator(views::Separator::HORIZONTAL);
   separator->SetColor(kBorderLightColor);
   separator->SetPreferredSize(kSeparatorWidth);
   separator->SetBorder(views::Border::CreateEmptyBorder(
       kMenuSeparatorVerticalPadding, 0, kMenuSeparatorVerticalPadding, 0));
   return separator;
 }
@@ skipping 20 matching lines @@
     if (selected_)
       state->AddStateFlag(ui::AX_STATE_CHECKED);
   }
 
  private:
   bool selected_;
 
   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));
     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);
+    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);
       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);
+      tri_view->AddView(TriView::Container::END, check_button);
     }
   }
 
   ~ImeListItemView() override {}
 
   // ActionableView:
   bool PerformAction(const ui::Event& event) override {
     ime_list_view_->HandleViewClicked(this);
     return true;
   }
 
  private:
   ImeListView* ime_list_view_;
   DISALLOW_COPY_AND_ASSIGN(ImeListItemView);
 };
 
 }  // 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)
     : TrayDetailsView(owner) {
   SystemTrayDelegate* delegate = WmShell::Get()->system_tray_delegate();
   IMEInfoList list;
   delegate->GetAvailableIMEList(&list);
   IMEPropertyInfoList property_list;
   delegate->GetCurrentIMEProperties(&property_list);
   Update(list, property_list, show_keyboard_toggle, single_ime_behavior);
 }
 
 ImeListView::~ImeListView() {}
 
 void ImeListView::Update(const IMEInfoList& list,
                          const IMEPropertyInfoList& property_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();
 
   // Appends IME list and IME properties.
   if (single_ime_behavior == ImeListView::SHOW_SINGLE_IME || list.size() > 1) {
     if (MaterialDesignController::IsSystemTrayMenuMaterial()) {
       AppendImeListAndProperties(list, property_list);
     } else {
       AppendIMEList(list);
       if (!property_list.empty())
         AppendIMEProperties(property_list);
     }
   }
 
-  if (show_keyboard_toggle) {
+  if (show_keyboard_toggle &&
+      !MaterialDesignController::IsSystemTrayMenuMaterial()) {
     if (list.size() > 1 || !property_list.empty())
       AddScrollSeparator();
     AppendKeyboardStatus();
   }
 
   Layout();
   SchedulePaint();
 }
 
 void ImeListView::AppendIMEList(const IMEInfoList& list) {
@@ skipping 56 matching lines @@
   HoverHighlightView* container = new HoverHighlightView(this);
   int id = keyboard::IsKeyboardEnabled() ? IDS_ASH_STATUS_TRAY_DISABLE_KEYBOARD
                                          : IDS_ASH_STATUS_TRAY_ENABLE_KEYBOARD;
   container->AddLabel(
       ui::ResourceBundle::GetSharedInstance().GetLocalizedString(id),
       gfx::ALIGN_LEFT, false /* highlight */);
   scroll_content()->AddChildView(container);
   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();
     return;
   }
 
   SystemTrayDelegate* delegate = WmShell::Get()->system_tray_delegate();
   std::map<views::View*, std::string>::const_iterator ime = ime_map_.find(view);
   if (ime != ime_map_.end()) {
     WmShell::Get()->RecordUserMetricsAction(UMA_STATUS_AREA_IME_SWITCH_MODE);
     std::string ime_id = ime->second;
     delegate->SwitchIME(ime_id);
   } else {
     std::map<views::View*, std::string>::const_iterator property =
         property_map_.find(view);
     if (property == property_map_.end())
       return;
     const std::string key = property->second;
     delegate->ActivateIMEProperty(key);
   }
 
   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