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

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

The focus was incorrectly moved to the textfield after using the
keyboard. This makes it awkward to search for the previous occurrence
using only the keyboard. The focus still moves to the textbox when using
the mouse so that the user is still able to type different search terms.

BUG=537714
R=msw@chromium.org
TEST=Press CTRL+F to open the Find-In-Page box. Enter random text. Press
TAB to focus previous or next button. Press ENTER or SPACE. The button
should still be focused.

Committed: https://crrev.com/f1777a5bee5a24d196deab700bf4c4a5e118a156
Cr-Commit-Position: refs/heads/master@{#374578}

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

Thanks for submitting this CL!

https://codereview.chromium.org/1660273003/diff/1/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/1660273003/diff/1/AUTHORS#newcode379
AUTHORS:379: Mario Pistrich <m.pistrich@gmail.com>
Have you completed the appropriate Contributor License Agreement?
https://www.chromium.org/developers/contributing-code

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
File chrome/browser/ui/views/find_bar_view.cc (right):

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
chrome/browser/ui/views/find_bar_view.cc:156:
find_previous_button_->set_tag(FIND_PREVIOUS_TAG);
nit: remove FIND_PREVIOUS_TAG and FIND_NEXT_TAG, use the new ids instead.

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
chrome/browser/ui/views/find_bar_view.cc:611: bool enable_buttons =
!match_count_text_->text().empty() || !no_match;
This change seems unrelated; why is it part of this CL?

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
File chrome/browser/ui/views/find_bar_views_interactive_uitest.cc (right):

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:133: //
TODO(mpistrich): Enable again when ui_test_utils::ClickOnView works with find
Why doesn't ClickOnView work with these buttons?

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:155:
VIEW_ID_FIND_IN_PAGE_PREVIOUS_BUTTON));
This should be VIEW_ID_FIND_IN_PAGE_TEXT_FIELD

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:161:
VIEW_ID_FIND_IN_PAGE_NEXT_BUTTON));
Ditto: This should be VIEW_ID_FIND_IN_PAGE_TEXT_FIELD

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

I've changed the usage of view IDs, but I am not sure about ClickOnView (see
comment).

https://codereview.chromium.org/1660273003/diff/1/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/1660273003/diff/1/AUTHORS#newcode379
AUTHORS:379: Mario Pistrich <m.pistrich@gmail.com>
On 2016/02/03 19:58:02, msw wrote:
> Have you completed the appropriate Contributor License Agreement?
> https://www.chromium.org/developers/contributing-code

Yes, I've already signed the Individual Contributor License Agreement.

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
File chrome/browser/ui/views/find_bar_view.cc (right):

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
chrome/browser/ui/views/find_bar_view.cc:156:
find_previous_button_->set_tag(FIND_PREVIOUS_TAG);
On 2016/02/03 19:58:02, msw wrote:
> nit: remove FIND_PREVIOUS_TAG and FIND_NEXT_TAG, use the new ids instead.

Done.

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
chrome/browser/ui/views/find_bar_view.cc:611: bool enable_buttons =
!match_count_text_->text().empty() || !no_match;
On 2016/02/03 19:58:02, msw wrote:
> This change seems unrelated; why is it part of this CL?

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.

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
File chrome/browser/ui/views/find_bar_views_interactive_uitest.cc (right):

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:133: //
TODO(mpistrich): Enable again when ui_test_utils::ClickOnView works with find
On 2016/02/03 19:58:02, msw wrote:
> Why doesn't ClickOnView work with these buttons?

I couldn't figure it out why ClickOnView does not work with these buttons. It
doesn't work with VIEW_ID_FIND_IN_PAGE_TEXT_FIELD either.
BrowserView::GetViewByID returned a nullptr. It seems like the view of the find
bar is contained in the BrowserView.

Should I try to fix this issue in this CL? Alternatively I can open another
issue and link to this test case.

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:155:
VIEW_ID_FIND_IN_PAGE_PREVIOUS_BUTTON));
On 2016/02/03 19:58:02, msw wrote:
> This should be VIEW_ID_FIND_IN_PAGE_TEXT_FIELD

Done.

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:161:
VIEW_ID_FIND_IN_PAGE_NEXT_BUTTON));
On 2016/02/03 19:58:02, msw wrote:
> Ditto: This should be VIEW_ID_FIND_IN_PAGE_TEXT_FIELD

Done.

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

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
File chrome/browser/ui/views/find_bar_view.cc (right):

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
chrome/browser/ui/views/find_bar_view.cc:611: bool enable_buttons =
!match_count_text_->text().empty() || !no_match;
On 2016/02/03 20:41:59, Mario Pistrich wrote:
> On 2016/02/03 19:58:02, msw wrote:
> > This change seems unrelated; why is it part of this CL?
> 
> 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.

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.

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
File chrome/browser/ui/views/find_bar_views_interactive_uitest.cc (right):

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:133: //
TODO(mpistrich): Enable again when ui_test_utils::ClickOnView works with find
On 2016/02/03 20:41:59, Mario Pistrich wrote:
> On 2016/02/03 19:58:02, msw wrote:
> > Why doesn't ClickOnView work with these buttons?
> 
> I couldn't figure it out why ClickOnView does not work with these buttons. It
> doesn't work with VIEW_ID_FIND_IN_PAGE_TEXT_FIELD either.
> BrowserView::GetViewByID returned a nullptr. It seems like the view of the
find
> bar is contained in the BrowserView.
> 
> Should I try to fix this issue in this CL? Alternatively I can open another
> issue and link to this test case.

Please file a bug and cite in this comment; follow up afterwards.

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

PTAL

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
File chrome/browser/ui/views/find_bar_view.cc (right):

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
chrome/browser/ui/views/find_bar_view.cc:611: bool enable_buttons =
!match_count_text_->text().empty() || !no_match;
On 2016/02/03 21:43:34, msw wrote:
> On 2016/02/03 20:41:59, Mario Pistrich wrote:
> > On 2016/02/03 19:58:02, msw wrote:
> > > This change seems unrelated; why is it part of this CL?
> > 
> > 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.
> 
> 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.

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

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
File chrome/browser/ui/views/find_bar_views_interactive_uitest.cc (right):

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:133: //
TODO(mpistrich): Enable again when ui_test_utils::ClickOnView works with find
On 2016/02/03 21:43:34, msw wrote:
> On 2016/02/03 20:41:59, Mario Pistrich wrote:
> > On 2016/02/03 19:58:02, msw wrote:
> > > Why doesn't ClickOnView work with these buttons?
> > 
> > I couldn't figure it out why ClickOnView does not work with these buttons.
It
> > doesn't work with VIEW_ID_FIND_IN_PAGE_TEXT_FIELD either.
> > BrowserView::GetViewByID returned a nullptr. It seems like the view of the
> find
> > bar is contained in the BrowserView.
> > 
> > Should I try to fix this issue in this CL? Alternatively I can open another
> > issue and link to this test case.
> 
> Please file a bug and cite in this comment; follow up afterwards.

Done, see http://crbug.com/584043

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

lgtm

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
File chrome/browser/ui/views/find_bar_view.cc (right):

https://codereview.chromium.org/1660273003/diff/1/chrome/browser/ui/views/fin...
chrome/browser/ui/views/find_bar_view.cc:611: bool enable_buttons =
!match_count_text_->text().empty() || !no_match;
On 2016/02/03 23:32:54, Mario Pistrich wrote:
> On 2016/02/03 21:43:34, msw wrote:
> > On 2016/02/03 20:41:59, Mario Pistrich wrote:
> > > On 2016/02/03 19:58:02, msw wrote:
> > > > This change seems unrelated; why is it part of this CL?
> > > 
> > > 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.
> > 
> > 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.
> 
> 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

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.

[Mario Pistrich, 2016-02-04 01:21:26]

The CQ bit was checked by m.pistrich@gmail.com

[commit-bot: I haz the power, 2016-02-04 01:23:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1660273003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1660273003/40001

[commit-bot: I haz the power, 2016-02-04 02:40:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-04 02:40:18]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mario Pistrich, 2016-02-04 03:29:13]

Description was changed from

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

The focus was incorrectly moved to the textfield after using the
keyboard. This makes it awkward to search for the previous occurrence
using only the keyboard. The focus still moves to the textbox when using
the mouse so that the user is still able to type different search terms.

BUG=537714
R=msw@chromium.org
TEST=Press CTRL+F to open the Find-In-Page box. Enter random text. Press
TAB to focus previous or next button. Press ENTER or SPACE. The button
should still be focused.
==========

to

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

The focus was incorrectly moved to the textfield after using the
keyboard. This makes it awkward to search for the previous occurrence
using only the keyboard. The focus still moves to the textbox when using
the mouse so that the user is still able to type different search terms.

BUG=537714
R=msw@chromium.org
TEST=Press CTRL+F to open the Find-In-Page box. Enter random text. Press
TAB to focus previous or next button. Press ENTER or SPACE. The button
should still be focused.
==========

[Mario Pistrich, 2016-02-04 03:29:13]

m.pistrich@gmail.com changed reviewers:
+ rohitrao@chromium.org

[Mario Pistrich, 2016-02-04 03:30:23]

On 2016/02/04 02:40:18, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

I've added the view IDs for the cocoa UI, can you please take a look at it,
rohitrao@?

[Mario Pistrich, 2016-02-08 23:24:13]

m.pistrich@gmail.com changed reviewers:
+ tapted@chromium.org

[Mario Pistrich, 2016-02-08 23:27:11]

tapted can you please take a look at the view IDs for the cocoa UI?

[tapted, 2016-02-09 02:41:24]

chrome/browser/ui/cocoa/ lgtm with the redundant comment removed

https://codereview.chromium.org/1660273003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm (right):

https://codereview.chromium.org/1660273003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm:112: // Set
ViewIDs for the buttons which don't have their dedicated class.
This comment isn't right. I think it can just be omitted, since this is located
pretty near to the initializer.

[Mario Pistrich, 2016-02-09 07:24:59]

The CQ bit was checked by m.pistrich@gmail.com

[Mario Pistrich, 2016-02-09 07:24:59]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/1660273003/#ps80001
(title: "Remove redundant comment in cocoa findbar controller.")

[commit-bot: I haz the power, 2016-02-09 07:25:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1660273003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1660273003/80001

[commit-bot: I haz the power, 2016-02-09 07:46:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-09 07:46:47]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Mario Pistrich, 2016-02-09 17:48:01]

The CQ bit was checked by m.pistrich@gmail.com

[Mario Pistrich, 2016-02-09 17:48:01]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/1660273003/#ps100001
(title: "Add missing import of view_id_util.h.")

[commit-bot: I haz the power, 2016-02-09 17:50:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1660273003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1660273003/100001

[commit-bot: I haz the power, 2016-02-09 19:26:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-09 19:26:14]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mario Pistrich, 2016-02-09 20:24:06]

The CQ bit was checked by m.pistrich@gmail.com

[Mario Pistrich, 2016-02-09 20:24:06]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/1660273003/#ps120001
(title: "Set viewID of findbar after it is loaded.")

[commit-bot: I haz the power, 2016-02-09 20:26:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1660273003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1660273003/120001

[commit-bot: I haz the power, 2016-02-09 22:25:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-09 22:25:58]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mario Pistrich, 2016-02-09 23:38:22]

The CQ bit was checked by m.pistrich@gmail.com

[Mario Pistrich, 2016-02-09 23:38:22]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/1660273003/#ps140001
(title: "Unset viewID before browser is set to nullptr.")

[commit-bot: I haz the power, 2016-02-09 23:39:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1660273003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1660273003/140001

[tapted, 2016-02-10 00:56:01]

https://codereview.chromium.org/1660273003/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm (right):

https://codereview.chromium.org/1660273003/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm:103:
view_id_util::UnsetID(closeButton_);
I don't think this will fix the problem.

I think the issue is that by the time we get here, Cocoa has unloaded the view
controller and its owned views, then set the IBOutlets to nil. Cocoa doesn't
have a reliable signal when its views go away since they're reference counted
(The 10.10SDK has -[NSViewController viewWillDisappear] but that's not available
on all the OSes we support and might not be what we need in any case).

My recommendation is to just add these view IDs to the big 'ol `if` condition in
view_id_util_browsetest.mm:


      // Mac implementation does not support following ids yet.
      // TODO(palmer): crbug.com/536257: Enable VIEW_ID_LOCATION_ICON.
      if (i == VIEW_ID_STAR_BUTTON ||
          i == VIEW_ID_CONTENTS_SPLIT ||
          i == VIEW_ID_BROWSER_ACTION ||
          i == VIEW_ID_FEEDBACK_BUTTON ||
          i == VIEW_ID_SCRIPT_BUBBLE ||
          i == VIEW_ID_SAVE_CREDIT_CARD_BUTTON ||
          i == VIEW_ID_TRANSLATE_BUTTON ||
          i == VIEW_ID_LOCATION_ICON) {
        continue;
      }

Since find_bar_views_interactive_uitest.cc won't be testing the Cocoa stuff
anyway, so they're not really needed execpt ot make the ViewIDTest.Basic test
pass as far as I can tell.

[Mario Pistrich, 2016-02-10 01:14:29]

Thanks, I've added the viewIDs to the exception list. PTAL.

https://codereview.chromium.org/1660273003/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm (right):

https://codereview.chromium.org/1660273003/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm:103:
view_id_util::UnsetID(closeButton_);
On 2016/02/10 00:56:01, tapted wrote:
> I don't think this will fix the problem.
> 
> I think the issue is that by the time we get here, Cocoa has unloaded the view
> controller and its owned views, then set the IBOutlets to nil. Cocoa doesn't
> have a reliable signal when its views go away since they're reference counted
> (The 10.10SDK has -[NSViewController viewWillDisappear] but that's not
available
> on all the OSes we support and might not be what we need in any case).
> 
> My recommendation is to just add these view IDs to the big 'ol `if` condition
in
> view_id_util_browsetest.mm:
> 
> 
>       // Mac implementation does not support following ids yet.
>       // TODO(palmer): crbug.com/536257: Enable VIEW_ID_LOCATION_ICON.
>       if (i == VIEW_ID_STAR_BUTTON ||
>           i == VIEW_ID_CONTENTS_SPLIT ||
>           i == VIEW_ID_BROWSER_ACTION ||
>           i == VIEW_ID_FEEDBACK_BUTTON ||
>           i == VIEW_ID_SCRIPT_BUBBLE ||
>           i == VIEW_ID_SAVE_CREDIT_CARD_BUTTON ||
>           i == VIEW_ID_TRANSLATE_BUTTON ||
>           i == VIEW_ID_LOCATION_ICON) {
>         continue;
>       }
> 
> Since find_bar_views_interactive_uitest.cc won't be testing the Cocoa stuff
> anyway, so they're not really needed execpt ot make the ViewIDTest.Basic test
> pass as far as I can tell.

Thanks for clarifying, I've added them to the if so that they are skipped.

[tapted, 2016-02-10 01:33:28]

lgtm if the tests are happy

[Mario Pistrich, 2016-02-10 01:34:01]

The CQ bit was checked by m.pistrich@gmail.com

[Mario Pistrich, 2016-02-10 01:34:02]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1660273003/#ps160001
(title: "Remove viewIDs for findbar and add them to the exception list.")

[commit-bot: I haz the power, 2016-02-10 01:36:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1660273003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1660273003/160001

[commit-bot: I haz the power, 2016-02-10 02:20:29]

Description was changed from

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

The focus was incorrectly moved to the textfield after using the
keyboard. This makes it awkward to search for the previous occurrence
using only the keyboard. The focus still moves to the textbox when using
the mouse so that the user is still able to type different search terms.

BUG=537714
R=msw@chromium.org
TEST=Press CTRL+F to open the Find-In-Page box. Enter random text. Press
TAB to focus previous or next button. Press ENTER or SPACE. The button
should still be focused.
==========

to

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

The focus was incorrectly moved to the textfield after using the
keyboard. This makes it awkward to search for the previous occurrence
using only the keyboard. The focus still moves to the textbox when using
the mouse so that the user is still able to type different search terms.

BUG=537714
R=msw@chromium.org
TEST=Press CTRL+F to open the Find-In-Page box. Enter random text. Press
TAB to focus previous or next button. Press ENTER or SPACE. The button
should still be focused.
==========

[commit-bot: I haz the power, 2016-02-10 02:20:30]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-02-10 02:21:39]

Description was changed from

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

The focus was incorrectly moved to the textfield after using the
keyboard. This makes it awkward to search for the previous occurrence
using only the keyboard. The focus still moves to the textbox when using
the mouse so that the user is still able to type different search terms.

BUG=537714
R=msw@chromium.org
TEST=Press CTRL+F to open the Find-In-Page box. Enter random text. Press
TAB to focus previous or next button. Press ENTER or SPACE. The button
should still be focused.
==========

to

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

The focus was incorrectly moved to the textfield after using the
keyboard. This makes it awkward to search for the previous occurrence
using only the keyboard. The focus still moves to the textbox when using
the mouse so that the user is still able to type different search terms.

BUG=537714
R=msw@chromium.org
TEST=Press CTRL+F to open the Find-In-Page box. Enter random text. Press
TAB to focus previous or next button. Press ENTER or SPACE. The button
should still be focused.

Committed: https://crrev.com/f1777a5bee5a24d196deab700bf4c4a5e118a156
Cr-Commit-Position: refs/heads/master@{#374578}
==========

[commit-bot: I haz the power, 2016-02-10 02:21:41]

Patchset 9 (id:??) landed as
https://crrev.com/f1777a5bee5a24d196deab700bf4c4a5e118a156
Cr-Commit-Position: refs/heads/master@{#374578}