Add menuhide event to menu button.

Add menuhide event to menu button.

BUG=499160
TEST=browser_tests:WebUIResourceBrowserTest.MenuButtonTest

Committed: https://crrev.com/2d99b64d0f236fc239d5321c75fbdfb05bf303d0
Cr-Commit-Position: refs/heads/master@{#334357}

[yawano, 2015-06-11 07:27:11]

yawano@chromium.org changed reviewers:
+ dbeam@chromium.org

[yawano, 2015-06-11 07:27:11]

PTAL. Thank you!

We want to use this event to implement http://crrev.com/1170913006.

[yawano, 2015-06-11 08:15:22]

Added test case. PTAL. Thank you!

[Dan Beam, 2015-06-12 16:33:24]

looks pretty good to me, but I was wondering how you're going to use 'menuhide'?
 this CL doesn't use it and the associated BUG= is pretty brief.

one wonders how the rest of chrome webui survived without 'menuhide' this long. 
maybe arv@ (CC'd) will know if it's intentional there's only been a 'menushow'.

it's possible that code could just listen for 'focus' changes (this.focus() is
called right before where you're adding 'menuhide'), but it's likely that
there's more ways for the button to get focused than just hiding the menu and
it'd probably result in confusing code.

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

https://codereview.chromium.org/1178783003/diff/40001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/menu_button.js:232: event.initUIEvent('menuhide',
true, true, window, null);
third arg should be false, i.e.:

  event.initUIEvent('menuhide', true, false, window, null);

as it's not cancelable (calling preventDefault() does nothing, for instance,
whereas it does for 'menushow')

btw, this method is deprecated and MDN advises against using it in new code:
https://developer.mozilla.org/en-US/docs/Web/API/UIEvent/initUIEvent

[fukino, 2015-06-12 17:38:06]

On 2015/06/12 16:33:24, Dan Beam wrote:
> looks pretty good to me, but I was wondering how you're going to use
'menuhide'?
>  this CL doesn't use it and the associated BUG= is pretty brief.
> 
> one wonders how the rest of chrome webui survived without 'menuhide' this
long. 
> maybe arv@ (CC'd) will know if it's intentional there's only been a
'menushow'.
> 
> it's possible that code could just listen for 'focus' changes (this.focus() is
> called right before where you're adding 'menuhide'), but it's likely that
> there's more ways for the button to get focused than just hiding the menu and
> it'd probably result in confusing code.
> 
>
https://codereview.chromium.org/1178783003/diff/40001/ui/webui/resources/js/c...
> File ui/webui/resources/js/cr/ui/menu_button.js (right):
> 
>
https://codereview.chromium.org/1178783003/diff/40001/ui/webui/resources/js/c...
> ui/webui/resources/js/cr/ui/menu_button.js:232: event.initUIEvent('menuhide',
> true, true, window, null);
> third arg should be false, i.e.:
> 
>   event.initUIEvent('menuhide', true, false, window, null);
> 
> as it's not cancelable (calling preventDefault() does nothing, for instance,
> whereas it does for 'menushow')
> 
> btw, this method is deprecated and MDN advises against using it in new code:
> https://developer.mozilla.org/en-US/docs/Web/API/UIEvent/initUIEvent

Thanks Dan!
Let me add some context.

https://codereview.chromium.org/1170913006 is the original CL which needed/added
'menuhide' event.
I asked yawano@ to separate webui part from others, as I thought this public API
addition could be reviewed independently.

Regarding the deprecation of initUIEvent, I suggested him to keep symmetric with
menushow for now.
As the only use of initUIEvent in cr.ui library is 'menushow', maybe we should
fix the construction of 'menushow' event along with this CL.

[Dan Beam, 2015-06-12 18:29:07]

lgtm

[Dan Beam, 2015-06-12 18:29:59]

On 2015/06/12 17:38:06, fukino wrote:
> Regarding the deprecation of initUIEvent, I suggested him to keep symmetric
with
> menushow for now.
> As the only use of initUIEvent in cr.ui library is 'menushow', maybe we should
> fix the construction of 'menushow' event along with this CL.

fwiw: this would be symmetric... if we updated both sites as well :)

[yawano, 2015-06-15 04:17:34]

Thank you for the review and responses!

While I had a concern to break existing code by changing initUIEvent, as far as
I quickly searched the code base, menushow event is used only by file manager (
https://code.google.com/p/chromium/codesearch#search/&q=menushow%20.js$ ).

I added a new patch set to remove initUIEvent to this CL.
PTAL again. Thank you!

[Dan Beam, 2015-06-15 05:55:01]

lgtm

[yawano, 2015-06-15 06:14:49]

On 2015/06/15 05:55:01, Dan Beam wrote:
> lgtm

Thank you!

[yawano, 2015-06-15 06:14:53]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2015-06-15 06:15:03]

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

[commit-bot: I haz the power, 2015-06-15 08:11:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-06-15 08:13:11]

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