Request focus on IME menu rows after keyboard selection

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"
@@ skipping 122 matching lines @@
           gfx::VectorIconId::CHECK_CIRCLE, kMenuIconSize, button_color));
       tri_view->AddView(TriView::Container::END, checked_image);
     }
     SetAccessibleName(label_->text());
   }
 
   ~ImeListItemView() override {}
 
   // 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;
   }
 
   void OnFocus() override {
     ActionableView::OnFocus();
     if (ime_list_view_ && ime_list_view_->scroll_content())
       ime_list_view_->scroll_content()->ScrollRectToVisible(bounds());
   }
 
   // views::View:
@@ skipping 92 matching lines @@
 
   // 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) {
+    : TrayDetailsView(owner), last_pressed_item_id_("") {
   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() {}
 
@@ skipping 22 matching lines @@
       PrependMaterialKeyboardStatus();
     } else {
       if (list.size() > 1 || !property_list.empty())
         AddScrollSeparator();
       AppendKeyboardStatus();
     }
   }
 
   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() {
   // Children are removed from the view hierarchy and deleted in Reset().
   Reset();
   material_keyboard_status_view_ = nullptr;
   keyboard_status_ = nullptr;
 }
 
 void ImeListView::AppendIMEList(const IMEInfoList& list) {
@@ skipping 70 matching lines @@
 
 void ImeListView::PrependMaterialKeyboardStatus() {
   DCHECK(MaterialDesignController::IsSystemTrayMenuMaterial());
   DCHECK(!material_keyboard_status_view_);
   MaterialKeyboardStatusRowView* view =
       new MaterialKeyboardStatusRowView(this, keyboard::IsKeyboardEnabled());
   scroll_content()->AddChildViewAt(view, 0);
   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;
   }
 
   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;
+    last_pressed_item_id_ = ime_id;
     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;
+    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.

 }
 
 void ImeListView::HandleButtonPressed(views::Button* sender,
                                       const ui::Event& event) {
   if (material_keyboard_status_view_ &&
       sender == material_keyboard_status_view_->toggle()) {
     WmShell::Get()->ToggleIgnoreExternalKeyboard();
   }
 }
 
 }  // namespace ash