Skips disabled menu items when selection is changed

Skips disabled menu items when selection is changed

BUG=297561
TEST=views_unittests --gtest_filter=*MenuControllerTest*SelectedItem*

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

[varkha, 2015-05-13 01:08:11]

varkha@chromium.org changed reviewers:
+ sadrul@chromium.org

[varkha, 2015-05-13 01:08:12]

sadrul@, can you please take a look?

[varkha, 2015-05-13 04:59:08]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-13 04:59:45]

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

[commit-bot: I haz the power, 2015-05-13 05:43:18]

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

[commit-bot: I haz the power, 2015-05-13 05:43:18]

Dry run: This issue passed the CQ dry run.

[sadrul, 2015-05-13 16:33:48]

Looks reasonable. Can this have a test?

[varkha, 2015-05-13 19:32:39]

Patchset #2 (id:20001) has been deleted

[varkha, 2015-05-13 19:35:33]

Patchset #2 (id:40001) has been deleted

[varkha, 2015-05-13 19:37:18]

Thanks, I have added a test for UP/DOWN key navigation. PTAL.

[varkha, 2015-05-13 20:00:39]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-13 20:01:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129203003/40002

[commit-bot: I haz the power, 2015-05-13 22:06:34]

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

[commit-bot: I haz the power, 2015-05-13 22:06:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[varkha, 2015-05-13 22:30:43]

Patchset #2 (id:40002) has been deleted

[varkha, 2015-05-13 22:31:02]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-13 22:31:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129203003/70001

[commit-bot: I haz the power, 2015-05-14 00:31:40]

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

[commit-bot: I haz the power, 2015-05-14 00:31:41]

Dry run: 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_...)

[sadrul, 2015-05-14 08:04:35]

The try failures on the mac bot look related to the CL. Mind taking a look at
those?

https://codereview.chromium.org/1129203003/diff/70001/chrome/test/base/view_e...
File chrome/test/base/view_event_test_base.h (right):

https://codereview.chromium.org/1129203003/diff/70001/chrome/test/base/view_e...
chrome/test/base/view_event_test_base.h:128: base::Closure
CreateEventTask(base::Closure closure) {
const &?

[sadrul, 2015-05-14 08:05:59]

Also, is this something we can test in a unit-test inside views? That I think
would be nicer.

[varkha, 2015-05-20 19:00:27]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-20 19:01:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129203003/110001

[commit-bot: I haz the power, 2015-05-20 20:35:44]

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

[commit-bot: I haz the power, 2015-05-20 20:35:46]

Dry run: This issue passed the CQ dry run.

[varkha, 2015-05-20 20:39:33]

Hi sadrul@, PTAL.

[sadrul, 2015-05-21 13:44:28]

https://codereview.chromium.org/1129203003/diff/110001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/1129203003/diff/110001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:349: // Disabling the item
"Two" gets it skipped when using keyboard to navigate.
Item "Three" is actually being disabled, right?

https://codereview.chromium.org/1129203003/diff/110001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:383: }
Would it be possible to add another test case where the first item of a
menu/submenu is disabled, and the test verifies that the right item is selected
when the [sub]menu opens?

[varkha, 2015-05-21 16:31:08]

Patchset #5 (id:130001) has been deleted

[varkha, 2015-05-21 16:31:14]

Patchset #5 (id:150001) has been deleted

[varkha, 2015-05-21 16:33:12]

Added MenuControllerTest.FirstSelectedItem, PTAL.

https://codereview.chromium.org/1129203003/diff/110001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/1129203003/diff/110001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:349: // Disabling the item
"Two" gets it skipped when using keyboard to navigate.
On 2015/05/21 13:44:28, sadrul wrote:
> Item "Three" is actually being disabled, right?

Done. Yes, good eye! Forgot to fix this after copy and paste.

https://codereview.chromium.org/1129203003/diff/110001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:383: }
On 2015/05/21 13:44:27, sadrul wrote:
> Would it be possible to add another test case where the first item of a
> menu/submenu is disabled, and the test verifies that the right item is
selected
> when the [sub]menu opens?

Done.

[varkha, 2015-05-21 19:05:54]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[sadrul, 2015-05-21 19:25:21]

Thanks! LGTM

[varkha, 2015-05-21 19:25:56]

The CQ bit was unchecked by varkha@chromium.org

[varkha, 2015-05-21 19:26:05]

The CQ bit was checked by varkha@chromium.org

[commit-bot: I haz the power, 2015-05-21 19:38:27]

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

[commit-bot: I haz the power, 2015-05-21 20:30:19]

Committed patchset #5 (id:170001)

[commit-bot: I haz the power, 2015-05-21 20:31:16]

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