context menu does not work if one already open

context menu does not work if one already open

This improves how ChromeOS handles mouse button presses 
outside of a context menu. On mouse button presses not
only is the context menu and is message loop exited, but
the event is processed with the same result as if the 
context menu had not been present. For example, a window
may be activated, another context menu may be brought up,
a new tab button may be executed, etc. This CL does not
yet handle touch events (separate issue).

The behavior is now more similar to how it works on Windows.

BUG=151074, 162172
TEST=manual
Bring up various context menus, then click various mouse
buttons outside of it.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178717

[sschmitz, 2013-01-04 00:41:05]

Scott, this CL needs more work. It accomplishes most of what we want with some
issues. It does not yet handle gestures. Before continuing, I want to confer
with you to see what you think and to get help on some of the fine points. Thx,
Stefan

[sschmitz, 2013-01-04 22:56:50]

Fixed problem with unit test. Had to be more careful as to when reposting a
mouse event.

[sky, 2013-01-07 16:06:13]

https://codereview.chromium.org/11761027/diff/18001/ui/views/controls/menu/me...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/11761027/diff/18001/ui/views/controls/menu/me...
ui/views/controls/menu/menu_controller.cc:849:
root_window->PostNativeEvent(&xevent);
Is PostNativeEvent async? I think this code will roughly need to mirror the
existing RepostEvent. In fact this should be in RepostEvent for X11.

It's also worth talking with Oshima about this since he initially thought we
didn't need this logic (the comment you're removing on 799).

[sschmitz, 2013-01-08 19:49:27]

Improved after talks with Oshimasan. Using XPutBackEvent to process the event
again after context menu has closed and its nested message loop has exited.
I'm cc'ing oshima@b and sadrul@. Please take a look.
I have not started to looked at touch gestures (longpress) yet. Any advice?
Thanks, Stefan

[oshima, 2013-01-08 20:01:51]

https://codereview.chromium.org/11761027/diff/31001/ui/base/events/event.cc
File ui/base/events/event.cc (right):

https://codereview.chromium.org/11761027/diff/31001/ui/base/events/event.cc#n...
ui/base/events/event.cc:328: int MouseEvent::GetRepeatCount(const MouseEvent&
event) {
It's better to check if it's synthesized event here
so that the reposted event generates same mouse event.

[sadrul, 2013-01-08 20:03:32]

On 2013/01/07 16:06:13, sky wrote:
>
https://codereview.chromium.org/11761027/diff/18001/ui/views/controls/menu/me...
> File ui/views/controls/menu/menu_controller.cc (right):
> 
>
https://codereview.chromium.org/11761027/diff/18001/ui/views/controls/menu/me...
> ui/views/controls/menu/menu_controller.cc:849:
> root_window->PostNativeEvent(&xevent);
> Is PostNativeEvent async? I think this code will roughly need to mirror the
> existing RepostEvent. In fact this should be in RepostEvent for X11.
> 
> It's also worth talking with Oshima about this since he initially thought we
> didn't need this logic (the comment you're removing on 799).

Hm. I added a RepostEvent a long while back in a CL:
https://codereview.chromium.org/9466028/ I think the original bug was already
fixed differently, and I wasn't confident this would be necessary.

sschmitz@ Would you mind looking at that CL and see if that can be of use here?

For touch-events, you would need to XSend/XPutBack an XI2.2 GenericEvent with
the appropriate cookie. I am not sure if/how that can be done.

[sschmitz, 2013-01-08 20:36:38]

Sadrul,
thanks for the quick reply. Your fix is very similar. I don't understand
why it wasn't in menu_controller anymore. I will make some changes based on
yours. I think XPutBackEvent has an advantage over
RootWindow::PostNativeEvent which uses XSendEvent. And since I do not
generate a new XEvent, but use the existing one, is there any chance that
it works for a gesture event?
Stefan


On Tue, Jan 8, 2013 at 12:03 PM, <sadrul@chromium.org> wrote:

> On 2013/01/07 16:06:13, sky wrote:
>
> https://codereview.chromium.**org/11761027/diff/18001/ui/**
>
views/controls/menu/menu_**controller.cc<https://codereview.chromium.org/11761027/diff/18001/ui/views/controls/menu/menu_controller.cc>
>
>> File ui/views/controls/menu/menu_**controller.cc (right):
>>
>
>
> https://codereview.chromium.**org/11761027/diff/18001/ui/**
>
views/controls/menu/menu_**controller.cc#newcode849<https://codereview.chromium.org/11761027/diff/18001/ui/views/controls/menu/menu_controller.cc#newcode849>
>
>> ui/views/controls/menu/menu_**controller.cc:849:
>> root_window->PostNativeEvent(&**xevent);
>> Is PostNativeEvent async? I think this code will roughly need to mirror
>> the
>> existing RepostEvent. In fact this should be in RepostEvent for X11.
>>
>
>  It's also worth talking with Oshima about this since he initially thought
>> we
>> didn't need this logic (the comment you're removing on 799).
>>
>
> Hm. I added a RepostEvent a long while back in a CL:
>
https://codereview.chromium.**org/9466028/<https://codereview.chromium.org/94...
think the original bug was already
> fixed differently, and I wasn't confident this would be necessary.
>
> sschmitz@ Would you mind looking at that CL and see if that can be of use
> here?
>
> For touch-events, you would need to XSend/XPutBack an XI2.2 GenericEvent
> with
> the appropriate cookie. I am not sure if/how that can be done.
>
>
https://codereview.chromium.**org/11761027/<https://codereview.chromium.org/1...
>

[sschmitz, 2013-01-08 22:48:57]

Improved based on feedback.

https://codereview.chromium.org/11761027/diff/18001/ui/views/controls/menu/me...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/11761027/diff/18001/ui/views/controls/menu/me...
ui/views/controls/menu/menu_controller.cc:849:
root_window->PostNativeEvent(&xevent);
On 2013/01/07 16:06:13, sky wrote:
> Is PostNativeEvent async? I think this code will roughly need to mirror the
> existing RepostEvent. In fact this should be in RepostEvent for X11.
> 
> It's also worth talking with Oshima about this since he initially thought we
> didn't need this logic (the comment you're removing on 799).

Done.

https://codereview.chromium.org/11761027/diff/31001/ui/base/events/event.cc
File ui/base/events/event.cc (right):

https://codereview.chromium.org/11761027/diff/31001/ui/base/events/event.cc#n...
ui/base/events/event.cc:328: int MouseEvent::GetRepeatCount(const MouseEvent&
event) {
On 2013/01/08 20:01:51, oshima wrote:
> It's better to check if it's synthesized event here
> so that the reposted event generates same mouse event.

Done.

[oshima, 2013-01-08 23:26:10]

https://codereview.chromium.org/11761027/diff/37004/ui/views/controls/menu/me...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/11761027/diff/37004/ui/views/controls/menu/me...
ui/views/controls/menu/menu_controller.cc:37: #if defined(OS_CHROMEOS)
shouldn't we use USE_X11?

https://codereview.chromium.org/11761027/diff/37004/ui/views/controls/menu/me...
ui/views/controls/menu/menu_controller.cc:2024:
XPutBackEvent(xevent.xany.display, &xevent);
Won't this cause clicking on a button actually activating the button after menu
gets closed? If so, I don't think that's desirable behavior. Maybe we should do
this only if the menu gets closed by clicking by left button in outside of the
menus?

[sschmitz, 2013-01-08 23:33:47]

Good point. I showed this to Alex and discussed this with him. He likes it.
We can fine-tune this easily as you pointed out.



On Tue, Jan 8, 2013 at 3:26 PM, <oshima@chromium.org> wrote:

>
> https://codereview.chromium.**org/11761027/diff/37004/ui/**
>
views/controls/menu/menu_**controller.cc<https://codereview.chromium.org/11761027/diff/37004/ui/views/controls/menu/menu_controller.cc>
> File ui/views/controls/menu/menu_**controller.cc (right):
>
> https://codereview.chromium.**org/11761027/diff/37004/ui/**
>
views/controls/menu/menu_**controller.cc#newcode37<https://codereview.chromium.org/11761027/diff/37004/ui/views/controls/menu/menu_controller.cc#newcode37>
> ui/views/controls/menu/menu_**controller.cc:37: #if defined(OS_CHROMEOS)
> shouldn't we use USE_X11?
>
> https://codereview.chromium.**org/11761027/diff/37004/ui/**
>
views/controls/menu/menu_**controller.cc#newcode2024<https://codereview.chromium.org/11761027/diff/37004/ui/views/controls/menu/menu_controller.cc#newcode2024>
> ui/views/controls/menu/menu_**controller.cc:2024:
> XPutBackEvent(xevent.xany.**display, &xevent);
> Won't this cause clicking on a button actually activating the button
> after menu gets closed? If so, I don't think that's desirable behavior.
> Maybe we should do this only if the menu gets closed by clicking by left
> button in outside of the menus?
>
>
https://codereview.chromium.**org/11761027/<https://codereview.chromium.org/1...
>

[sschmitz, 2013-01-18 20:30:15]

Thanks for all discussions. I'm submitting this now for review. I'm splitting of
the work for touch events into a separate issue.

https://codereview.chromium.org/11761027/diff/37004/ui/views/controls/menu/me...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/11761027/diff/37004/ui/views/controls/menu/me...
ui/views/controls/menu/menu_controller.cc:37: #if defined(OS_CHROMEOS)
On 2013/01/08 23:26:10, oshima wrote:
> shouldn't we use USE_X11?

Done.

https://codereview.chromium.org/11761027/diff/37004/ui/views/controls/menu/me...
ui/views/controls/menu/menu_controller.cc:2024:
XPutBackEvent(xevent.xany.display, &xevent);
On 2013/01/08 23:26:10, oshima wrote:
> Won't this cause clicking on a button actually activating the button after
menu
> gets closed? If so, I don't think that's desirable behavior. Maybe we should
do
> this only if the menu gets closed by clicking by left button in outside of the
> menus?
The function is only invoked when the mouse was not over any menu. See comment
in the caller.
Yes, clicking the left button will close the menu and then activate what's
below. I showed it to kuscher@ and he wanted it. This also works on Windows
(very similarly).

[oshima, 2013-01-18 22:05:16]

lgtm

[commit-bot: I haz the power, 2013-01-18 22:08:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sschmitz@chromium.org/11761027/41001

[commit-bot: I haz the power, 2013-01-18 22:08:38]

Presubmit check for 11761027-41001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ui/views
    ui

Presubmit checks took 1.2s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[sky, 2013-01-22 22:35:11]

This LGTM, but this is so subtle it's worth a test. Could you add a test for
this?

[sschmitz, 2013-01-24 04:08:06]

On 2013/01/22 22:35:11, sky wrote:
> This LGTM, but this is so subtle it's worth a test. Could you add a test for
> this?

I will add a unit test. I created Issue 171891 for that.

[commit-bot: I haz the power, 2013-01-24 04:09:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sschmitz@chromium.org/11761027/41001

[commit-bot: I haz the power, 2013-01-24 04:28:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sschmitz@chromium.org/11761027/41001

[commit-bot: I haz the power, 2013-01-24 22:47:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sschmitz@chromium.org/11761027/41001

[commit-bot: I haz the power, 2013-01-25 01:22:56]

Change committed as 178717