Bug 34585 - [Qt] The target element of a Touch should be the target where that touch originated, not where it is now.
Summary: [Qt] The target element of a Touch should be the target where that touch orig...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ben Murdoch
URL: http://www.ksasq.com/touch/touch-targ...
Keywords:
Depends on: 34476
Blocks: 32485
  Show dependency treegraph
 
Reported: 2010-02-04 06:27 PST by Ben Murdoch
Modified: 2010-02-25 05:24 PST (History)
4 users (show)

See Also:


Attachments
First draft of code to fix the problem. Layout tests to follow later. (3.10 KB, patch)
2010-02-04 06:31 PST, Ben Murdoch
no flags Details | Formatted Diff | Diff
Patch and layout test changes. (13.99 KB, patch)
2010-02-24 04:16 PST, Ben Murdoch
no flags Details | Formatted Diff | Diff
Proposed patch and test. (14.01 KB, patch)
2010-02-24 10:37 PST, Ben Murdoch
no flags Details | Formatted Diff | Diff
Patch (line wrapped Changelogs) (14.08 KB, patch)
2010-02-24 10:56 PST, Ben Murdoch
no flags Details | Formatted Diff | Diff
Patch and layout test changes. (14.45 KB, patch)
2010-02-25 02:41 PST, Ben Murdoch
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Murdoch 2010-02-04 06:27:01 PST
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
Comment 1 Ben Murdoch 2010-02-04 06:31:08 PST
Created attachment 48136 [details]
First draft of code to fix the problem. Layout tests to follow later.
Comment 2 Ben Murdoch 2010-02-24 04:16:33 PST
Created attachment 49378 [details]
Patch and layout test changes.
Comment 3 Ben Murdoch 2010-02-24 10:37:40 PST
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!
Comment 4 Ben Murdoch 2010-02-24 10:39:09 PST
Tagging Qt as there are some small edits to Qt specific files.
Comment 5 Kenneth Rohde Christiansen 2010-02-24 10:47:22 PST
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.
Comment 6 Ben Murdoch 2010-02-24 10:51:21 PST
(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.
Comment 7 Ben Murdoch 2010-02-24 10:56:14 PST
Created attachment 49412 [details]
Patch (line wrapped Changelogs)
Comment 8 Kenneth Rohde Christiansen 2010-02-24 11:12:51 PST
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.
Comment 9 Ben Murdoch 2010-02-24 14:21:35 PST
(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
Comment 10 Kenneth Rohde Christiansen 2010-02-24 14:52:06 PST
> 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!
Comment 11 Ben Murdoch 2010-02-25 02:41:34 PST
Created attachment 49470 [details]
Patch and layout test changes.

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

Thanks, Ben
Comment 12 WebKit Commit Bot 2010-02-25 05:24:18 PST
Comment on attachment 49470 [details]
Patch and layout test changes.

Clearing flags on attachment: 49470

Committed r55230: <http://trac.webkit.org/changeset/55230>
Comment 13 WebKit Commit Bot 2010-02-25 05:24:23 PST
All reviewed patches have been landed.  Closing bug.