Scroll to make the selected IME visible.

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 cf2b34a25d2d1ad022714ecc4ad2d684573f38bc..32c0331c6f1886019956d4495709eac925692364 100644
--- a/ash/common/system/chromeos/ime_menu/ime_list_view.cc
+++ b/ash/common/system/chromeos/ime_menu/ime_list_view.cc
@@ -262,7 +262,8 @@ ImeListView::ImeListView(SystemTrayItem* owner,
                          SingleImeBehavior single_ime_behavior)
     : TrayDetailsView(owner),
       last_item_selected_with_keyboard_(false),
-      should_focus_ime_after_selection_with_keyboard_(false) {
+      should_focus_ime_after_selection_with_keyboard_(false),
+      current_ime_view_(nullptr) {
   SystemTrayDelegate* delegate = WmShell::Get()->system_tray_delegate();
   IMEInfoList list;
   delegate->GetAvailableIMEList(&list);
@@ -281,6 +282,7 @@ void ImeListView::Update(const IMEInfoList& list,
   ime_map_.clear();
   property_map_.clear();
   CreateScrollableList();
+  current_ime_view_ = nullptr;

[tdanderson, 2017/01/12 00:16:14]

Should this be placed in ResetImeListView() instead?

[Azure Wei, 2017/01/12 01:20:54]

Moved to ResetImeListView(). Thanks.

 
   // Appends IME list and IME properties.
   if (single_ime_behavior == ImeListView::SHOW_SINGLE_IME || list.size() > 1) {
@@ -309,6 +311,8 @@ void ImeListView::Update(const IMEInfoList& list,
   if (should_focus_ime_after_selection_with_keyboard_ &&
       last_item_selected_with_keyboard_)
     FocusCurrentImeIfNeeded();
+  else if (current_ime_view_ != nullptr)

[tdanderson, 2017/01/12 00:16:15]

I don't think I understand why this is an 'else if' instead of an 'if'. Don't
you want to execute line 315 even if
(should_focus_ime_after_selection_with_keyboard_ &&
last_item_selected_with_keyboard_) is true?

Similarly, on line 458 I don't think I understand the need to early return if
the middle clause is true (should_focus_ime_after_selection_with_keyboard_ &&
last_item_selected_with_keyboard_). 

Can you please explain?

also, nit: just "if (current_ime_view_)" should be sufficient here

[Azure Wei, 2017/01/12 01:20:54]

If (should_focus_ime_after_selection_with_keyboard_ &&
last_item_selected_with_keyboard_), we need to focus to last item selected with
keyboard, which could be IME property. Some IME have more than 5 properties
(e.g. Japanese Input), thus the current IME and the selected property cannot be
shown at the same time.  

[tdanderson, 2017/01/12 21:27:33]

Acknowledged.

+    scroll_content()->ScrollRectToVisible(current_ime_view_->bounds());

[tdanderson, 2017/01/12 00:16:14]

please use {} after the if and else-if clauses since the 'if' boolean expression
spans more than one line

[Azure Wei, 2017/01/12 01:20:54]

Done.

 }
 
 void ImeListView::ResetImeListView() {
@@ -320,6 +324,7 @@ void ImeListView::ResetImeListView() {
 
 void ImeListView::CloseImeListView() {
   last_selected_item_id_.clear();
+  current_ime_view_ = nullptr;
   last_item_selected_with_keyboard_ = false;
   GetWidget()->Close();
 }
@@ -359,6 +364,9 @@ void ImeListView::AppendImeListAndProperties(
     scroll_content()->AddChildView(ime_view);
     ime_map_[ime_view] = list[i].id;
 
+    if (list[i].selected)
+      current_ime_view_ = ime_view;
+
     // In material design, the property items will be added after the current
     // selected IME item.
     if (list[i].selected && !property_list.empty()) {
@@ -446,6 +454,15 @@ void ImeListView::HandleButtonPressed(views::Button* sender,
   }
 }
 
+void ImeListView::VisibilityChanged(View* starting_from, bool is_visible) {
+  if (!is_visible || (should_focus_ime_after_selection_with_keyboard_ &&
+                      last_item_selected_with_keyboard_) ||

[tdanderson, 2017/01/12 00:16:15]

This formatting seems strange, can you run 'git cl format ash' ?

[Azure Wei, 2017/01/12 01:20:54]

This is formatted by 'git cl format'.

+      current_ime_view_ == nullptr)

[tdanderson, 2017/01/12 00:16:14]

just "!current_ime_view_" instead of "== nullptr"

[Azure Wei, 2017/01/12 01:20:54]

Done.

+    return;

[tdanderson, 2017/01/12 00:16:14]

use {} since boolean expression spans more than one line

[Azure Wei, 2017/01/12 01:20:54]

Done.

+
+  scroll_content()->ScrollRectToVisible(current_ime_view_->bounds());

[tdanderson, 2017/01/12 00:16:14]

If I am switching to an IME that has a property list associated with it, based
on this line and line 368 it seems that only the IME's name will be scrolled
into view (and its property list will remain hidden). Is that true? Wouldn't we
want to show the whole thing (IME plus its properties)?

[Azure Wei, 2017/01/12 01:20:54]

It's hard to show the whole thing. For opt-in IME menu, at max 5 items could be
shown for the same time.

[tdanderson, 2017/01/12 21:27:33]

Acknowledged.

+}
+
 void ImeListView::FocusCurrentImeIfNeeded() {
   views::FocusManager* manager = GetFocusManager();
   if (!manager || manager->GetFocusedView() || last_selected_item_id_.empty())