MenuController Do Not SetSelection to Empty Items on Drag

MenuController Do Not SetSelection to Empty Items on Drag

There is a Chrome crash caused by the following:
  - Menu opened
  - Submenu of the above opened, with only an empty menu item
  - Context menu nested above the empty menu item
  - A dragging right-click outside of the context menu, and within the empty
    menu item
  - Pressing ESC once the context menu has relaunched

Empty menu items cannot be set as selection targets, and we do not allow arrow
keys to select them. In a normal mouse over they are ignored.

MenuController::OnMouseDragged however was not handling the result of
GetMenuPart which signified an empty menu item. It would attempt to update the
selection to an invalid state, and would hide the current menu.

The subsequent OnMouseReleased would bring back the context menu, as the drag
was only hiding the menu, not closing.

The subsequent ESC key would then attempt to be processed by the invalid menu
set in OnMouseDragged.

This change update OnMouseDragged to properly ingore mouse targets of empty menu
items. Bringing it inline with the behaviours of OnMouseMoved.

TEST=MenuControllerTest.RepostEventToEmptyMenuItem
BUG=704846
Review-Url: https://codereview.chromium.org/2778383002
Cr-Commit-Position: refs/heads/master@{#460449}
Committed: https://chromium.googlesource.com/chromium/src/+/1f74ed2b7c59a6202072a0ceee38bbfb922bfb61

[jonross, 2017-03-28 22:55:18]

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

[commit-bot: I haz the power, 2017-03-28 22:56:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jonross, 2017-03-28 22:56:46]

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

[jonross, 2017-03-28 22:56:47]

Hey sky@,

I have a fix for a crash that was found in menus, dating back to M30 :D

Could you PTAL.

Thanks,
Jon

[commit-bot: I haz the power, 2017-03-28 23:04:18]

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

[commit-bot: I haz the power, 2017-03-28 23:04:19]

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

[sky, 2017-03-28 23:47:00]

LGTM

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

https://codereview.chromium.org/2778383002/diff/1/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_controller.cc:630: if (!(!part.menu &&
part.submenu)) {
This is rather unreadable. How about:
if (part.menu || !part.submenu) ?

[jonross, 2017-03-29 13:13:31]

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

[commit-bot: I haz the power, 2017-03-29 13:13:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 13:53:45]

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

[commit-bot: I haz the power, 2017-03-29 13:53:45]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jonross, 2017-03-29 15:09:28]

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

[commit-bot: I haz the power, 2017-03-29 15:10:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 16:10:24]

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

[commit-bot: I haz the power, 2017-03-29 16:10:26]

Dry run: This issue passed the CQ dry run.

[jonross, 2017-03-29 16:58:24]

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

https://codereview.chromium.org/2778383002/diff/1/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_controller.cc:630: if (!(!part.menu &&
part.submenu)) {
On 2017/03/28 23:47:00, sky wrote:
> This is rather unreadable. How about:
> if (part.menu || !part.submenu) ?

Done.

[jonross, 2017-03-29 16:58:28]

The CQ bit was checked by jonross@chromium.org

[jonross, 2017-03-29 16:58:28]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2778383002/#ps60001
(title: "Make logic readible")

[commit-bot: I haz the power, 2017-03-29 16:59:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 17:55:06]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490806708268620,
"parent_rev": "7283eda4c582d4aa64838377970466ae6a4d2f2e", "commit_rev":
"1f74ed2b7c59a6202072a0ceee38bbfb922bfb61"}

[commit-bot: I haz the power, 2017-03-29 17:55:52]

Description was changed from

==========
MenuController Do Not SetSelection to Empty Items on Drag

There is a Chrome crash caused by the following:
  - Menu opened
  - Submenu of the above opened, with only an empty menu item
  - Context menu nested above the empty menu item
  - A dragging right-click outside of the context menu, and within the empty
    menu item
  - Pressing ESC once the context menu has relaunched

Empty menu items cannot be set as selection targets, and we do not allow arrow
keys to select them. In a normal mouse over they are ignored.

MenuController::OnMouseDragged however was not handling the result of
GetMenuPart which signified an empty menu item. It would attempt to update the
selection to an invalid state, and would hide the current menu.

The subsequent OnMouseReleased would bring back the context menu, as the drag
was only hiding the menu, not closing.

The subsequent ESC key would then attempt to be processed by the invalid menu
set in OnMouseDragged.

This change update OnMouseDragged to properly ingore mouse targets of empty menu
items. Bringing it inline with the behaviours of OnMouseMoved.

TEST=MenuControllerTest.RepostEventToEmptyMenuItem
BUG=704846
==========

to

==========
MenuController Do Not SetSelection to Empty Items on Drag

There is a Chrome crash caused by the following:
  - Menu opened
  - Submenu of the above opened, with only an empty menu item
  - Context menu nested above the empty menu item
  - A dragging right-click outside of the context menu, and within the empty
    menu item
  - Pressing ESC once the context menu has relaunched

Empty menu items cannot be set as selection targets, and we do not allow arrow
keys to select them. In a normal mouse over they are ignored.

MenuController::OnMouseDragged however was not handling the result of
GetMenuPart which signified an empty menu item. It would attempt to update the
selection to an invalid state, and would hide the current menu.

The subsequent OnMouseReleased would bring back the context menu, as the drag
was only hiding the menu, not closing.

The subsequent ESC key would then attempt to be processed by the invalid menu
set in OnMouseDragged.

This change update OnMouseDragged to properly ingore mouse targets of empty menu
items. Bringing it inline with the behaviours of OnMouseMoved.

TEST=MenuControllerTest.RepostEventToEmptyMenuItem
BUG=704846

Review-Url: https://codereview.chromium.org/2778383002
Cr-Commit-Position: refs/heads/master@{#460449}
Committed:
https://chromium.googlesource.com/chromium/src/+/1f74ed2b7c59a6202072a0ceee38...
==========

[commit-bot: I haz the power, 2017-03-29 17:55:53]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/1f74ed2b7c59a6202072a0ceee38...