Bug 51258 – TouchEvents does not support multi-touch on a page with multiple touch targets

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jonathan Dixon]

The way EventHandler::handleTouchEvent currently works means that all touch points of a given type (press, move, release) will all be sent to a single target.
This does not work correctly with the page above, as each draggable square is its own div with its own event target. So, for e.g., using 2 fingers to move 2 divs requires each div receive its own ontouchmove events, containing the respective changedTouches.

Currently, the implementation only dispatches all of the move events to a single target:

3011	    if (movedTouches->length() > 0) {
3012	        Touch* changedTouch = movedTouches->item(0);  // <-- only looking up the first touch point's target
3014	        touchEventTarget = changedTouch->target();
....
3026	        touchEventTarget->dispatchEvent(moveEv.get(), ec);
3027	        defaultPrevented |= moveEv->defaultPrevented();  // <-- dispatching all the move events to this single touch target
3028	    }
3029	

ditto for touch start, end and cancel events.

[Jonathan Dixon]

I have started work on a patch to address this.

[Jonathan Dixon]

Created attachment 77097 [details]
Proposed fix 1

[Jonathan Dixon]

(In reply to comment #2)
> Created an attachment (id=77097) [details]
> Proposed fix 1

forgot to mention: tested using the Qt test runner:


$ ./WebKitTools/Scripts/run-webkit-tests --qt --repeat-each 50 --timeout 5 fast/events/touch/
Running build-dumprendertree
Running tests from /usr/local/google/home/joth/dev/b.clank/src/third_party/WebKit/LayoutTests
Testing 13 test cases, repeating each test 50 times.
fast/events/touch .................................................................................................
6.72s total testing time

all 650 test cases succeeded

[Ben Murdoch]

Comment on attachment 77097 [details]
Proposed fix 1

View in context: https://bugs.webkit.org/attachment.cgi?id=77097&action=review

Nice cleanup, thanks! Just a few comments.

> WebCore/page/EventHandler.cpp:2789
> +static const AtomicString& eventNameForTouchPointState(int state)

PlatformTouchPoint::State instead of int?

> WebCore/page/EventHandler.cpp:2808
> +    // The complete set of touches in this event, for the event touches list.

I think this comment is a little confusing. How about something like
"Holds the complete set of touches on the screen and will be used as the 'touches' list in the JS event."

> WebCore/page/EventHandler.cpp:2811
> +    // For each target holds the complete set of currently active touches, for the targetTouches list.

Again, a bit confusing. Maybe be more explicit and mention that it forms the 'targetTouches' list in the JS event. I think the touch code needs as many clear comments as possible :)

> WebCore/page/EventHandler.cpp:2815
> +    // Array of touches per state, used to assembe the the changedTouches list.

assembe -> assemble and too many 'the's :), changedTouches list in JS event.

> WebCore/page/EventHandler.cpp:2820
> +        HashSet<EventTarget*> m_targets;

Should the EventTargets be RefPtrs?

> WebCore/page/EventHandler.cpp:2854
> +        ASSERT(node);

node.get()?

> WebCore/page/EventHandler.cpp:2891
> +        if (!touchTarget.get())

Why is this moved? Seemed better in it's old place so that we didn't bother creating a Touch without a target.

> WebCore/page/EventHandler.cpp:2899
> +        // touches and targetTouches should contain information about every touch currently on the screen.

Maybe expand this comment to say that if it is a TouchRelease or Cancel then as it's not on the screen it won't be in touches or targetTouches, but will be in changedTouches.

> WebCore/page/EventHandler.cpp:2916
> +            changedTouches[point.state()].m_targets.add(touchTarget.get());

Perhaps point.state() could go into a local, we seem to be calling it a lot.

> WebCore/page/EventHandler.cpp:2932
> +        const AtomicString& stateName(eventNameForTouchPointState(state));

Can we move this into the TouchEvent::create call directly?

> WebCore/page/EventHandler.cpp:2938
> +            ASSERT(targetTouches);

targetTouches.get()?

[Jonathan Dixon]

Comment on attachment 77097 [details]
Proposed fix 1

View in context: https://bugs.webkit.org/attachment.cgi?id=77097&action=review

Thanks for the comments, think I covered them all. Just preparing a new patch...

>> WebCore/page/EventHandler.cpp:2789
>> +static const AtomicString& eventNameForTouchPointState(int state)
> 
> PlatformTouchPoint::State instead of int?

Done

>> WebCore/page/EventHandler.cpp:2808
>> +    // The complete set of touches in this event, for the event touches list.
> 
> I think this comment is a little confusing. How about something like
> "Holds the complete set of touches on the screen and will be used as the 'touches' list in the JS event."

Done. Thanks.

>> WebCore/page/EventHandler.cpp:2811
>> +    // For each target holds the complete set of currently active touches, for the targetTouches list.
> 
> Again, a bit confusing. Maybe be more explicit and mention that it forms the 'targetTouches' list in the JS event. I think the touch code needs as many clear comments as possible :)

How about:
    // Another different on the 'touches' list above: filtered and grouped by event target. Used for the
    // 'targetTouches' list in the JS event.

>> WebCore/page/EventHandler.cpp:2815
>> +    // Array of touches per state, used to assembe the the changedTouches list.
> 
> assembe -> assemble and too many 'the's :), changedTouches list in JS event.

Done

>> WebCore/page/EventHandler.cpp:2820
>> +        HashSet<EventTarget*> m_targets;
> 
> Should the EventTargets be RefPtrs?

Done. + added a typedef EventTargetSet

>> WebCore/page/EventHandler.cpp:2854
>> +        ASSERT(node);
> 
> node.get()?

Apparently not, jorlow says to rely on the conversion operator.

>> WebCore/page/EventHandler.cpp:2891
>> +        if (!touchTarget.get())
> 
> Why is this moved? Seemed better in it's old place so that we didn't bother creating a Touch without a target.

Mistake, fixed. Good spot!

>> WebCore/page/EventHandler.cpp:2899
>> +        // touches and targetTouches should contain information about every touch currently on the screen.
> 
> Maybe expand this comment to say that if it is a TouchRelease or Cancel then as it's not on the screen it won't be in touches or targetTouches, but will be in changedTouches.

Changed to:
        // touches and targetTouches should only contain information about touches still on the screen, so if this point is
        // released or cancelled it will only appear in the changedTouches list.

I also added a link to http://www.sitepen.com/blog/2008/07/10/touching-and-gesturing-on-the-iphone/ upto, which hopefully helps spell this lot out.

>> WebCore/page/EventHandler.cpp:2916
>> +            changedTouches[point.state()].m_targets.add(touchTarget.get());
> 
> Perhaps point.state() could go into a local, we seem to be calling it a lot.

Done

>> WebCore/page/EventHandler.cpp:2932
>> +        const AtomicString& stateName(eventNameForTouchPointState(state));
> 
> Can we move this into the TouchEvent::create call directly?

Minor benefit it doing it outside the loop, and also it now has a static_cast (see your comment #1) so probably clearer as it too.

>> WebCore/page/EventHandler.cpp:2938
>> +            ASSERT(targetTouches);
> 
> targetTouches.get()?

Apparently not

[Jonathan Dixon]

Created attachment 77116 [details]
Patch 2 - benm comments.

[Antonio Gomes]

Comment on attachment 77116 [details]
Patch 2 - benm comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=77116&action=review

> WebCore/page/EventHandler.cpp:2801
> +        ASSERT(false);

ASSERT_NOT_REACHED()

[Jonathan Dixon]

Created attachment 77200 [details]
Patch 3 - tonikitoo comment and fix comment typo

Thanks.

[Ben Murdoch]

Comment on attachment 77200 [details]
Patch 3 - tonikitoo comment and fix comment typo

View in context: https://bugs.webkit.org/attachment.cgi?id=77200&action=review

WebCore changes look good. Had a quick look over the layout tests ...

To any other folks participating in the review: I don't have review rights so I cannot r+ this patch when it's ready.

> LayoutTests/fast/events/touch/touch-target-expected.txt:41
> +TEST COMPLETE

We now have two "test complete" messages in this test - doesn't seem right?

> LayoutTests/fast/events/touch/touch-target-limited-expected.txt:45
> +TEST COMPLETE

ditto

[Jonathan Dixon]

Created attachment 77201 [details]
Patch 4 - corrects expected results to only have one TEST COMPLETE

[Steve Block]

Comment on attachment 77201 [details]
Patch 4 - corrects expected results to only have one TEST COMPLETE

View in context: https://bugs.webkit.org/attachment.cgi?id=77201&action=review

> WebCore/page/EventHandler.cpp:2860
> +        RefPtr<Node> node = result.innerNode();

Why do we now take a reference? Is this required?

> WebCore/page/EventHandler.cpp:2943
> +            RefPtr<TouchList> targetTouches(state == PlatformTouchPoint::TouchCancelled ? emptyList : touchesByTarget.get(touchEventTarget));

Would it be clearer to have a isTouchCancelEvent boolean set up outside this inner loop?

[Jonathan Dixon]

Created attachment 77219 [details]
Patch 5 - steveblock comments

Thanks, both comments done.
Could you commit-queue+ it too? Cheers.

[Steve Block]

Comment on attachment 77219 [details]
Patch 5 - steveblock comments

r=me

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 77219 [details]:

inspector/debugger-pause-on-breakpoint.html bug 51320 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 77219 [details]
Patch 5 - steveblock comments

Clearing flags on attachment: 77219

Committed r74553: <http://trac.webkit.org/changeset/74553>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andy Estes]

Comment on attachment 77219 [details]
Patch 5 - steveblock comments

View in context: https://bugs.webkit.org/attachment.cgi?id=77219&action=review

> LayoutTests/fast/events/touch/script-tests/multi-touch-grouped-targets.js:75
> +    eventSender.addTouchPoint(50, 150);

Did you intend to add two identical touch points here? Since you really only add two unique test points, a correct implementation should fail this test (event.changedTouches.length should never be 3, for instance).