Chromium Code Reviews
|
|
Chromium Code Reviews
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 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. |
DescriptionDon'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
Depends on Patchset: Messages
Total messages: 47 (13 generated)
ananta@chromium.org changed reviewers: + sadrul@chromium.org, sky@chromium.org
Have you verified this doesn't break dragging the tabstrip? Specifically what happens if you start a mouse drag and then touch? What if you start drag with touch and drag into a separate window? -Scott On Wed, Jan 6, 2016 at 4:01 PM, <ananta@chromium.org> wrote: > Reviewers: sky, sadrul > CL: https://codereview.chromium.org/1565013002/ > > Description: > Don't send touch events to the window with capture on Windows. This is not > how > gestures and touches work. > http://blogs.msdn.com/b/marcpe/archive/2009/06/29/let-s-talk-about-touch-part... > > 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. > > The other change is to 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 > > Base URL: https://chromium.googlesource.com/chromium/src.git@master > > Affected files (+12, -17 lines): > M ui/events/gesture_detection/touch_disposition_gesture_filter.cc > M ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc > > > Index: ui/events/gesture_detection/touch_disposition_gesture_filter.cc > diff --git a/ui/events/gesture_detection/touch_disposition_gesture_filter.cc > b/ui/events/gesture_detection/touch_disposition_gesture_filter.cc > index > 5a9e75247eea5b6519d51b928e51eef1bedbe77e..886d75a60d25e46f6e8f5161355a198006ae467c > 100644 > --- a/ui/events/gesture_detection/touch_disposition_gesture_filter.cc > +++ b/ui/events/gesture_detection/touch_disposition_gesture_filter.cc > @@ -168,14 +168,18 @@ TouchDispositionGestureFilter::OnGesturePacket( > return SUCCESS; > } > > - // Check the packet's unique_touch_event_id is valid and unique. > + // Check the packet's unique_touch_event_id is valid and unique with the > + // exception of TOUCH_TIMEOUT packets which have the > unique_touch_event_id_ > + // of 0. > if (!Tail().empty()) { > - DCHECK_NE(packet.unique_touch_event_id(), > - Tail().back().unique_touch_event_id()); > + DCHECK((packet.gesture_source() == > GestureEventDataPacket::TOUCH_TIMEOUT) > + || (packet.unique_touch_event_id() != > + Tail().back().unique_touch_event_id())); > } > if (!Head().empty()) { > - DCHECK_NE(packet.unique_touch_event_id(), > - Head().front().unique_touch_event_id()); > + DCHECK((packet.gesture_source() == > GestureEventDataPacket::TOUCH_TIMEOUT) > + || (packet.unique_touch_event_id() != > + Head().front().unique_touch_event_id())); > } > > Tail().push(packet); > Index: ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc > diff --git a/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc > b/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc > index > 48368f962f2f3433b76c5ad20746ad28f135a481..20057c3f8e22759549d1b0de3c7b3feb0d40dcb1 > 100644 > --- a/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc > +++ b/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc > @@ -817,23 +817,14 @@ void DesktopWindowTreeHostWin::HandleTouchEvent( > if (!GetWidget()->GetNativeView()) > return; > > - // Currently we assume the window that has capture gets touch events too. > + // If we have a window with capture, then ensure that capture is > released. > aura::WindowTreeHost* host = > aura::WindowTreeHost::GetForAcceleratedWidget(GetCapture()); > if (host) { > DesktopWindowTreeHostWin* target = > host->window()->GetProperty(kDesktopWindowTreeHostKey); > - if (target && target->HasCapture() && target != this) { > - POINT target_location(event.location().ToPOINT()); > - ClientToScreen(GetHWND(), &target_location); > - ScreenToClient(target->GetHWND(), &target_location); > - ui::TouchEvent target_event(event, static_cast<View*>(NULL), > - static_cast<View*>(NULL)); > - target_event.set_location(gfx::Point(target_location)); > - target_event.set_root_location(target_event.location()); > - target->SendEventToProcessor(&target_event); > - return; > - } > + if (target && target->HasCapture() && target != this) > + target->ReleaseCapture(); > } > SendEventToProcessor(const_cast<ui::TouchEvent*>(&event)); > } > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Don't send touch events to the window with capture on Windows. This is not how gestures and touches work. http://blogs.msdn.com/b/marcpe/archive/2009/06/29/let-s-talk-about-touch-part... 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. The other change is to 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 ========== to ========== 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. The other change is to 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 ==========
On 2016/01/07 00:42:05, sky wrote: > Have you verified this doesn't break dragging the tabstrip? > Specifically what happens if you start a mouse drag and then touch? > What if you start drag with touch and drag into a separate window? > > -Scott > > On Wed, Jan 6, 2016 at 4:01 PM, <mailto:ananta@chromium.org> wrote: > > Reviewers: sky, sadrul > > CL: https://codereview.chromium.org/1565013002/ > > > > Description: > > Don't send touch events to the window with capture on Windows. This is not > > how > > gestures and touches work. > > > http://blogs.msdn.com/b/marcpe/archive/2009/06/29/let-s-talk-about-touch-part... > > > > 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. > > > > The other change is to 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 > > > > Base URL: https://chromium.googlesource.com/chromium/src.git@master > > > > Affected files (+12, -17 lines): > > M ui/events/gesture_detection/touch_disposition_gesture_filter.cc > > M ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc > > > > > > Index: ui/events/gesture_detection/touch_disposition_gesture_filter.cc > > diff --git a/ui/events/gesture_detection/touch_disposition_gesture_filter.cc > > b/ui/events/gesture_detection/touch_disposition_gesture_filter.cc > > index > > > 5a9e75247eea5b6519d51b928e51eef1bedbe77e..886d75a60d25e46f6e8f5161355a198006ae467c > > 100644 > > --- a/ui/events/gesture_detection/touch_disposition_gesture_filter.cc > > +++ b/ui/events/gesture_detection/touch_disposition_gesture_filter.cc > > @@ -168,14 +168,18 @@ TouchDispositionGestureFilter::OnGesturePacket( > > return SUCCESS; > > } > > > > - // Check the packet's unique_touch_event_id is valid and unique. > > + // Check the packet's unique_touch_event_id is valid and unique with the > > + // exception of TOUCH_TIMEOUT packets which have the > > unique_touch_event_id_ > > + // of 0. > > if (!Tail().empty()) { > > - DCHECK_NE(packet.unique_touch_event_id(), > > - Tail().back().unique_touch_event_id()); > > + DCHECK((packet.gesture_source() == > > GestureEventDataPacket::TOUCH_TIMEOUT) > > + || (packet.unique_touch_event_id() != > > + Tail().back().unique_touch_event_id())); > > } > > if (!Head().empty()) { > > - DCHECK_NE(packet.unique_touch_event_id(), > > - Head().front().unique_touch_event_id()); > > + DCHECK((packet.gesture_source() == > > GestureEventDataPacket::TOUCH_TIMEOUT) > > + || (packet.unique_touch_event_id() != > > + Head().front().unique_touch_event_id())); > > } > > > > Tail().push(packet); > > Index: ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc > > diff --git a/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc > > b/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc > > index > > > 48368f962f2f3433b76c5ad20746ad28f135a481..20057c3f8e22759549d1b0de3c7b3feb0d40dcb1 > > 100644 > > --- a/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc > > +++ b/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc > > @@ -817,23 +817,14 @@ void DesktopWindowTreeHostWin::HandleTouchEvent( > > if (!GetWidget()->GetNativeView()) > > return; > > > > - // Currently we assume the window that has capture gets touch events too. > > + // If we have a window with capture, then ensure that capture is > > released. > > aura::WindowTreeHost* host = > > aura::WindowTreeHost::GetForAcceleratedWidget(GetCapture()); > > if (host) { > > DesktopWindowTreeHostWin* target = > > host->window()->GetProperty(kDesktopWindowTreeHostKey); > > - if (target && target->HasCapture() && target != this) { > > - POINT target_location(event.location().ToPOINT()); > > - ClientToScreen(GetHWND(), &target_location); > > - ScreenToClient(target->GetHWND(), &target_location); > > - ui::TouchEvent target_event(event, static_cast<View*>(NULL), > > - static_cast<View*>(NULL)); > > - target_event.set_location(gfx::Point(target_location)); > > - target_event.set_root_location(target_event.location()); > > - target->SendEventToProcessor(&target_event); > > - return; > > - } > > + if (target && target->HasCapture() && target != this) > > + target->ReleaseCapture(); > > } > > SendEventToProcessor(const_cast<ui::TouchEvent*>(&event)); > > } > > > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > Thanks for bringing up the issue with tab drag. As per our discussion, I tried the event target route to pre handle the touch event. However that fails as we have special handling for touch events where in they are intercepted by the gesture handler and not sent further down the chain. As a workaround/hack I set a window property on the top level widget on windows when you are in the context of a menu which is cleared when the menu goes away. We check this property in the DesktopWindowTreeHostWin::HandleTouchEvent function to release capture on touch down and dispatch the message to the correct window.
https://codereview.chromium.org/1565013002/diff/60001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1565013002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:830: if ((event.type() == ui::ET_TOUCH_PRESSED) && What happens when you touch on a window in another app?
https://codereview.chromium.org/1565013002/diff/60001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1565013002/diff/60001/ui/views/widget/desktop... 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 wrote: > What happens when you touch on a window in another app? If you open the context menu in chrome, and touch elsewhere, you get a cancel mode message which dismisses it. A tab dragging operation is aborted on similar lines. This patch does not affect that behavior.
https://codereview.chromium.org/1565013002/diff/60001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1565013002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:848: } Correct me if I'm wrong, but if we have a menu showing and you change this code, won't we be sending the event from the nested message loop run by menus? If so, that's bad and will most definitely cause problems.
https://codereview.chromium.org/1565013002/diff/60001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1565013002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:848: } On 2016/01/08 16:05:12, sky wrote: > Correct me if I'm wrong, but if we have a menu showing and you change this code, > won't we be sending the event from the nested message loop run by menus? If so, > that's bad and will most definitely cause problems. Yeah. Changed to post the event via a task
https://codereview.chromium.org/1565013002/diff/100001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1565013002/diff/100001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:837: base::MessageLoop::current()->PostTask( Is there no way to have the menu code do the posting so that we don't need kReleaseCaptureOnTouchOutside?
https://codereview.chromium.org/1565013002/diff/100001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1565013002/diff/100001/ui/views/widget/deskto... 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 no way to have the menu code do the posting so that we don't need > kReleaseCaptureOnTouchOutside? The touch events are handled by the gesture recognizer in the PredispatchEvent phase. It short circuits them there and ensures that we will only see gestures in chrome. The code is here https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window_eve... and that basically forwards the event to the gesture recognizer which short circuits it
On 2016/01/09 00:01:15, ananta wrote: > https://codereview.chromium.org/1565013002/diff/100001/ui/views/widget/deskto... > File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): > > https://codereview.chromium.org/1565013002/diff/100001/ui/views/widget/deskto... > 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 no way to have the menu code do the posting so that we don't need > > kReleaseCaptureOnTouchOutside? > > The touch events are handled by the gesture recognizer in the PredispatchEvent > phase. It short circuits them there and ensures that we will only see gestures > in chrome. > > The code is here > https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window_eve... > and that basically forwards the event to the gesture recognizer which short > circuits it Scratch the last reply. The event dispatching occurs in two phases. 1. HWND to the aura Window hierarchy 2. Aura to views. The touch events don't make it through the second part. We could possibly set a pre target handler on the underlying window in the menu host and handle touch there. One point to note is that the touch coming in has its location transformed to the coordinate space of the menu window. We would have to somehow figure out who the touch was targeted to and then repost it. It may not always be the parent widget. Imagine the menu popping up on one chrome window and then touch on the other chrome window. I updated the patch to set the window property on the menu host widget window instead of the parent window. This now works across windows.
On Fri, Jan 8, 2016 at 5:18 PM, <ananta@chromium.org> wrote: > On 2016/01/09 00:01:15, ananta wrote: > > https://codereview.chromium.org/1565013002/diff/100001/ui/views/widget/deskto... >> >> File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): > > > > https://codereview.chromium.org/1565013002/diff/100001/ui/views/widget/deskto... >> >> 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 no way to have the menu code do the posting so that we don't >> > need >> > kReleaseCaptureOnTouchOutside? > > >> The touch events are handled by the gesture recognizer in the >> PredispatchEvent >> phase. It short circuits them there and ensures that we will only see >> gestures >> in chrome. > > >> The code is here > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window_eve... >> >> and that basically forwards the event to the gesture recognizer which >> short >> circuits it > > > Scratch the last reply. The event dispatching occurs in two phases. > 1. HWND to the aura Window hierarchy > 2. Aura to views. > > The touch events don't make it through the second part. We could possibly > set a > pre target handler on the underlying window in the menu host and handle > touch > there. One point to note is that the touch coming in has its location > transformed to the coordinate space of the menu window. We would have to > somehow > figure out who the touch was targeted to and then repost it. MenuController::RepostEvent does this already, right? -Scott > It may not always be the parent widget. Imagine the menu popping up on one > chrome window and then touch on the other chrome window. > > I updated the patch to set the window property on the menu host widget > window > instead of the parent window. This now works across windows. > > > > https://codereview.chromium.org/1565013002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== 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. The other change is to 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 ========== to ========== 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 are now template functions with the template argument being the event type. 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 ==========
Description was changed from ========== 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 are now template functions with the template argument being the event type. 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 ========== to ========== 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 are now template functions with the template argument being the event type. 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 ==========
Description was changed from ========== 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 are now template functions with the template argument being the event type. 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 ========== to ========== 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 are now template functions with the template argument being the event type. 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 ==========
Description was changed from ========== 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 are now template functions with the template argument being the event type. 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 ========== to ========== 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 are now template functions with the template argument being the event type. 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 ==========
Hi Scott I updated the patch to handle touch in the menu code. Details below:- 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. Changes in this patch:- 1. The MenuController::RepostEvent, MenuController::SetSelectionOnPointerDown, MenuMessageLoop::RepostEventToWindow functions are now template functions with the template argument being the event type. This is needed to ensure that we don't lose information while downcasting the event. 2. 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.
https://codereview.chromium.org/1565013002/diff/240001/ui/aura/window_event_d... File ui/aura/window_event_dispatcher.cc (right): https://codereview.chromium.org/1565013002/diff/240001/ui/aura/window_event_d... ui/aura/window_event_dispatcher.cc:106: event.type() == ui::ET_TOUCH_PRESSED); You'll want to update the description in the .h for this, and add test coverage here. https://codereview.chromium.org/1565013002/diff/240001/ui/aura/window_event_d... ui/aura/window_event_dispatcher.cc:120: DispatchDetails details = OnEventFromSource( Doesn't this process immediatley right here? Don't you need to do something like ET_MOUSE_PRESSED and PostNonNestableTask? https://codereview.chromium.org/1565013002/diff/240001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1565013002/diff/240001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:686: RepostEvent(source, *event); If you repostevent don't you need to stop propagation of the touch event? https://codereview.chromium.org/1565013002/diff/240001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:2294: base::Bind(&MenuMessageLoop::RepostEventToWindow<EventType>, RepostEventtoWindow() should post after a delay. You shouldn't have to use PostTask here. https://codereview.chromium.org/1565013002/diff/240001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.h (right): https://codereview.chromium.org/1565013002/diff/240001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.h:511: template<class EventType> I'm not seeing why you need the template args.
Description was changed from ========== 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 are now template functions with the template argument being the event type. 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 ========== to ========== 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 ==========
https://codereview.chromium.org/1565013002/diff/240001/ui/aura/window_event_d... File ui/aura/window_event_dispatcher.cc (right): https://codereview.chromium.org/1565013002/diff/240001/ui/aura/window_event_d... ui/aura/window_event_dispatcher.cc:106: event.type() == ui::ET_TOUCH_PRESSED); On 2016/01/12 21:16:42, sky wrote: > You'll want to update the description in the .h for this, and add test coverage > here. Done. https://codereview.chromium.org/1565013002/diff/240001/ui/aura/window_event_d... ui/aura/window_event_dispatcher.cc:120: DispatchDetails details = OnEventFromSource( On 2016/01/12 21:16:42, sky wrote: > Doesn't this process immediatley right here? Don't you need to do something like > ET_MOUSE_PRESSED and PostNonNestableTask? Done. https://codereview.chromium.org/1565013002/diff/240001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1565013002/diff/240001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:686: RepostEvent(source, *event); On 2016/01/12 21:16:42, sky wrote: > If you repostevent don't you need to stop propagation of the touch event? Done. https://codereview.chromium.org/1565013002/diff/240001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:2294: base::Bind(&MenuMessageLoop::RepostEventToWindow<EventType>, On 2016/01/12 21:16:42, sky wrote: > RepostEventtoWindow() should post after a delay. You shouldn't have to use > PostTask here. Done. https://codereview.chromium.org/1565013002/diff/240001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.h (right): https://codereview.chromium.org/1565013002/diff/240001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.h:511: template<class EventType> On 2016/01/12 21:16:42, sky wrote: > I'm not seeing why you need the template args. Replaced with LocatedEvent* here and other places.
Is it possible to add coverage to BookmarkBarViewTest? That requires us to be able to generate system touch events. I'm not sure that is possible. Is it? https://codereview.chromium.org/1565013002/diff/340001/ui/views/controls/menu... File ui/views/controls/menu/menu_host.cc (right): https://codereview.chromium.org/1565013002/diff/340001/ui/views/controls/menu... ui/views/controls/menu/menu_host.cc:41: explicit PreMenuEventDispatchHandler(const MenuController* controller, nit: no explict https://codereview.chromium.org/1565013002/diff/340001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1565013002/diff/340001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:99: // All HWND's we create should have WindowTreeHost instances associated with Is this change related?
On 2016/01/13 16:31:13, sky wrote: > Is it possible to add coverage to BookmarkBarViewTest? That requires us to be > able to generate system touch events. I'm not sure that is possible. Is it? > > https://codereview.chromium.org/1565013002/diff/340001/ui/views/controls/menu... > File ui/views/controls/menu/menu_host.cc (right): > > https://codereview.chromium.org/1565013002/diff/340001/ui/views/controls/menu... > ui/views/controls/menu/menu_host.cc:41: explicit > PreMenuEventDispatchHandler(const MenuController* controller, > nit: no explict > > https://codereview.chromium.org/1565013002/diff/340001/ui/views/widget/deskto... > File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): > > https://codereview.chromium.org/1565013002/diff/340001/ui/views/widget/deskto... > ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:99: // All HWND's > we create should have WindowTreeHost instances associated with > Is this change related? I think there are API's which can generate touch events. If it is ok, we can do that in a subsequent patch?
I'm ok with tests later, although don't wait too long as I wouldn't be surprised if you see different behavior on different OS versions which you may need to deal with. My only remaining comment is here: https://codereview.chromium.org/1565013002/diff/360001/ui/views/widget/deskto... -Scott On Wed, Jan 13, 2016 at 11:03 AM, <ananta@chromium.org> wrote: > On 2016/01/13 16:31:13, sky wrote: >> >> Is it possible to add coverage to BookmarkBarViewTest? That requires us to >> be >> able to generate system touch events. I'm not sure that is possible. Is >> it? > > > > https://codereview.chromium.org/1565013002/diff/340001/ui/views/controls/menu... >> >> File ui/views/controls/menu/menu_host.cc (right): > > > > https://codereview.chromium.org/1565013002/diff/340001/ui/views/controls/menu... >> >> ui/views/controls/menu/menu_host.cc:41: explicit >> PreMenuEventDispatchHandler(const MenuController* controller, >> nit: no explict > > > > https://codereview.chromium.org/1565013002/diff/340001/ui/views/widget/deskto... >> >> File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): > > > > https://codereview.chromium.org/1565013002/diff/340001/ui/views/widget/deskto... >> >> ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:99: // All >> HWND's >> we create should have WindowTreeHost instances associated with >> Is this change related? > > > I think there are API's which can generate touch events. If it is ok, we can > do > that in a subsequent patch? > > > https://codereview.chromium.org/1565013002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
sadrul@chromium.org changed reviewers: + tdresser@chromium.org
+tdresser@ to double-check the change in TouchDispositionGestureFilter (looks like a subtle change) https://codereview.chromium.org/1565013002/diff/360001/ui/aura/window_event_d... File ui/aura/window_event_dispatcher.cc (right): https://codereview.chromium.org/1565013002/diff/360001/ui/aura/window_event_d... ui/aura/window_event_dispatcher.cc:131: } Can this be like: if (event->type() == ui::ET_MOUSE_PRESSED) { ... } else if (event->type() == ui::ET_TOUCH_PRESSED) { ... } else { DCHECK_EQ(ui::ET_GESTURE_TAP_DOWN, event->type()); ... } if (held_repostable_event_) { ... post task } https://codereview.chromium.org/1565013002/diff/360001/ui/aura/window_event_d... ui/aura/window_event_dispatcher.cc:679: event.reset(held_repostable_event_.release()); Use std::move instead?
https://codereview.chromium.org/1565013002/diff/340001/ui/views/controls/menu... File ui/views/controls/menu/menu_host.cc (right): https://codereview.chromium.org/1565013002/diff/340001/ui/views/controls/menu... ui/views/controls/menu/menu_host.cc:41: explicit PreMenuEventDispatchHandler(const MenuController* controller, On 2016/01/13 16:31:13, sky wrote: > nit: no explict Done. https://codereview.chromium.org/1565013002/diff/340001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1565013002/diff/340001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:99: // All HWND's we create should have WindowTreeHost instances associated with On 2016/01/13 16:31:13, sky wrote: > Is this change related? All HWND's created by views have WindowTreeHost instances. However the content HWND (LegacyRenderWidgetHostHWND) does not. If we get that HWND we need to go up to its parent. https://codereview.chromium.org/1565013002/diff/360001/ui/aura/window_event_d... File ui/aura/window_event_dispatcher.cc (right): https://codereview.chromium.org/1565013002/diff/360001/ui/aura/window_event_d... ui/aura/window_event_dispatcher.cc:131: } On 2016/01/13 20:24:24, sadrul wrote: > Can this be like: > > if (event->type() == ui::ET_MOUSE_PRESSED) { > ... > } else if (event->type() == ui::ET_TOUCH_PRESSED) { > ... > } else { > DCHECK_EQ(ui::ET_GESTURE_TAP_DOWN, event->type()); > ... > } > > if (held_repostable_event_) { > ... post task > } Done. https://codereview.chromium.org/1565013002/diff/360001/ui/aura/window_event_d... ui/aura/window_event_dispatcher.cc:679: event.reset(held_repostable_event_.release()); On 2016/01/13 20:24:24, sadrul wrote: > Use std::move instead? done
https://codereview.chromium.org/1565013002/diff/380001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1565013002/diff/380001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:2226: // If we don't find a native window for the HWND at the current location, Shouldn't this code be in the if starting on 2244? And can't it be merged with 2247, where it gets the target_window? https://codereview.chromium.org/1565013002/diff/380001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1565013002/diff/380001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:99: // All HWND's we create should have WindowTreeHost instances associated with remove comment (at least last sentence, first two are fine).
https://codereview.chromium.org/1565013002/diff/380001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1565013002/diff/380001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:2226: // If we don't find a native window for the HWND at the current location, On 2016/01/13 21:56:08, sky wrote: > Shouldn't this code be in the if starting on 2244? And can't it be merged with > 2247, where it gets the target_window? Yes. unified this code. https://codereview.chromium.org/1565013002/diff/380001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1565013002/diff/380001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:99: // All HWND's we create should have WindowTreeHost instances associated with On 2016/01/13 21:56:08, sky wrote: > remove comment (at least last sentence, first two are fine). Done.
https://codereview.chromium.org/1565013002/diff/420001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1565013002/diff/420001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:2233: gfx::ScreenWin* screen_win = static_cast<gfx::ScreenWin*>(screen); You shouldn't have to use static_cast here. Instead use aura::WindowTreeHost::GetForAcceleratedWidget(hwnd).
https://codereview.chromium.org/1565013002/diff/420001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1565013002/diff/420001/ui/views/controls/menu... 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: > You shouldn't have to use static_cast here. Instead use > aura::WindowTreeHost::GetForAcceleratedWidget(hwnd). Done. Both approaches seem ugly. The behavior with aura::WindowTreeHost::GetForAcceleratedWidget is slightly different as in we get the roowindow for the point. Eventually the events make it through to the correct window. In this case RWHVA.
LGTM
https://codereview.chromium.org/1565013002/diff/440001/ui/events/gesture_dete... File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right): https://codereview.chromium.org/1565013002/diff/440001/ui/events/gesture_dete... ui/events/gesture_detection/touch_disposition_gesture_filter.cc:173: // of 0. Add to this comment something along the lines of: |TOUCH_TIMEOUT| packets don't wait for an ack, they are dispatched as soon as they reach the head of the queue, in |SendAckedEvents|. https://codereview.chromium.org/1565013002/diff/440001/ui/events/gesture_dete... ui/events/gesture_detection/touch_disposition_gesture_filter.cc:182: Head().front().unique_touch_event_id())); I don't think we should ever reach this code with a TOUCH_TIMEOUT packet at the front of the queue. We should have already dispatched the TOUCH_TIMEOUT packet in the most recent SendAckedEvents. Did you ever see this trip in practice?
https://codereview.chromium.org/1565013002/diff/440001/ui/events/gesture_dete... File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right): https://codereview.chromium.org/1565013002/diff/440001/ui/events/gesture_dete... ui/events/gesture_detection/touch_disposition_gesture_filter.cc:173: // of 0. On 2016/01/14 15:35:40, tdresser wrote: > Add to this comment something along the lines of: > |TOUCH_TIMEOUT| packets don't wait for an ack, they are dispatched as soon as > they reach the head of the queue, in |SendAckedEvents|. Done. https://codereview.chromium.org/1565013002/diff/440001/ui/events/gesture_dete... ui/events/gesture_detection/touch_disposition_gesture_filter.cc:182: Head().front().unique_touch_event_id())); On 2016/01/14 15:35:40, tdresser wrote: > I don't think we should ever reach this code with a TOUCH_TIMEOUT packet at the > front of the queue. We should have already dispatched the TOUCH_TIMEOUT packet > in the most recent SendAckedEvents. > > Did you ever see this trip in practice? No. I just changed the DCHECK for consistency. The first one does fire though. Hence that change is needed.
touch_disposition_gesture_filter.cc LGTM with nits. https://codereview.chromium.org/1565013002/diff/440001/ui/events/gesture_dete... File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right): https://codereview.chromium.org/1565013002/diff/440001/ui/events/gesture_dete... ui/events/gesture_detection/touch_disposition_gesture_filter.cc:182: Head().front().unique_touch_event_id())); On 2016/01/14 20:09:30, ananta wrote: > On 2016/01/14 15:35:40, tdresser wrote: > > I don't think we should ever reach this code with a TOUCH_TIMEOUT packet at > the > > front of the queue. We should have already dispatched the TOUCH_TIMEOUT packet > > in the most recent SendAckedEvents. > > > > Did you ever see this trip in practice? > > No. I just changed the DCHECK for consistency. The first one does fire though. > Hence that change is needed. Yeah, it makes sense that the first would fire. Let's remove the second DCHECK, and maybe add a comment indicating that TOUCH_TIMEOUT packets should never read the head of the queue.
https://codereview.chromium.org/1565013002/diff/440001/ui/events/gesture_dete... File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right): https://codereview.chromium.org/1565013002/diff/440001/ui/events/gesture_dete... 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 20:09:30, ananta wrote: > > On 2016/01/14 15:35:40, tdresser wrote: > > > I don't think we should ever reach this code with a TOUCH_TIMEOUT packet at > > the > > > front of the queue. We should have already dispatched the TOUCH_TIMEOUT > packet > > > in the most recent SendAckedEvents. > > > > > > Did you ever see this trip in practice? > > > > No. I just changed the DCHECK for consistency. The first one does fire though. > > Hence that change is needed. > > Yeah, it makes sense that the first would fire. Let's remove the second DCHECK, > and maybe add a comment indicating that TOUCH_TIMEOUT packets should never read > the head of the queue. Done.
https://codereview.chromium.org/1565013002/diff/440002/ui/events/gesture_dete... File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (left): https://codereview.chromium.org/1565013002/diff/440002/ui/events/gesture_dete... ui/events/gesture_detection/touch_disposition_gesture_filter.cc:178: Head().front().unique_touch_event_id()); Sorry, I completely mistyped. Can we leave the second DCHECK in place as it was before? Sorry!
https://codereview.chromium.org/1565013002/diff/440002/ui/events/gesture_dete... File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (left): https://codereview.chromium.org/1565013002/diff/440002/ui/events/gesture_dete... 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 completely mistyped. > > Can we leave the second DCHECK in place as it was before? > > Sorry! Done.
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/1565013002/#ps490001 (title: "Restore DCHECK")
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
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #26 (id:490001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 26 (id:??) landed as https://crrev.com/575088602f6e03d99bbc280d1ec8bcede6081315 Cr-Commit-Position: refs/heads/master@{#369561}
Message was sent while issue was closed.
jonross@chromium.org changed reviewers: + jonross@chromium.org
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. |
