Keep focus on Find-In-Page buttons when using the keyboard to navigate.

chrome/browser/ui/views/find_bar_view.cc

Index: chrome/browser/ui/views/find_bar_view.cc
diff --git a/chrome/browser/ui/views/find_bar_view.cc b/chrome/browser/ui/views/find_bar_view.cc
index dbfa65b73c2a8a65e1b298f11b74a490232cf401..91ed00a92c249c4a702896dbd84b8d22a39b5514 100644
--- a/chrome/browser/ui/views/find_bar_view.cc
+++ b/chrome/browser/ui/views/find_bar_view.cc
@@ -152,6 +152,7 @@ FindBarView::FindBarView(FindBarHost* host)
     close_button_ = new views::ImageButton(this);
   }
 
+  find_previous_button_->set_id(VIEW_ID_FIND_IN_PAGE_PREVIOUS_BUTTON);
   find_previous_button_->set_tag(FIND_PREVIOUS_TAG);

[msw, 2016/02/03 19:58:02]

nit: remove FIND_PREVIOUS_TAG and FIND_NEXT_TAG, use the new ids instead.

[Mario Pistrich, 2016/02/03 20:41:59]

Done.

   find_previous_button_->SetFocusable(true);
   find_previous_button_->set_request_focus_on_press(false);
@@ -161,6 +162,7 @@ FindBarView::FindBarView(FindBarHost* host)
       l10n_util::GetStringUTF16(IDS_ACCNAME_PREVIOUS));
   AddChildView(find_previous_button_);
 
+  find_next_button_->set_id(VIEW_ID_FIND_IN_PAGE_NEXT_BUTTON);
   find_next_button_->set_tag(FIND_NEXT_TAG);
   find_next_button_->SetFocusable(true);
   find_next_button_->set_request_focus_on_press(false);
@@ -432,8 +434,11 @@ void FindBarView::ButtonPressed(
                                       sender->tag() == FIND_NEXT_TAG,
                                       false);  // Not case sensitive.
       }
-      // Move focus to the find textfield.
-      find_text_->RequestFocus();
+
+      if (event.IsMouseEvent()) {
+        // Move focus to the find textfield.
+        find_text_->RequestFocus();
+      }
       break;
     case CLOSE_TAG:
       find_bar_host()->GetFindBarController()->EndFindSession(
@@ -601,7 +606,9 @@ void FindBarView::Find(const base::string16& search_text) {
 }
 
 void FindBarView::UpdateMatchCountAppearance(bool no_match) {
-  bool enable_buttons = !match_count_text_->text().empty() && !no_match;
+  // Enable the buttons if there is a match or if there is a match count text
+  // set (from a prepopulated view).
+  bool enable_buttons = !match_count_text_->text().empty() || !no_match;

[msw, 2016/02/03 19:58:02]

This change seems unrelated; why is it part of this CL?

[Mario Pistrich, 2016/02/03 20:41:59]

I changed this so that the focus is not moved to the close button (due to being
temporarily disabled by FindBarView::ClearMatchCount). The side effect is, that
the buttons are also enabled on a prepopulated find box (e.g. new tab where the
find box was not opened before) which in my opinion is not a bad thing, because
they can be focused right away with TAB.

[msw, 2016/02/03 21:43:34]

Sorry, when/how is focus moved to the close button without this change? (if it's
because ButtonPressed no longer calls |find_text_->RequestFocus()| on keyboard
events, can we instead call |sender->RequestFocus()| there for keyboard events
and revert this change?). It sounds very odd if the buttons are being disabled
when pressed, and this might be a reasonable change, but I don't fully
understand its implications, and it seems tangential to the main defect being
addressed. I'd prefer that the behavior of disabling buttons was changed in a
separate change, if at all.

[Mario Pistrich, 2016/02/03 23:32:54]

I should have described the reason of the change more detailed, sorry about
that.

Using |sender->RequestFocus()| will temporarily change the focus, but shortly
after it is moved to the close button by following call flow:

* After the button was pressed, the UI is updated via
FindBarHost::UpdateUIForFindResult
* Before updating the view, the match count is cleared with
|find_bar_view()->ClearMatchCount()|
* FindBarView::ClearMatchCount clears the match count text and calls
|UpdateMatchCountAppearance(false)|
* |enable_buttons| is false in this case, so the buttons get disabled
* Focus is moved to the close button via View::AdvanceFocusIfNecessary
* Buttons are reenabled, if |!find_text.empty()| with
|find_bar_view()->UpdateForResult(result, find_text);|
* The close button is still focused

[msw, 2016/02/04 00:35:20]

Hmm, it's too bad that the complexity of this design causes the buttons to be
disabled and then re-enabled without a proper reason, and that causes the focus
to shift. This change may not be ideal, but I suppose it's okay; the side effect
of enabling the buttons for new tab searches is probably alright.

   find_previous_button_->SetEnabled(enable_buttons);
   find_next_button_->SetEnabled(enable_buttons);