Fixes alternating keyboard and mouse hot-tracking in menus

Fixes alternating keyboard and mouse hot-tracking in menus

This scenario:
1. Open app menu with extension buttons overflow
2. Place mouse pointer over one of them (triggers hot-tracking)
Call that "button1"
3. Move visible focus with a keyboard (down) to the next extension button (call
that button2)
4. Move a mouse just enough to re-trigger a hot-tracked state on "button1".
5. Press [arrow down] key again.

This should move to "button2" but used to move instead to "button3" (since step
4 did not update the hot-tracked state in menu controller and the last updated
state in MenuController refers to step 3).

BUG=590351
TEST=see steps above

Committed: https://crrev.com/0a4f75a3aea3dd256ea741e524817ea7958b6f10
Cr-Commit-Position: refs/heads/master@{#378300}

[varkha, 2016-02-26 22:04:24]

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

[commit-bot: I haz the power, 2016-02-26 22:05:42]

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

[varkha, 2016-02-26 22:07:09]

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

[varkha, 2016-02-26 22:07:10]

sky@, I'd like you to review this CL. It was split from hot-tracking visuals CL
and implements updates to |hot_button_| state using technique that you have
suggested.
Still need to see if I need to do something similar for gestures but it should
be easy if needed.
Thanks!

https://codereview.chromium.org/1741093002/diff/1/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_item_view.cc (right):

https://codereview.chromium.org/1741093002/diff/1/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_item_view.cc:550: x -= width + kChildXPadding;
I think this is a typo that I came across while writing a test for hot-tracking.
The reason I think we have never noticed that is because we have container views
that host child buttons in app menu and those container views ensure proper
layout.
Consider line
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...
which is using padding correctly to add to the preferred width.

[varkha, 2016-02-26 22:09:06]

https://codereview.chromium.org/1741093002/diff/1/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/1741093002/diff/1/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_controller.cc:2111: SetHotTrackedButton(button_hot);
This block will not be necessary after
https://codereview.chromium.org/1661673004/ lands.

[varkha, 2016-02-26 22:24:46]

Description was changed from

==========
Fixes alternating keyboard and mouse hot-tracking in menus

This scenario:
1. Open app menu with extension buttons overflow
2. Place mouse pointer over one of them (triggers hot-tracking)
Call that "button1"
3. Move visible focus with a keyboard (down) to the next extension button (call
that button2)
4. Move a mouse just enough to re-trigger a hot-tracked state on "button1".
5. Press [arrow down] key again.

This should move to "button2" but used to move instead to "button3" (since step
4 did not update the hot-tracked state in menu controller and the last updated
state in MenuController refers to step 3).

BUG=TBD
TEST=see steps above
==========

to

==========
Fixes alternating keyboard and mouse hot-tracking in menus

This scenario:
1. Open app menu with extension buttons overflow
2. Place mouse pointer over one of them (triggers hot-tracking)
Call that "button1"
3. Move visible focus with a keyboard (down) to the next extension button (call
that button2)
4. Move a mouse just enough to re-trigger a hot-tracked state on "button1".
5. Press [arrow down] key again.

This should move to "button2" but used to move instead to "button3" (since step
4 did not update the hot-tracked state in menu controller and the last updated
state in MenuController refers to step 3).

BUG=590351
TEST=see steps above
==========

[commit-bot: I haz the power, 2016-02-26 22:30:23]

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

[commit-bot: I haz the power, 2016-02-26 22:30:24]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[varkha, 2016-02-26 22:34:19]

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

[varkha, 2016-02-26 22:34:42]

https://codereview.chromium.org/1741093002/diff/1/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/1741093002/diff/1/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_controller.cc:2111: SetHotTrackedButton(button_hot);
On 2016/02/26 22:09:06, varkha wrote:
> This block will not be necessary after
> https://codereview.chromium.org/1661673004/ lands.

Done.

[commit-bot: I haz the power, 2016-02-26 22:36:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1741093002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1741093002/20001

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

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

[commit-bot: I haz the power, 2016-02-26 23:09:22]

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

[varkha, 2016-02-27 00:23:50]

Patchset #2 (id:20001) has been deleted

[varkha, 2016-02-27 00:24:08]

Patchset #2 (id:40001) has been deleted

[varkha, 2016-02-27 00:24:38]

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

[commit-bot: I haz the power, 2016-02-27 00:27:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1741093002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1741093002/60001

[commit-bot: I haz the power, 2016-02-27 01:43:27]

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

[commit-bot: I haz the power, 2016-02-27 01:43:27]

Dry run: This issue passed the CQ dry run.

[sky, 2016-02-29 16:33:32]

https://codereview.chromium.org/1741093002/diff/60001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/1741093002/diff/60001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:572: return processed;
Do you need to update this branch to change the selection too?

https://codereview.chromium.org/1741093002/diff/60001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:822: // Update |hot_button_| if it
gets deleted while a menu is up.
deleted->removed

https://codereview.chromium.org/1741093002/diff/60001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:1053: CustomButton* button =
GetFirstHotTrackedView(pending_state_.item);
It doesn't seem necessary to call this anymore. Can't you just
SetHotTrackedButton(nullptr)?

[varkha, 2016-02-29 19:05:32]

Patchset #3 (id:80001) has been deleted

[varkha, 2016-02-29 19:09:44]

https://codereview.chromium.org/1741093002/diff/60001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/1741093002/diff/60001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:572: return processed;
On 2016/02/29 16:33:32, sky wrote:
> Do you need to update this branch to change the selection too?

Done. I could not see how another button could have acquired a HOVERED state
here but I can see how another button could get pressed or we should update
selection so that no button appears hot-tracked. This is why I think I need to
update (reset) hot-tracked state above if a view under mouse pointer is not 
equal to hot_tracked_.
Similar change done in OnGestureEvent() as well.

https://codereview.chromium.org/1741093002/diff/60001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:822: // Update |hot_button_| if it
gets deleted while a menu is up.
On 2016/02/29 16:33:32, sky wrote:
> deleted->removed

Done.

https://codereview.chromium.org/1741093002/diff/60001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:1053: CustomButton* button =
GetFirstHotTrackedView(pending_state_.item);
On 2016/02/29 16:33:32, sky wrote:
> It doesn't seem necessary to call this anymore. Can't you just
> SetHotTrackedButton(nullptr)?

Here, yes. Done.

https://codereview.chromium.org/1741093002/diff/100001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/1741093002/diff/100001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller.cc:104: if (button &&
button->IsHotTracked())
Equivalent change, just seems simpler.

https://codereview.chromium.org/1741093002/diff/100001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller.cc:2136: return;
Equivalent change, just seems simpler.

[sky, 2016-02-29 20:58:41]

LGTM

[varkha, 2016-02-29 21:32:06]

The CQ bit was checked by varkha@chromium.org

[commit-bot: I haz the power, 2016-02-29 21:33:00]

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

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

Description was changed from

==========
Fixes alternating keyboard and mouse hot-tracking in menus

This scenario:
1. Open app menu with extension buttons overflow
2. Place mouse pointer over one of them (triggers hot-tracking)
Call that "button1"
3. Move visible focus with a keyboard (down) to the next extension button (call
that button2)
4. Move a mouse just enough to re-trigger a hot-tracked state on "button1".
5. Press [arrow down] key again.

This should move to "button2" but used to move instead to "button3" (since step
4 did not update the hot-tracked state in menu controller and the last updated
state in MenuController refers to step 3).

BUG=590351
TEST=see steps above
==========

to

==========
Fixes alternating keyboard and mouse hot-tracking in menus

This scenario:
1. Open app menu with extension buttons overflow
2. Place mouse pointer over one of them (triggers hot-tracking)
Call that "button1"
3. Move visible focus with a keyboard (down) to the next extension button (call
that button2)
4. Move a mouse just enough to re-trigger a hot-tracked state on "button1".
5. Press [arrow down] key again.

This should move to "button2" but used to move instead to "button3" (since step
4 did not update the hot-tracked state in menu controller and the last updated
state in MenuController refers to step 3).

BUG=590351
TEST=see steps above
==========

[commit-bot: I haz the power, 2016-02-29 22:52:11]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-02-29 22:53:07]

Description was changed from

==========
Fixes alternating keyboard and mouse hot-tracking in menus

This scenario:
1. Open app menu with extension buttons overflow
2. Place mouse pointer over one of them (triggers hot-tracking)
Call that "button1"
3. Move visible focus with a keyboard (down) to the next extension button (call
that button2)
4. Move a mouse just enough to re-trigger a hot-tracked state on "button1".
5. Press [arrow down] key again.

This should move to "button2" but used to move instead to "button3" (since step
4 did not update the hot-tracked state in menu controller and the last updated
state in MenuController refers to step 3).

BUG=590351
TEST=see steps above
==========

to

==========
Fixes alternating keyboard and mouse hot-tracking in menus

This scenario:
1. Open app menu with extension buttons overflow
2. Place mouse pointer over one of them (triggers hot-tracking)
Call that "button1"
3. Move visible focus with a keyboard (down) to the next extension button (call
that button2)
4. Move a mouse just enough to re-trigger a hot-tracked state on "button1".
5. Press [arrow down] key again.

This should move to "button2" but used to move instead to "button3" (since step
4 did not update the hot-tracked state in menu controller and the last updated
state in MenuController refers to step 3).

BUG=590351
TEST=see steps above

Committed: https://crrev.com/0a4f75a3aea3dd256ea741e524817ea7958b6f10
Cr-Commit-Position: refs/heads/master@{#378300}
==========

[commit-bot: I haz the power, 2016-02-29 22:53:09]

Patchset 3 (id:??) landed as
https://crrev.com/0a4f75a3aea3dd256ea741e524817ea7958b6f10
Cr-Commit-Position: refs/heads/master@{#378300}