Context menu is not getting hided in chrome://bookmarks after delete key press.

Context menu is not getting hided in chrome://bookmarks after delete key press.

We are getting context menu by selecting left mouse and by right mouse button
on 'Action icon' in bookmarked item.
'Delete' key is currently not handled by menu.js as their is no selected item in the context menu.
So then we should do preventDefault() and stopPropagation() in menu_button.js similar to context_menu_handler.js file.

So we have same behavior for selecting 'Delete' key when we have context menu on 'Action' button by left/right click of mouse. As as we are stopping propagation of event so no delete will happen, and it will not go in unstable state.
BUG=330587
Committed: https://crrev.com/f3700597c2e357838a8bbf2336fa329038c734e2
Cr-Commit-Position: refs/heads/master@{#325998}

[Deepak, 2015-04-19 12:16:36]

deepak.m1@samsung.com changed reviewers:
+ bauerb@chromium.org

[Deepak, 2015-04-19 12:16:37]

PTAL
Thanks

[Bernhard Bauer, 2015-04-20 10:46:40]

https://codereview.chromium.org/1098803002/diff/20001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/menu_button.js (right):

https://codereview.chromium.org/1098803002/diff/20001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/menu_button.js:253: case 'U+007F': // Delete
This doesn't seem like the right place to hide the menu. Note that this is a
general-purpose context menu which doesn't know that pressing the Delete key
will delete a bookmark.

Overall, I think the behavior for context menus spawned by right-clicking (where
the menu gets all key events) is the correct one -- see also the comment in
context_menu_handler.js: "If the menu is visible we let it handle all the
keyboard events". That comment also appears in this file in line 124, but the
code does not actually match that behavior (preventDefault() and
stopPropagation() is only called if the menu does not handle the key press), so
I think that is what we should fix.

[Deepak, 2015-04-20 12:38:35]

Thanks for review.
Changes done as suggested.
PTAL

https://codereview.chromium.org/1098803002/diff/20001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/menu_button.js (right):

https://codereview.chromium.org/1098803002/diff/20001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/menu_button.js:253: case 'U+007F': // Delete
On 2015/04/20 10:46:40, Bernhard Bauer wrote:
> This doesn't seem like the right place to hide the menu. Note that this is a
> general-purpose context menu which doesn't know that pressing the Delete key
> will delete a bookmark.
> 
> Overall, I think the behavior for context menus spawned by right-clicking
(where
> the menu gets all key events) is the correct one -- see also the comment in
> context_menu_handler.js: "If the menu is visible we let it handle all the
> keyboard events". That comment also appears in this file in line 124, but the
> code does not actually match that behavior (preventDefault() and
> stopPropagation() is only called if the menu does not handle the key press),
so
> I think that is what we should fix.

Thanks for giving me good understanding, I got your point.
Earlier deletion was happening as event get bubbled up as 'delete' key was not
handled. Now we are preventing this.
Now we have same behavior for selecting 'Delete' key by keyboard when we have
context menu on 'Action' button by left/right mouse click selection.
I have updated description also.

[Bernhard Bauer, 2015-04-20 17:06:45]

Thanks!

https://codereview.chromium.org/1098803002/diff/40001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/menu_button.js (right):

https://codereview.chromium.org/1098803002/diff/40001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/menu_button.js:126: this.menu.handleKeyDown(e);
Indent two spaces less (should be indented two more than the previous line).

[Deepak, 2015-04-21 04:06:37]

Thanks for review.
Indentation done.
PTAL

https://codereview.chromium.org/1098803002/diff/40001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/menu_button.js (right):

https://codereview.chromium.org/1098803002/diff/40001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/menu_button.js:126: this.menu.handleKeyDown(e);
On 2015/04/20 17:06:45, Bernhard Bauer wrote:
> Indent two spaces less (should be indented two more than the previous line).

Done.

[Bernhard Bauer, 2015-04-21 07:40:09]

LGTM

[Deepak, 2015-04-21 07:40:38]

On 2015/04/21 07:40:09, Bernhard Bauer wrote:
> LGTM

Thanks.

[Deepak, 2015-04-21 07:40:41]

The CQ bit was checked by deepak.m1@samsung.com

[commit-bot: I haz the power, 2015-04-21 07:40:54]

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

[commit-bot: I haz the power, 2015-04-21 09:22:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-21 09:23:35]

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