Allow arrow keys to navigate tile results in the experimental app list.

Allow arrow keys to navigate tile results in the experimental app list.

This CL implements an experimental scheme for navigating the tile results
in the experimental app list. The left and right arrow keys now navigate
the tile list if the search box cursor is at the end of the text.

BUG=436327
Committed: https://crrev.com/60e5f1185e04c0bcb43f82b2e224de2ad463abee
Cr-Commit-Position: refs/heads/master@{#308310}

[calamity, 2014-12-10 08:02:05]

calamity@chromium.org changed reviewers:
+ mgiuca@chromium.org

[Matt Giuca, 2014-12-11 07:55:17]

https://codereview.chromium.org/793633002/diff/1/ui/app_list/views/search_res...
File ui/app_list/views/search_result_tile_item_list_view.cc (right):

https://codereview.chromium.org/793633002/diff/1/ui/app_list/views/search_res...
ui/app_list/views/search_result_tile_item_list_view.cc:80:
search_box_->GetCursorPosition() == search_box_->text().length();
|| search_box_->text().empty();

I guess the text box can never really be empty here, but just for completeness
(say that one day we want to reuse this code for the tiles on the start page),
we should handle this case.

If the text box is empty, arrow keys should be free to navigate the tiles.

https://codereview.chromium.org/793633002/diff/1/ui/app_list/views/search_res...
ui/app_list/views/search_result_tile_item_list_view.cc:90: // the beginning of
the list. This means that the text cursor in the
I think this first sentence should go down below where IsValidSelectionIndex()
is called. This code here does not care where the tile selection is.

Rewrite this comment to say essentially what the VKEY_RIGHT comment says.

https://codereview.chromium.org/793633002/diff/1/ui/app_list/views/search_res...
File ui/app_list/views/search_result_tile_item_list_view.h (right):

https://codereview.chromium.org/793633002/diff/1/ui/app_list/views/search_res...
ui/app_list/views/search_result_tile_item_list_view.h:41: views::Textfield*
search_box_;  // Owned by the views hierarchy.
// The ContentsView's search box. Used to inform the keyboard navigation of
tiles (behaves differently depending on the text cursor position).

[calamity, 2014-12-12 05:23:41]

https://codereview.chromium.org/793633002/diff/1/ui/app_list/views/search_res...
File ui/app_list/views/search_result_tile_item_list_view.cc (right):

https://codereview.chromium.org/793633002/diff/1/ui/app_list/views/search_res...
ui/app_list/views/search_result_tile_item_list_view.cc:80:
search_box_->GetCursorPosition() == search_box_->text().length();
On 2014/12/11 07:55:17, Matt Giuca wrote:
> || search_box_->text().empty();
> 
> I guess the text box can never really be empty here, but just for completeness
> (say that one day we want to reuse this code for the tiles on the start page),
> we should handle this case.
> 
> If the text box is empty, arrow keys should be free to navigate the tiles.

If the text box is empty, this will be 0 == 0? Should already be fine.

https://codereview.chromium.org/793633002/diff/1/ui/app_list/views/search_res...
ui/app_list/views/search_result_tile_item_list_view.cc:90: // the beginning of
the list. This means that the text cursor in the
On 2014/12/11 07:55:17, Matt Giuca wrote:
> I think this first sentence should go down below where IsValidSelectionIndex()
> is called. This code here does not care where the tile selection is.
> 
> Rewrite this comment to say essentially what the VKEY_RIGHT comment says.

Um.. But the fact that the behavior is specific to VKEY_LEFT is a good reason to
put it here. It's not that any dir == -1 will trigger this. We rely on the fact
that the search box will eventually handle this VKEY_LEFT to get this behavior.

If I'm looking for info on how the left key works, I'm going to look here, not
down at IsValidSelectionIndex().

https://codereview.chromium.org/793633002/diff/1/ui/app_list/views/search_res...
File ui/app_list/views/search_result_tile_item_list_view.h (right):

https://codereview.chromium.org/793633002/diff/1/ui/app_list/views/search_res...
ui/app_list/views/search_result_tile_item_list_view.h:41: views::Textfield*
search_box_;  // Owned by the views hierarchy.
On 2014/12/11 07:55:17, Matt Giuca wrote:
> // The ContentsView's search box. Used to inform the keyboard navigation of
> tiles (behaves differently depending on the text cursor position).

I think the first part of this comment is misleading (it implies CV owns SBV.
=S) and the second half is too easily out-dated.

[Matt Giuca, 2014-12-12 05:37:30]

lgtm

https://codereview.chromium.org/793633002/diff/1/ui/app_list/views/search_res...
File ui/app_list/views/search_result_tile_item_list_view.cc (right):

https://codereview.chromium.org/793633002/diff/1/ui/app_list/views/search_res...
ui/app_list/views/search_result_tile_item_list_view.cc:80:
search_box_->GetCursorPosition() == search_box_->text().length();
On 2014/12/12 05:23:41, calamity wrote:
> On 2014/12/11 07:55:17, Matt Giuca wrote:
> > || search_box_->text().empty();
> > 
> > I guess the text box can never really be empty here, but just for
completeness
> > (say that one day we want to reuse this code for the tiles on the start
page),
> > we should handle this case.
> > 
> > If the text box is empty, arrow keys should be free to navigate the tiles.
> 
> If the text box is empty, this will be 0 == 0? Should already be fine.

Acknowledged.

https://codereview.chromium.org/793633002/diff/1/ui/app_list/views/search_res...
ui/app_list/views/search_result_tile_item_list_view.cc:90: // the beginning of
the list. This means that the text cursor in the
On 2014/12/12 05:23:41, calamity wrote:
> On 2014/12/11 07:55:17, Matt Giuca wrote:
> > I think this first sentence should go down below where
IsValidSelectionIndex()
> > is called. This code here does not care where the tile selection is.
> > 
> > Rewrite this comment to say essentially what the VKEY_RIGHT comment says.
> 
> Um.. But the fact that the behavior is specific to VKEY_LEFT is a good reason
to
> put it here. It's not that any dir == -1 will trigger this. We rely on the
fact
> that the search box will eventually handle this VKEY_LEFT to get this
behavior.
> 
> If I'm looking for info on how the left key works, I'm going to look here, not
> down at IsValidSelectionIndex().

Acknowledged.

https://codereview.chromium.org/793633002/diff/1/ui/app_list/views/search_res...
File ui/app_list/views/search_result_tile_item_list_view.h (right):

https://codereview.chromium.org/793633002/diff/1/ui/app_list/views/search_res...
ui/app_list/views/search_result_tile_item_list_view.h:41: views::Textfield*
search_box_;  // Owned by the views hierarchy.
On 2014/12/12 05:23:41, calamity wrote:
> On 2014/12/11 07:55:17, Matt Giuca wrote:
> > // The ContentsView's search box. Used to inform the keyboard navigation of
> > tiles (behaves differently depending on the text cursor position).
> 
> I think the first part of this comment is misleading (it implies CV owns SBV.
> =S) and the second half is too easily out-dated.

Acknowledged.

[calamity, 2014-12-12 08:57:38]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-12-12 08:58:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/793633002/1

[commit-bot: I haz the power, 2014-12-12 09:21:47]

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

[commit-bot: I haz the power, 2014-12-12 09:21:47]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[calamity, 2014-12-15 01:41:35]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-12-15 01:42:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/793633002/1

[commit-bot: I haz the power, 2014-12-15 02:07:03]

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

[commit-bot: I haz the power, 2014-12-15 02:07:04]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[calamity, 2014-12-15 02:28:34]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-12-15 02:28:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/793633002/20001

[commit-bot: I haz the power, 2014-12-15 03:51:27]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-12-15 03:52:17]

Patchset 2 (id:??) landed as
https://crrev.com/60e5f1185e04c0bcb43f82b2e224de2ad463abee
Cr-Commit-Position: refs/heads/master@{#308310}