Bug 34585 – [Qt] The target element of a Touch should be the target where that touch originated, not where it is now.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Ben Murdoch]

To match the current behavior on iPhone and Android, the target of a touch point within a touch event should be the same target as when the touch point was first pressed. This matches the behavior Android and iPhone currently exhibit.

For example, see http://www.ksasq.com/touch/touch-target.html which shows 3 DIVs and the target of the current touch points. On Android and iPhone, the target does not change with touchmove events, even if the touch moves into a different DIV.

I have a patch to the handleTouchEvent function to fix this, and will send a new patch to this bug to also update the layout tests to match the new behavior once 34476 lands. Until then, I'd really appreciate any feedback on the code.

Whilst developing the patch, I noticed a bug in the Qt event sender: touch points where not being assigned a unique id. A patch to correct that is also included.

Thanks, Ben

[Ben Murdoch]

Created attachment 48136 [details]
First draft of code to fix the problem. Layout tests to follow later.

[Ben Murdoch]

Created attachment 49378 [details]
Patch and layout test changes.

[Ben Murdoch]

Created attachment 49409 [details]
Proposed patch and test.

Sorry for the review request/patch churn on this bug... this patch is the correct one to review :)

Thanks!

[Ben Murdoch]

Tagging Qt as there are some small edits to Qt specific files.

[Kenneth Rohde Christiansen]

Comment on attachment 49409 [details]
Proposed patch and test.

Couldn't you wrap those ChangeLog lines? Most people do that. It is very hard to read how it is now.

[Ben Murdoch]

(In reply to comment #5)
> (From update of attachment 49409 [details])
> Couldn't you wrap those ChangeLog lines? Most people do that. It is very hard
> to read how it is now.

Sure thing. I'll send a new patch.

[Ben Murdoch]

Created attachment 49412 [details]
Patch (line wrapped Changelogs)

[Kenneth Rohde Christiansen]

Comment on attachment 49412 [details]
Patch (line wrapped Changelogs)


> +        if (point.state() == PlatformTouchPoint::TouchPressed) {
> +            m_originatingTouchPointTargets.set(touchPointTargetKey, target);
> +            touchTarget = target;
> +        } else if (point.state() == PlatformTouchPoint::TouchReleased || point.state() == PlatformTouchPoint::TouchCancelled)
> +            touchTarget = m_originatingTouchPointTargets.take(touchPointTargetKey).get();
> +        else
> +            touchTarget = m_originatingTouchPointTargets.get(touchPointTargetKey).get();

When do you get in this last else? I would add a comment (like in the changelog) above the point.state() == PlatformTouchPoint::TouchReleased, that the target is the original point.


> -    m_id = point.id();
> +    // The QTouchEvent::TouchPoint API states that ids will be >= 0.
> +    m_id = static_cast<unsigned>(point.id());

Didn't you just change id to be unsigned? why is the cast then needed

> +
>  #if QT_VERSION >= QT_VERSION_CHECK(4, 6, 0)
> -    int id = m_touchPoints.count();
> +    int index = m_touchPoints.count();
> +    int id = m_touchPoints.isEmpty() ? 0 : m_touchPoints.last().id() + 1;

It is not so obvious what you are doing here, could you add a small comment?

The rest looks fine.

[Ben Murdoch]

(In reply to comment #8)

Hi Kenneth, thanks for the review! Comments inline.

> (From update of attachment 49412 [details])
> 
> > +        if (point.state() == PlatformTouchPoint::TouchPressed) {
> > +            m_originatingTouchPointTargets.set(touchPointTargetKey, target);
> > +            touchTarget = target;
> > +        } else if (point.state() == PlatformTouchPoint::TouchReleased || point.state() == PlatformTouchPoint::TouchCancelled)
> > +            touchTarget = m_originatingTouchPointTargets.take(touchPointTargetKey).get();
> > +        else
> > +            touchTarget = m_originatingTouchPointTargets.get(touchPointTargetKey).get();
> 
> When do you get in this last else? I would add a comment (like in the
> changelog) above the point.state() == PlatformTouchPoint::TouchReleased, that
> the target is the original point.

The last else is for when the touch is a move event, i.e. an update to a touch point that has already been pressed down but not yet released. Then we just want to pull the original target out of the map. I can add the comment like you suggest.

> 
> > -    m_id = point.id();
> > +    // The QTouchEvent::TouchPoint API states that ids will be >= 0.
> > +    m_id = static_cast<unsigned>(point.id());
> 
> Didn't you just change id to be unsigned? why is the cast then needed

The Qt API returns a (signed) int from the id() function, I just wanted to avoid any compiler warnings and make it explicit that the id must be unsigned.

> 
> > +
> >  #if QT_VERSION >= QT_VERSION_CHECK(4, 6, 0)
> > -    int id = m_touchPoints.count();
> > +    int index = m_touchPoints.count();
> > +    int id = m_touchPoints.isEmpty() ? 0 : m_touchPoints.last().id() + 1;
> 
> It is not so obvious what you are doing here, could you add a small comment?
> 

Sure. Basically, with the old code, touches were not always being assigned unique ids. For example, imagine the case when you press two fingers down, first finger would get id 0, second would get id 1. Then lift the first finger and place another down - it would get assigned id 1 and then both the pressed touches have the same id.

My change is just to ensure that we assign a unique ID to each new touch press. When we first start or after all touches are released, we start again assigning id 0.

> The rest looks fine.

Great! I can upload a new patch tomorrow or if you are happy with my explanations I can add the comments on commit, whichever you prefer.

Thanks again for the review!

Ben

[Kenneth Rohde Christiansen]

> The last else is for when the touch is a move event, i.e. an update to a touch
> point that has already been pressed down but not yet released. Then we just
> want to pull the original target out of the map. I can add the comment like you
> suggest.

great, I would like that.

> The Qt API returns a (signed) int from the id() function, I just wanted to
> avoid any compiler warnings and make it explicit that the id must be unsigned.

ok, got it.


> Sure. Basically, with the old code, touches were not always being assigned
> unique ids. For example, imagine the case when you press two fingers down,
> first finger would get id 0, second would get id 1. Then lift the first finger
> and place another down - it would get assigned id 1 and then both the pressed
> touches have the same id.
> 
> My change is just to ensure that we assign a unique ID to each new touch press.
> When we first start or after all touches are released, we start again assigning
> id 0.

Ah, sounds good. Would be good with a comment like the above (maybe a bit shorter :-))

> Great! I can upload a new patch tomorrow or if you are happy with my
> explanations I can add the comments on commit, whichever you prefer.

Sounds good! I will re-review it tomorrow then.

> Thanks again for the review!

You're welcome!

[Ben Murdoch]

Created attachment 49470 [details]
Patch and layout test changes.

Hi Kenneth, please see the new patch with extra comments added.

Thanks, Ben

[WebKit Commit Bot]

Comment on attachment 49470 [details]
Patch and layout test changes.

Clearing flags on attachment: 49470

Committed r55230: <http://trac.webkit.org/changeset/55230>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.