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

chrome/browser/ui/views/find_bar_view.cc

 // Copyright (c) 2012 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 "chrome/browser/ui/views/find_bar_view.h"
 
 #include <algorithm>
 
 #include "base/macros.h"
 #include "base/strings/string_number_conversions.h"
@@ skipping 134 matching lines @@
 
     find_previous_button_ = find_previous;
     find_next_button_ = find_next;
     close_button_ = close;
   } else {
     find_previous_button_ = new views::ImageButton(this);
     find_next_button_ = new views::ImageButton(this);
     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);
   find_previous_button_->SetTooltipText(
       l10n_util::GetStringUTF16(IDS_FIND_IN_PAGE_PREVIOUS_TOOLTIP));
   find_previous_button_->SetAccessibleName(
       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);
   find_next_button_->SetTooltipText(
       l10n_util::GetStringUTF16(IDS_FIND_IN_PAGE_NEXT_TOOLTIP));
   find_next_button_->SetAccessibleName(
       l10n_util::GetStringUTF16(IDS_ACCNAME_NEXT));
   AddChildView(find_next_button_);
 
   close_button_->set_tag(CLOSE_TAG);
@@ skipping 251 matching lines @@
   switch (sender->tag()) {
     case FIND_PREVIOUS_TAG:
     case FIND_NEXT_TAG:
       if (!find_text_->text().empty()) {
         FindTabHelper* find_tab_helper = FindTabHelper::FromWebContents(
             find_bar_host()->GetFindBarController()->web_contents());
         find_tab_helper->StartFinding(find_text_->text(),
                                       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(
           FindBarController::kKeepSelectionOnPage,
           FindBarController::kKeepResultsInFindBox);
       break;
     default:
       NOTREACHED() << L"Unknown button";
       break;
   }
@@ skipping 147 matching lines @@
     // sent for a lot more things than just the user nulling out the search
     // terms. See http://crbug.com/45372.
     Profile* profile =
         Profile::FromBrowserContext(web_contents->GetBrowserContext());
     FindBarState* find_bar_state = FindBarStateFactory::GetForProfile(profile);
     find_bar_state->set_last_prepopulate_text(base::string16());
   }
 }
 
 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);
 
   if (ui::MaterialDesignController::IsModeMaterial())
     return;
 
   if (no_match) {
     match_count_text_->SetBackgroundColor(kBackgroundColorNoMatch);
     match_count_text_->SetEnabledColor(kTextColorNoMatch);
   } else {
@@ skipping 42 matching lines @@
   SkColor text_color =
       theme->GetSystemColor(ui::NativeTheme::kColorId_TextfieldDefaultColor);
   match_count_text_->SetEnabledColor(SkColorSetA(text_color, 0x69));
   separator_->SetColor(SkColorSetA(text_color, 0x26));
 }
 
 SkColor FindBarView::GetTextColorForIcon() {
   return GetNativeTheme()->GetSystemColor(
       ui::NativeTheme::kColorId_TextfieldDefaultColor);
 }