Bug 35431

Summary: The element that a touchevent is dispatched to and the targetTouches list aren't always right
Product: WebKit Reporter: Ben Murdoch <benm>
Component: WebCore Misc.Assignee: Ben Murdoch <benm>
Status: RESOLVED FIXED    
Severity: Normal CC: android-webkit-unforking, hausmann, kenneth
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 32485    
Attachments:
Description Flags
Proposed patch kenneth: review+

Ben Murdoch
Reported 2010-02-26 07:40:23 PST
Currently, we cache the target element that touch events are dispatched to in EventhHandler::handleTouchEvent() in the m_touchEventTarget member. This cached target is only updated to a new element when the first element in the PlatformTouchEvent's list of points is a TouchPressed event. This does not match observed behavior on iPhone and Android. To better match the iPhone and Android behavior, I propose that we dispatch the event to the target element of the touch that caused the event to be fired in the first place, i.e. the target of the touch in the changedTouches list. Also, this element should be the 'target' that the 'touches' list is filtered over to create the targetTouches list. I have a patch and layout test update to follow.
Attachments
Proposed patch (12.50 KB, patch)
2010-02-26 07:58 PST, Ben Murdoch 		kenneth: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Ben Murdoch
Comment 1 2010-02-26 07:58:57 PST
Created attachment 49584 [details] Proposed patch
Kenneth Rohde Christiansen
Comment 2 2010-02-26 08:15:38 PST
Comment on attachment 49584 [details] Proposed patch > + The element that a touchevent is dispatched to isn't always right > + https://bugs.webkit.org/show_bug.cgi?id=35431 > + > + The element that touch events are dispatched on is not always the > + correct one, as the cached m_touchEventTarget member is only updated > + when the first element of the PlatformTouchEvent touch list is in the > + TouchPressed state. > + > + This patch changes this behavior to dispatch the event to the target > + of the touch that caused the event to be generated and eliminates the > + m_touchEventTarget in favour of using the touch target hashmap. It also Thus no more caching, right? > > + if (cancelTouches->length() > 0) { > + // We dispatch the event to the target of the touch that caused this touch event to be generated, i.e. > + // we take it from the list that will be used as the changedTouches property of the event. > + // The choice to use the touch at index 0 guaratees that there is a target (as we checked the length guarantees, spelling error. > + // above). In the case that there are multiple touches in what becomes the changedTouches list, it is > + // difficult to say how we should prioritise touches and as such, item 0 is an arbitrary choice. This is the same behaviour you noticed on iphone/android?
Ben Murdoch
Comment 3 2010-02-26 08:32:46 PST
(In reply to comment #2) Hey Kenneth, thanks for taking a look at the patch! > (From update of attachment 49584 [details]) > > > + The element that a touchevent is dispatched to isn't always right > > + https://bugs.webkit.org/show_bug.cgi?id=35431 > > + > > + The element that touch events are dispatched on is not always the > > + correct one, as the cached m_touchEventTarget member is only updated > > + when the first element of the PlatformTouchEvent touch list is in the > > + TouchPressed state. > > + > > + This patch changes this behavior to dispatch the event to the target > > + of the touch that caused the event to be generated and eliminates the > > + m_touchEventTarget in favour of using the touch target hashmap. It also > > Thus no more caching, right? Well, the appropriate target is cached inside the hashmap, so we no longer have a need for this extra m_touchEventTarget cached value. > > > > > > + if (cancelTouches->length() > 0) { > > + // We dispatch the event to the target of the touch that caused this touch event to be generated, i.e. > > + // we take it from the list that will be used as the changedTouches property of the event. > > + // The choice to use the touch at index 0 guaratees that there is a target (as we checked the length > > guarantees, spelling error. Thanks, well spotted. Will fix. > > > + // above). In the case that there are multiple touches in what becomes the changedTouches list, it is > > + // difficult to say how we should prioritise touches and as such, item 0 is an arbitrary choice. > > This is the same behaviour you noticed on iphone/android? Not exactly. It's particularly difficult to get into the state where you have two elements in the changedTouches list, as you must press two fingers down at *precisely* the same time. I think it's a pretty far out edge case and in that scenario acceptable to just take the first element in the list (which we know exists because we're only in this block if the length() is > 0.
Kenneth Rohde Christiansen
Comment 4 2010-02-26 08:35:36 PST
> Not exactly. It's particularly difficult to get into the state where you have > two elements in the changedTouches list, as you must press two fingers down at > *precisely* the same time. I think it's a pretty far out edge case and in that > scenario acceptable to just take the first element in the list (which we know > exists because we're only in this block if the length() is > 0. Sounds good to me! Please fix that spelling issue before landing.
Ben Murdoch
Comment 5 2010-02-26 08:52:43 PST
Thanks for the review! Fixed typo and committed revision 55287.
Note You need to log in before you can comment on or make changes to this bug.