Request focus on IME menu rows after keyboard selection

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 1452eeb447d518398a7e8a9e0cff73c60718c128..5bbc96b5cee873c6daa6a9f7f0075111b2814720 100644
--- a/ash/common/system/chromeos/ime_menu/ime_list_view.cc
+++ b/ash/common/system/chromeos/ime_menu/ime_list_view.cc
@@ -140,7 +140,11 @@ class ImeListItemView : public ActionableView {
 
   // ActionableView:
   bool PerformAction(const ui::Event& event) override {
-    ime_list_view_->HandleViewClicked(this);
+    if (event.type() == ui::EventType::ET_MOUSE_RELEASED) {

[tdanderson, 2016/12/08 00:42:11]

nit: no {} needed around the if and else-if blocks since the boolean expressions
and the blocks themselves are all single line.

[Azure Wei, 2016/12/08 13:14:55]

Done.

+      ime_list_view_->HandleViewClicked(this);
+    } else if (event.type() == ui::EventType::ET_KEY_PRESSED) {
+      ime_list_view_->HandleViewPressed(this);
+    }
     return true;
   }
 
@@ -253,7 +257,7 @@ class MaterialKeyboardStatusRowView : public views::View {
 ImeListView::ImeListView(SystemTrayItem* owner,
                          bool show_keyboard_toggle,
                          SingleImeBehavior single_ime_behavior)
-    : TrayDetailsView(owner) {
+    : TrayDetailsView(owner), last_pressed_item_id_("") {
   SystemTrayDelegate* delegate = WmShell::Get()->system_tray_delegate();
   IMEInfoList list;
   delegate->GetAvailableIMEList(&list);
@@ -296,6 +300,21 @@ void ImeListView::Update(const IMEInfoList& list,
 
   Layout();
   SchedulePaint();
+
+  FocusCurrentImeIfNeeded();
+}
+
+void ImeListView::FocusCurrentImeIfNeeded() {
+  views::FocusManager* manager = GetFocusManager();
+  if (!manager || manager->GetFocusedView() || last_pressed_item_id_.empty()) {

[tdanderson, 2016/12/08 00:42:11]

Can you clarify why you take an early return if manager->GetFocusedView()
exists?

and nit: {} not needed for the same reasons mentioned above.

[Azure Wei, 2016/12/08 13:14:55]

When switching IMEs, all the {@link ImeListItemView}s will be reset and the
{@link ImeListView} will create new {@link ImeListItemView}s. No views should be
focused yet.
Unless users focus to some other views manually (with mouse or 'Tab' key), for
which cases I think we should focus to the new selected IME item view. I think
this case may be impossible to happen, but it's no harm to add a check. 

[tdanderson, 2016/12/08 22:56:37]

Acknowledged.

+    return;
+  }
+
+  for (auto ime_map : ime_map_) {
+    if (ime_map.second == last_pressed_item_id_) {

[tdanderson, 2016/12/08 00:42:11]

nit: {} not needed for the same reasons mentioned above.

[Azure Wei, 2016/12/08 13:14:55]

Done.

+      (ime_map.first)->RequestFocus();
+    }
+  }
 }
 
 void ImeListView::ResetImeListView() {
@@ -386,9 +405,10 @@ void ImeListView::PrependMaterialKeyboardStatus() {
   material_keyboard_status_view_ = view;
 }
 
-void ImeListView::HandleViewClicked(views::View* view) {
+void ImeListView::HandleViewPressed(views::View* view) {
   if (view == keyboard_status_) {

[tdanderson, 2016/12/08 00:42:11]

From the documentation in the header apparently |keyboard_status_| is not being
used in material design. Wouldn't you need to make a change similar to line 411
in ImeListView::HandleButtonPressed() to also fix the bug in MD?

(I assume that lpalmaro is checking and giving approval only for the MD version
of this feature in M-56, is that correct?)

[Azure Wei, 2016/12/08 13:14:55]

Done.

     WmShell::Get()->ToggleIgnoreExternalKeyboard();
+    last_pressed_item_id_ = "";

[tdanderson, 2016/12/08 00:42:11]

Consider using base::Optional rather than the empty string to signal the "not
set" state of this member.

[Azure Wei, 2016/12/08 13:14:55]

Updated as last_selected_item_id_.clear().
Also, added a new bool ImeListView::last_item_selected_with_keyboard_ to deal
with cases like first time to create ImeListView or switching with shortcuts.

     return;
   }
 
@@ -397,6 +417,7 @@ void ImeListView::HandleViewClicked(views::View* view) {
   if (ime != ime_map_.end()) {
     WmShell::Get()->RecordUserMetricsAction(UMA_STATUS_AREA_IME_SWITCH_MODE);
     std::string ime_id = ime->second;
+    last_pressed_item_id_ = ime_id;
     delegate->SwitchIME(ime_id);
   } else {
     std::map<views::View*, std::string>::const_iterator property =
@@ -404,9 +425,14 @@ void ImeListView::HandleViewClicked(views::View* view) {
     if (property == property_map_.end())
       return;
     const std::string key = property->second;
+    last_pressed_item_id_ = key;
     delegate->ActivateIMEProperty(key);
   }
+}
 
+void ImeListView::HandleViewClicked(views::View* view) {
+  HandleViewPressed(view);
+  last_pressed_item_id_ = "";
   GetWidget()->Close();

[tdanderson, 2016/12/08 00:42:11]

So it looks as though when selecting with keyboard you're not closing the opt-in
menu, is that correct? If so, are you sure that the green checkmarks are being
updated correctly (previously-selected IME checkmark should disappear, and the
newly-selected IME checkmark should appear).

If that works as intended, in a follow up CL (not targeted for m-56) I would be
interested in keeping the opt-in IME menu open in cases where the IME switch
happens with mouse or touch. IMO that should be the consistent behavior among
all tray menus and in all detailed views in the system menu.

[Azure Wei, 2016/12/08 13:14:55]

Yes, the green checkmarks work correctly. 
We are not closing the opt-in menu, but it will be reset and create new items
view since get IME refresh event to call ImeListView::Update(). In Update(), all
items are new created based on the current IME list and focus based on the last
operation.

Let me ask lpalmaro@ whether we should change the behavior of mouse or touch. As
for myself, it seems redundant operation if I selected IME and then need to
close the menu.

[tdanderson, 2016/12/08 22:56:37]

Acknowledged.
Actually don't worry about checking this, if we do it for IME then I think we
should do it for everything in the system menu, and that's a much larger change
that would need to have more thought put into it. I'll keep that on my radar for
possible future work.

 }