Fix keyboard navigation in app launcher (OSX)

Fix keyboard navigation in app launcher (OSX)

This causes left/right to page flip on boundary and up/down to do nothing. This
patch also makes out of bounds selections on the last page snap to the last
available item instead of not switching page.

This patch updates the OSX app launcher to be consistent with revision 200812.

BUG=237092
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202738

[tapted, 2013-05-22 10:30:50]

As discussed, I think we can roll the logic into handleCommandBySelector to
avoid doing essentially the same switch twice.

I don't have strong feelings about where things end up on the last page when
there's no target cell. Simple & consistent sounds good ;)

[jackhou1, 2013-05-23 00:44:38]

Yup, this is much neater.

[tapted, 2013-05-23 11:26:16]

oops - I didn't notice this got updated for some reason. lgtm! (with 1 comment)

https://codereview.chromium.org/15648003/diff/3001/ui/app_list/cocoa/apps_gri...
File ui/app_list/cocoa/apps_grid_controller_unittest.mm (right):

https://codereview.chromium.org/15648003/diff/3001/ui/app_list/cocoa/apps_gri...
ui/app_list/cocoa/apps_grid_controller_unittest.mm:300: [apps_grid_controller_
scrollToPage:0];
should this be here? (delete?)

[jackhou1, 2013-05-24 01:52:49]

Updated CL description.

https://codereview.chromium.org/15648003/diff/3001/ui/app_list/cocoa/apps_gri...
File ui/app_list/cocoa/apps_grid_controller_unittest.mm (right):

https://codereview.chromium.org/15648003/diff/3001/ui/app_list/cocoa/apps_gri...
ui/app_list/cocoa/apps_grid_controller_unittest.mm:300: [apps_grid_controller_
scrollToPage:0];
On 2013/05/23 11:26:16, tapted wrote:
> should this be here? (delete?)

Yeah the preceding code ends up on page 1, so scrollToPage:1 does nothing.

[commit-bot: I haz the power, 2013-05-24 01:53:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/15648003/3001

[commit-bot: I haz the power, 2013-05-24 01:53:08]

Failed to apply patch for ui/app_list/cocoa/apps_grid_controller_unittest.mm:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ui/app_list/cocoa/apps_grid_controller_unittest.mm
  Hunk #1 FAILED at 228.
  Hunk #2 succeeded at 274 (offset 32 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
ui/app_list/cocoa/apps_grid_controller_unittest.mm.rej

Patch:       ui/app_list/cocoa/apps_grid_controller_unittest.mm
Index: ui/app_list/cocoa/apps_grid_controller_unittest.mm
diff --git a/ui/app_list/cocoa/apps_grid_controller_unittest.mm
b/ui/app_list/cocoa/apps_grid_controller_unittest.mm
index
cae06439a5922c0d284917f3dfb2c609d66352db..81e934c1a0f4ab566a6d09254c804e07e4103566
100644
--- a/ui/app_list/cocoa/apps_grid_controller_unittest.mm
+++ b/ui/app_list/cocoa/apps_grid_controller_unittest.mm
@@ -228,9 +228,9 @@ TEST_F(AppsGridControllerTest, FirstPageKeyboardNavigation)
{
 TEST_F(AppsGridControllerTest, CrossPageKeyboardNavigation) {
   [AppsGridController setScrollAnimationDuration:0.0];
 
-  model()->PopulateApps(2 * kItemsPerPage);
+  model()->PopulateApps(kItemsPerPage + 10);
   EXPECT_EQ(kItemsPerPage, [[GetPageAt(0) content] count]);
-  EXPECT_EQ(kItemsPerPage, [[GetPageAt(1) content] count]);
+  EXPECT_EQ(10u, [[GetPageAt(1) content] count]);
 
   // Moving Left, Up, or PageUp from the top-left corner of the first page does
   // nothing.
@@ -242,77 +242,62 @@ TEST_F(AppsGridControllerTest,
CrossPageKeyboardNavigation) {
   SimulateKeyAction(@selector(scrollPageUp:));
   EXPECT_EQ(0u, [apps_grid_controller_ selectedItemIndex]);
 
-  // Moving Right from the right side wraps to the next row.
+  // Moving Right from the right side goes to the next page. Moving Left goes
+  // back to the first page.
   [apps_grid_controller_ selectItemAtIndex:3];
   SimulateKeyAction(@selector(moveRight:));
-  EXPECT_EQ(4u, [apps_grid_controller_ selectedItemIndex]);
+  EXPECT_EQ(1u, [apps_grid_controller_ visiblePage]);
+  EXPECT_EQ(kItemsPerPage, [apps_grid_controller_ selectedItemIndex]);
+  SimulateKeyAction(@selector(moveLeft:));
+  EXPECT_EQ(0u, [apps_grid_controller_ visiblePage]);
+  EXPECT_EQ(3u, [apps_grid_controller_ selectedItemIndex]);
 
-  // Moving Down from the bottom goes to the next page and selects the same
-  // column. Moving Up again goes back to the first page.
+  // Moving Down from the bottom does nothing.
   [apps_grid_controller_ selectItemAtIndex:13];
   EXPECT_EQ(0u, [apps_grid_controller_ visiblePage]);
   SimulateKeyAction(@selector(moveDown:));
-  EXPECT_EQ(1u, [apps_grid_controller_ visiblePage]);
-  EXPECT_EQ(kItemsPerPage + 1, [apps_grid_controller_ selectedItemIndex]);
-  SimulateKeyAction(@selector(moveUp:));
-  EXPECT_EQ(0u, [apps_grid_controller_ visiblePage]);
   EXPECT_EQ(13u, [apps_grid_controller_ selectedItemIndex]);
 
-  // Moving Right from the bottom-right corner goes to the next page.
-  // Moving Left again goes back to the first page.
+  // Moving Right into a non-existent square on the next page will select the
+  // last item.
   [apps_grid_controller_ selectItemAtIndex:15];
   EXPECT_EQ(0u, [apps_grid_controller_ visiblePage]);
   SimulateKeyAction(@selector(moveRight:));
   EXPECT_EQ(1u, [apps_grid_controller_ visiblePage]);
-  EXPECT_EQ(kItemsPerPage, [apps_grid_controller_ selectedItemIndex]);
-  SimulateKeyAction(@selector(moveLeft:));
-  EXPECT_EQ(0u, [apps_grid_controller_ visiblePage]);
-  EXPECT_EQ(15u, [apps_grid_controller_ selectedItemIndex]);
+  EXPECT_EQ(kItemsPerPage + 9, [apps_grid_controller_ selectedItemIndex]);
 
   // PageDown and PageUp switches pages while maintaining the same selection
   // position.
-  [apps_grid_controller_ selectItemAtIndex:10];
+  [apps_grid_controller_ selectItemAtIndex:6];
   EXPECT_EQ(0u, [apps_grid_controller_ visiblePage]);
   SimulateKeyAction(@selector(scrollPageDown:));
   EXPECT_EQ(1u, [apps_grid_controller_ visiblePage]);
-  EXPECT_EQ(kItemsPerPage + 10, [apps_grid_controller_ selectedItemIndex]);
+  EXPECT_EQ(kItemsPerPage + 6, [apps_grid_controller_ selectedItemIndex]);
   SimulateKeyAction(@selector(scrollPageUp:));
   EXPECT_EQ(0u, [apps_grid_controller_ visiblePage]);
-  EXPECT_EQ(10u, [apps_grid_controller_ selectedItemIndex]);
+  EXPECT_EQ(6u, [apps_grid_controller_ selectedItemIndex]);
 
-  // Moving Right, Down, or PageDown from the bottom-right corner of the last
-  // page does nothing.
-  [apps_grid_controller_ selectItemAtIndex:(2 * kItemsPerPage - 1)];
-  EXPECT_EQ(1u, [apps_grid_controller_ visiblePage]);
-  SimulateKeyAction(@selector(moveRight:));
-  EXPECT_EQ(kItemsPerPage + 15, [apps_grid_controller_ selectedItemIndex]);
-  SimulateKeyAction(@selector(moveDown:));
-  EXPECT_EQ(kItemsPerPage + 15, [apps_grid_controller_ selectedItemIndex]);
+  // PageDown into a non-existent square on the next page will select the last
+  // item.
+  [apps_grid_controller_ selectItemAtIndex:11];
+  EXPECT_EQ(0u, [apps_grid_controller_ visiblePage]);
   SimulateKeyAction(@selector(scrollPageDown:));
-  EXPECT_EQ(kItemsPerPage + 15, [apps_grid_controller_ selectedItemIndex]);
+  EXPECT_EQ(1u, [apps_grid_controller_ visiblePage]);
+  EXPECT_EQ(kItemsPerPage + 9, [apps_grid_controller_ selectedItemIndex]);
 
-  // Moving down on the bottom of the last page does nothing.
-  [apps_grid_controller_ selectItemAtIndex:(2 * kItemsPerPage - 4)];
+  // Moving Right, Down, or PageDown from the bottom-right corner of the last
+  // page (not the last item) does nothing.
+  [apps_grid_controller_ selectItemAtIndex:kItemsPerPage + 9];
   EXPECT_EQ(1u, [apps_grid_controller_ visiblePage]);
+  SimulateKeyAction(@selector(moveRight:));
+  EXPECT_EQ(kItemsPerPage + 9, [apps_grid_controller_ selectedItemIndex]);
   SimulateKeyAction(@selector(moveDown:));
-  EXPECT_EQ(kItemsPerPage + 12, [apps_grid_controller_ selectedItemIndex]);
-
-  // Remove some items from the last page.
-  ReplaceTestModel(kItemsPerPage + 8);
-  EXPECT_EQ(8u, [[GetPageAt(1) content] count]);
-  EXPECT_EQ(NSNotFound, [apps_grid_controller_ selectedItemIndex]);
-
-  // PageUp/Down to an invalid selection does nothing.
-  [apps_grid_controller_ selectItemAtIndex:10];
-  EXPECT_EQ(0u, [apps_grid_controller_ visiblePage]);
-  SimulateKeyAction(@selector(scrollPageUp:));
-  EXPECT_EQ(0u, [apps_grid_controller_ visiblePage]);
-  EXPECT_EQ(10u, [apps_grid_controller_ selectedItemIndex]);
+  EXPECT_EQ(kItemsPerPage + 9, [apps_grid_controller_ selectedItemIndex]);
   SimulateKeyAction(@selector(scrollPageDown:));
-  EXPECT_EQ(0u, [apps_grid_controller_ visiblePage]);
-  EXPECT_EQ(10u, [apps_grid_controller_ selectedItemIndex]);
+  EXPECT_EQ(kItemsPerPage + 9, [apps_grid_controller_ selectedItemIndex]);
 
   // After page switch, arrow keys select first item on current page.
+  [apps_grid_controller_ scrollToPage:0];
   [apps_grid_controller_ scrollToPage:1];
   EXPECT_EQ(NSNotFound, [apps_grid_controller_ selectedItemIndex]);
   SimulateKeyAction(@selector(moveUp:));

[commit-bot: I haz the power, 2013-05-27 00:35:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/15648003/10001

[commit-bot: I haz the power, 2013-05-27 03:07:36]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-29 01:42:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/15648003/10001

[commit-bot: I haz the power, 2013-05-29 01:43:09]

Change committed as 202738