mash: Close system tray bubble on click outside its bounds, part 1

mash: Close system tray bubble on click outside its bounds, part 1

Mus does not support global event handlers, so the existing TrayEventFilter
cannot be used to close the bubble. Instead we use mouse capture.

* Capture the mouse explicitly when the bubble is spawned and release it on
clicks outside its bounds.
* Always close the bubble on capture loss, in case some other part of the
window system breaks capture.

Also fix an issue with auto-release-capture at the widget level. If a view
explicitly requested capture via Widget::SetCapture(my_view) and explicitly
did not request auto-release-capture, then any mouse release event would
cause the view to lose capture (that is, being an explicit mouse handler
target at the RootView level). Fix this so that these sorts of views will
continue to receive events until they explicit release capture.

Note that this code still eats the mousedown event that causes the bubble to
close. Fixing that is part 2.

BUG=599142
Committed: https://crrev.com/5eb778dc5a2a4762fe95d67f2adc151f928d7939
Cr-Commit-Position: refs/heads/master@{#386402}

[James Cook, 2016-04-08 21:46:45]

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

[commit-bot: I haz the power, 2016-04-08 21:47:06]

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

[commit-bot: I haz the power, 2016-04-08 23:02:19]

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

[commit-bot: I haz the power, 2016-04-08 23:02:20]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-04-09 17:14:16]

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

[commit-bot: I haz the power, 2016-04-09 17:14:19]

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

[James Cook, 2016-04-09 17:28:20]

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

[James Cook, 2016-04-09 17:28:21]

sky, please take a look.

I tried again to get widget_interactive_uitests.cc to run in views_mus_unittests
but couldn't get it working. I captured what I know about it in
https://bugs.chromium.org/p/chromium/issues/detail?id=602036

[commit-bot: I haz the power, 2016-04-09 17:52:37]

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

[commit-bot: I haz the power, 2016-04-09 17:52:38]

Dry run: This issue passed the CQ dry run.

[sky, 2016-04-11 15:25:45]

LGTM

https://codereview.chromium.org/1876553002/diff/40001/ui/views/widget/root_vi...
File ui/views/widget/root_view.cc (right):

https://codereview.chromium.org/1876553002/diff/40001/ui/views/widget/root_vi...
ui/views/widget/root_view.cc:445: if (clear_mouse_handler_on_release_) {
nit: no {}

[James Cook, 2016-04-11 15:38:54]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-04-11 15:38:54]

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/1876553002/#ps60001
(title: "review comments")

[commit-bot: I haz the power, 2016-04-11 15:39:08]

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

[commit-bot: I haz the power, 2016-04-11 16:30:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-11 16:32:02]

Description was changed from

==========
mash: Close system tray bubble on click outside its bounds, part 1

Mus does not support global event handlers, so the existing TrayEventFilter
cannot be used to close the bubble. Instead we use mouse capture.

* Capture the mouse explicitly when the bubble is spawned and release it on
clicks outside its bounds.
* Always close the bubble on capture loss, in case some other part of the
window system breaks capture.

Also fix an issue with auto-release-capture at the widget level. If a view
explicitly requested capture via Widget::SetCapture(my_view) and explicitly
did not request auto-release-capture, then any mouse release event would
cause the view to lose capture (that is, being an explicit mouse handler
target at the RootView level). Fix this so that these sorts of views will
continue to receive events until they explicit release capture.

Note that this code still eats the mousedown event that causes the bubble to
close. Fixing that is part 2.

BUG=599142
==========

to

==========
mash: Close system tray bubble on click outside its bounds, part 1

Mus does not support global event handlers, so the existing TrayEventFilter
cannot be used to close the bubble. Instead we use mouse capture.

* Capture the mouse explicitly when the bubble is spawned and release it on
clicks outside its bounds.
* Always close the bubble on capture loss, in case some other part of the
window system breaks capture.

Also fix an issue with auto-release-capture at the widget level. If a view
explicitly requested capture via Widget::SetCapture(my_view) and explicitly
did not request auto-release-capture, then any mouse release event would
cause the view to lose capture (that is, being an explicit mouse handler
target at the RootView level). Fix this so that these sorts of views will
continue to receive events until they explicit release capture.

Note that this code still eats the mousedown event that causes the bubble to
close. Fixing that is part 2.

BUG=599142

Committed: https://crrev.com/5eb778dc5a2a4762fe95d67f2adc151f928d7939
Cr-Commit-Position: refs/heads/master@{#386402}
==========

[commit-bot: I haz the power, 2016-04-11 16:32:04]

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

[msw, 2016-04-11 16:52:19]

msw@chromium.org changed reviewers:
+ msw@chromium.org

[msw, 2016-04-11 16:52:20]

driveby comment nit

https://codereview.chromium.org/1876553002/diff/60001/ui/views/widget/root_vi...
File ui/views/widget/root_view.h (right):

https://codereview.chromium.org/1876553002/diff/60001/ui/views/widget/root_vi...
ui/views/widget/root_view.h:165: // must be explicitly by calling
SetMouseHandler() with null.
nit: 'explicitly released'

[sadrul, 2016-04-14 20:08:34]

sadrul@chromium.org changed reviewers:
+ sadrul@chromium.org

[sadrul, 2016-04-14 20:08:35]

https://codereview.chromium.org/1876553002/diff/60001/ui/views/bubble/tray_bu...
File ui/views/bubble/tray_bubble_view.cc (right):

https://codereview.chromium.org/1876553002/diff/60001/ui/views/bubble/tray_bu...
ui/views/bubble/tray_bubble_view.cc:363: GetWidget()->SetCapture(this);
This unfortunately breaks clicking in the tray-bubble. This is because
mouse-press etc. events in the bubble go to |this| (TrayBubbleView), instead of
the view under the cursor. As a result, clicking on the bluetooth row, it
doesn't take us to the relevant submenu.

If you use nullptr instead of |this|, then I think that would fix this.

[James Cook, 2016-04-14 22:52:19]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1887263002/ by jamescook@chromium.org.

The reason for reverting is: Breaks clicking in the system tray bubble on mus.
We need an entirely different architecture for this.
.

[James Cook, 2016-04-14 23:05:12]

https://codereview.chromium.org/1876553002/diff/60001/ui/views/bubble/tray_bu...
File ui/views/bubble/tray_bubble_view.cc (right):

https://codereview.chromium.org/1876553002/diff/60001/ui/views/bubble/tray_bu...
ui/views/bubble/tray_bubble_view.cc:363: GetWidget()->SetCapture(this);
On 2016/04/14 20:08:35, sadrul wrote:
> This unfortunately breaks clicking in the tray-bubble. This is because
> mouse-press etc. events in the bubble go to |this| (TrayBubbleView), instead
of
> the view under the cursor. As a result, clicking on the bluetooth row, it
> doesn't take us to the relevant submenu.
> 
> If you use nullptr instead of |this|, then I think that would fix this.

Unfortunately setting the capture to a nullptr view doesn't work - clicking in
the bubble works, but it doesn't close, because no view is notified of events
outside the widget's bounds. I don't see a good way around this problem.

I talked to Scott - I'm going to revert this. We need a different approach.