Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1168)

Issue 1565013002: Don't send touch events to windows like menus when the touch occurs outside the menu bounds. (Closed)

Created:
4 years, 11 months ago by ananta
Modified:
4 years, 11 months ago
Reviewers:
jonross, sadrul, sky, tdresser
CC:
chromium-reviews, tfarina, tdanderson+views_chromium.org, tdresser+watch_chromium.org, jonross
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't send touch events to windows like menus when the touch occurs outside the menu bounds. Along with not sending touch events to the window with capture, we also need to release capture when we receive a touch for a different target. We use the RepostEvent functionality in the menu code and the WindowEventDispatcher to achieve this. These functions have been updated to support touch. Changes in this patch are as below:- 1. Handle touch events in the menu host via a pre target handler set on the menu window. This event is forwarded to the menu controller which determines if the touch occurred outside the menu bounds. If yes it reposts the event to the appropriate window using the RepostEvent functionality. 2. The MenuController::RepostEvent, MenuController::SetSelectionOnPointerDown, MenuMessageLoop::RepostEventToWindow functions now take in a LocatedEvent* instead of the copied event. This is needed to ensure that we don't lose information while downcasting the event. 3. Added support to dispatch touch events in the WindowEventDispatcher. The earlier approach of using a window property to determine if capture should be released on touch outside is not needed anymore. 4. Fix the DCHECK in the TouchDispositionGestureFilter::OnGesturePacket function which fires if we have duplicate packets with the same unique_touch_event_id. This does fire if we have multiple touch_timeout packets in the queue. These packets have a unique_touch_event_id of 0. BUG=562739 Committed: https://crrev.com/575088602f6e03d99bbc280d1ec8bcede6081315 Cr-Commit-Position: refs/heads/master@{#369561}

Patch Set 1 #

Patch Set 2 : Release capture only if we get a touch down event #

Patch Set 3 : Use a window property to identify menu context #

Patch Set 4 : Add comment #

Total comments: 4

Patch Set 5 : Post the touch event via a task #

Patch Set 6 : Remove include #

Total comments: 2

Patch Set 7 : Set the window property on the menu host window #

Patch Set 8 : Revert changes to menu_controller.cc #

Patch Set 9 : Handle touch events in the menu host and use the RepostEvent functionality to post it to the correc… #

Patch Set 10 : Remove unnecessary includes #

Patch Set 11 : Revert changes to desktop_window_tree_host_win.h #

Patch Set 12 : Cleanup #

Patch Set 13 : Fix build redness #

Total comments: 10

Patch Set 14 : Replace template args with LocatedEvent* #

Patch Set 15 : Remove include #

Patch Set 16 : Observe the window in the PreMenuEventDispatchHandler class and remove the handlers when it is goin… #

Patch Set 17 : Fix build redness #

Patch Set 18 : Small cleanup in the WindowEventDispatcher::RepostEvent function #

Total comments: 4

Patch Set 19 : Remove explicit from the ctor for the PreMenuEventDispatchHandler class #

Total comments: 4

Patch Set 20 : Address review comments. Moved the code to get the WindowTreeHost instance from the parent to menu_… #

Total comments: 4

Patch Set 21 : Clean up code in MenuController::RepostEvent #

Patch Set 22 : Fixed comment #

Total comments: 2

Patch Set 23 : Use WindowTreeHost to get the aura Window #

Total comments: 6

Patch Set 24 : Revert the second DCHECK in touch_disposition_gesture_filter.cc #

Patch Set 25 : Remove DCHECK and update comment #

Total comments: 2

Patch Set 26 : Restore DCHECK #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+196 lines, -65 lines) Patch
M ui/aura/window_event_dispatcher.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +3 lines, -3 lines 0 comments Download
M ui/aura/window_event_dispatcher.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +23 lines, -16 lines 0 comments Download
M ui/aura/window_event_dispatcher_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 9 chunks +17 lines, -10 lines 0 comments Download
M ui/events/gesture_detection/touch_disposition_gesture_filter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 25 1 chunk +8 lines, -3 lines 0 comments Download
M ui/views/controls/menu/menu_controller.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +3 lines, -2 lines 0 comments Download
M ui/views/controls/menu/menu_controller.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 13 chunks +45 lines, -23 lines 1 comment Download
M ui/views/controls/menu/menu_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +13 lines, -0 lines 0 comments Download
M ui/views/controls/menu/menu_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 4 chunks +69 lines, -0 lines 0 comments Download
M ui/views/controls/menu/menu_message_loop.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -1 line 0 comments Download
M ui/views/controls/menu/menu_message_loop_aura.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +10 lines, -6 lines 0 comments Download
M ui/views/controls/menu/menu_message_loop_mac.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -1 line 0 comments Download
M ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +3 lines, -0 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 47 (13 generated)
ananta
4 years, 11 months ago (2016-01-07 00:01:39 UTC) #2
sky
Have you verified this doesn't break dragging the tabstrip? Specifically what happens if you start ...
4 years, 11 months ago (2016-01-07 00:42:05 UTC) #3
ananta
On 2016/01/07 00:42:05, sky wrote: > Have you verified this doesn't break dragging the tabstrip? ...
4 years, 11 months ago (2016-01-07 03:28:55 UTC) #5
sky
https://codereview.chromium.org/1565013002/diff/60001/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1565013002/diff/60001/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc#newcode830 ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:830: if ((event.type() == ui::ET_TOUCH_PRESSED) && What happens when you ...
4 years, 11 months ago (2016-01-07 16:17:57 UTC) #6
ananta
https://codereview.chromium.org/1565013002/diff/60001/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1565013002/diff/60001/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc#newcode830 ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:830: if ((event.type() == ui::ET_TOUCH_PRESSED) && On 2016/01/07 16:17:57, sky ...
4 years, 11 months ago (2016-01-07 19:54:45 UTC) #7
sky
https://codereview.chromium.org/1565013002/diff/60001/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1565013002/diff/60001/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc#newcode848 ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:848: } Correct me if I'm wrong, but if we ...
4 years, 11 months ago (2016-01-08 16:05:12 UTC) #8
ananta
https://codereview.chromium.org/1565013002/diff/60001/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1565013002/diff/60001/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc#newcode848 ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:848: } On 2016/01/08 16:05:12, sky wrote: > Correct me ...
4 years, 11 months ago (2016-01-08 23:34:10 UTC) #9
sky
https://codereview.chromium.org/1565013002/diff/100001/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1565013002/diff/100001/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc#newcode837 ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:837: base::MessageLoop::current()->PostTask( Is there no way to have the menu ...
4 years, 11 months ago (2016-01-08 23:51:56 UTC) #10
ananta
https://codereview.chromium.org/1565013002/diff/100001/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1565013002/diff/100001/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc#newcode837 ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:837: base::MessageLoop::current()->PostTask( On 2016/01/08 23:51:56, sky wrote: > Is there ...
4 years, 11 months ago (2016-01-09 00:01:15 UTC) #11
ananta
On 2016/01/09 00:01:15, ananta wrote: > https://codereview.chromium.org/1565013002/diff/100001/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc > File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): > > https://codereview.chromium.org/1565013002/diff/100001/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc#newcode837 > ...
4 years, 11 months ago (2016-01-09 01:18:25 UTC) #12
sky
On Fri, Jan 8, 2016 at 5:18 PM, <ananta@chromium.org> wrote: > On 2016/01/09 00:01:15, ananta ...
4 years, 11 months ago (2016-01-11 16:14:37 UTC) #13
ananta
Hi Scott I updated the patch to handle touch in the menu code. Details below:- ...
4 years, 11 months ago (2016-01-12 01:31:16 UTC) #18
sky
https://codereview.chromium.org/1565013002/diff/240001/ui/aura/window_event_dispatcher.cc File ui/aura/window_event_dispatcher.cc (right): https://codereview.chromium.org/1565013002/diff/240001/ui/aura/window_event_dispatcher.cc#newcode106 ui/aura/window_event_dispatcher.cc:106: event.type() == ui::ET_TOUCH_PRESSED); You'll want to update the description ...
4 years, 11 months ago (2016-01-12 21:16:42 UTC) #19
ananta
https://codereview.chromium.org/1565013002/diff/240001/ui/aura/window_event_dispatcher.cc File ui/aura/window_event_dispatcher.cc (right): https://codereview.chromium.org/1565013002/diff/240001/ui/aura/window_event_dispatcher.cc#newcode106 ui/aura/window_event_dispatcher.cc:106: event.type() == ui::ET_TOUCH_PRESSED); On 2016/01/12 21:16:42, sky wrote: > ...
4 years, 11 months ago (2016-01-13 01:21:24 UTC) #21
sky
Is it possible to add coverage to BookmarkBarViewTest? That requires us to be able to ...
4 years, 11 months ago (2016-01-13 16:31:13 UTC) #22
ananta
On 2016/01/13 16:31:13, sky wrote: > Is it possible to add coverage to BookmarkBarViewTest? That ...
4 years, 11 months ago (2016-01-13 19:03:50 UTC) #23
sky
I'm ok with tests later, although don't wait too long as I wouldn't be surprised ...
4 years, 11 months ago (2016-01-13 20:10:21 UTC) #24
sadrul
+tdresser@ to double-check the change in TouchDispositionGestureFilter (looks like a subtle change) https://codereview.chromium.org/1565013002/diff/360001/ui/aura/window_event_dispatcher.cc File ui/aura/window_event_dispatcher.cc ...
4 years, 11 months ago (2016-01-13 20:24:24 UTC) #26
ananta
https://codereview.chromium.org/1565013002/diff/340001/ui/views/controls/menu/menu_host.cc File ui/views/controls/menu/menu_host.cc (right): https://codereview.chromium.org/1565013002/diff/340001/ui/views/controls/menu/menu_host.cc#newcode41 ui/views/controls/menu/menu_host.cc:41: explicit PreMenuEventDispatchHandler(const MenuController* controller, On 2016/01/13 16:31:13, sky wrote: ...
4 years, 11 months ago (2016-01-13 21:34:48 UTC) #27
sky
https://codereview.chromium.org/1565013002/diff/380001/ui/views/controls/menu/menu_controller.cc File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1565013002/diff/380001/ui/views/controls/menu/menu_controller.cc#newcode2226 ui/views/controls/menu/menu_controller.cc:2226: // If we don't find a native window for ...
4 years, 11 months ago (2016-01-13 21:56:08 UTC) #28
ananta
https://codereview.chromium.org/1565013002/diff/380001/ui/views/controls/menu/menu_controller.cc File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1565013002/diff/380001/ui/views/controls/menu/menu_controller.cc#newcode2226 ui/views/controls/menu/menu_controller.cc:2226: // If we don't find a native window for ...
4 years, 11 months ago (2016-01-13 22:16:32 UTC) #29
sky
https://codereview.chromium.org/1565013002/diff/420001/ui/views/controls/menu/menu_controller.cc File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1565013002/diff/420001/ui/views/controls/menu/menu_controller.cc#newcode2233 ui/views/controls/menu/menu_controller.cc:2233: gfx::ScreenWin* screen_win = static_cast<gfx::ScreenWin*>(screen); You shouldn't have to use ...
4 years, 11 months ago (2016-01-13 23:32:22 UTC) #30
ananta
https://codereview.chromium.org/1565013002/diff/420001/ui/views/controls/menu/menu_controller.cc File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1565013002/diff/420001/ui/views/controls/menu/menu_controller.cc#newcode2233 ui/views/controls/menu/menu_controller.cc:2233: gfx::ScreenWin* screen_win = static_cast<gfx::ScreenWin*>(screen); On 2016/01/13 23:32:22, sky wrote: ...
4 years, 11 months ago (2016-01-14 00:22:09 UTC) #31
sky
LGTM
4 years, 11 months ago (2016-01-14 15:15:53 UTC) #32
tdresser
https://codereview.chromium.org/1565013002/diff/440001/ui/events/gesture_detection/touch_disposition_gesture_filter.cc File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right): https://codereview.chromium.org/1565013002/diff/440001/ui/events/gesture_detection/touch_disposition_gesture_filter.cc#newcode173 ui/events/gesture_detection/touch_disposition_gesture_filter.cc:173: // of 0. Add to this comment something along ...
4 years, 11 months ago (2016-01-14 15:35:40 UTC) #33
ananta
https://codereview.chromium.org/1565013002/diff/440001/ui/events/gesture_detection/touch_disposition_gesture_filter.cc File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right): https://codereview.chromium.org/1565013002/diff/440001/ui/events/gesture_detection/touch_disposition_gesture_filter.cc#newcode173 ui/events/gesture_detection/touch_disposition_gesture_filter.cc:173: // of 0. On 2016/01/14 15:35:40, tdresser wrote: > ...
4 years, 11 months ago (2016-01-14 20:09:30 UTC) #34
tdresser
touch_disposition_gesture_filter.cc LGTM with nits. https://codereview.chromium.org/1565013002/diff/440001/ui/events/gesture_detection/touch_disposition_gesture_filter.cc File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right): https://codereview.chromium.org/1565013002/diff/440001/ui/events/gesture_detection/touch_disposition_gesture_filter.cc#newcode182 ui/events/gesture_detection/touch_disposition_gesture_filter.cc:182: Head().front().unique_touch_event_id())); On 2016/01/14 20:09:30, ananta ...
4 years, 11 months ago (2016-01-14 20:14:10 UTC) #35
ananta
https://codereview.chromium.org/1565013002/diff/440001/ui/events/gesture_detection/touch_disposition_gesture_filter.cc File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right): https://codereview.chromium.org/1565013002/diff/440001/ui/events/gesture_detection/touch_disposition_gesture_filter.cc#newcode182 ui/events/gesture_detection/touch_disposition_gesture_filter.cc:182: Head().front().unique_touch_event_id())); On 2016/01/14 20:14:10, tdresser wrote: > On 2016/01/14 ...
4 years, 11 months ago (2016-01-14 20:21:15 UTC) #36
tdresser
https://codereview.chromium.org/1565013002/diff/440002/ui/events/gesture_detection/touch_disposition_gesture_filter.cc File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (left): https://codereview.chromium.org/1565013002/diff/440002/ui/events/gesture_detection/touch_disposition_gesture_filter.cc#oldcode178 ui/events/gesture_detection/touch_disposition_gesture_filter.cc:178: Head().front().unique_touch_event_id()); Sorry, I completely mistyped. Can we leave the ...
4 years, 11 months ago (2016-01-14 20:34:03 UTC) #37
ananta
https://codereview.chromium.org/1565013002/diff/440002/ui/events/gesture_detection/touch_disposition_gesture_filter.cc File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (left): https://codereview.chromium.org/1565013002/diff/440002/ui/events/gesture_detection/touch_disposition_gesture_filter.cc#oldcode178 ui/events/gesture_detection/touch_disposition_gesture_filter.cc:178: Head().front().unique_touch_event_id()); On 2016/01/14 20:34:03, tdresser wrote: > Sorry, I ...
4 years, 11 months ago (2016-01-14 20:39:10 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1565013002/490001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1565013002/490001
4 years, 11 months ago (2016-01-14 20:47:01 UTC) #41
commit-bot: I haz the power
Committed patchset #26 (id:490001)
4 years, 11 months ago (2016-01-14 21:54:50 UTC) #43
commit-bot: I haz the power
Patchset 26 (id:??) landed as https://crrev.com/575088602f6e03d99bbc280d1ec8bcede6081315 Cr-Commit-Position: refs/heads/master@{#369561}
4 years, 11 months ago (2016-01-14 22:03:55 UTC) #45
jonross
4 years, 11 months ago (2016-01-14 23:25:02 UTC) #47
Message was sent while issue was closed.
https://codereview.chromium.org/1565013002/diff/490001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/1565013002/diff/490001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller.cc:683: void
MenuController::OnTouchEvent(SubmenuView* source, ui::TouchEvent* event) {
This change prevents CrOS from being able to close Menu Widgets on touch events
that are out of bounds.

The lack of canceling like in SetSelectionOnPointerDown prevents this.

I have an upcoming patch that addresses a crash in RepostEvent. I'll provide a
fix to this there.

Powered by Google App Engine
This is Rietveld 408576698