Bug 35431 – The element that a touchevent is dispatched to and the targetTouches list aren't always right

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Ben Murdoch]

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.

[Ben Murdoch]

Created attachment 49584 [details]
Proposed patch

[Kenneth Rohde Christiansen]

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]

(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]

> 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]

Thanks for the review!

Fixed typo and committed revision 55287.