Improve touch handling performance by reusing the hitTest result

• Component: UI Events
• Status: RESOLVED FIXED

[Min Qin]

Currently we are doing a hit test for every touch events, this is unnecessary.
For TouchRelease and TouchMove events, the touchTarget is always the same as the TouchPressed event.
Even worse, performing a hitTest on a TouchMove could give us a new containing frame as finger moves. This could totally mess up the touch handling.
One good example for this is scrolling http://www.nme.com/reviews/young-knives/11931 on android browser with galaxy nexus. Touch the first paragraph and start scrolling upwards. When finger reaches the video area, flickering will appear.

This change reuses the hit test results from TouchPressed for further touch events.

[Min Qin]

Created attachment 121025 [details]
Patch

[Alexey Proskuryakov]

> For TouchRelease and TouchMove events, the touchTarget is always the same as the TouchPressed event.

Is it really the case? Page content may move, so another element will be at the touch position. The original element may even be removed already.

[Min Qin]

Yes, this is the case. TouchTarget should not change before finger is released. In the current EventHandler.cc, for example, the touchTarget  for TouchRelease and TouchMove is always the node given by TouchPressed.
Yes, the touchTarget could be removed during touch moves. But since it is ref counted, so as long as eventHandler maintains a pointer to it, it will not be GCed. And subsequent events send to it should do nothing. 

(In reply to comment #2)
> > For TouchRelease and TouchMove events, the touchTarget is always the same as the TouchPressed event.
> 
> Is it really the case? Page content may move, so another element will be at the touch position. The original element may even be removed already.

[Min Qin]

From the spec: https://dvcs.w3.org/hg/webevents/raw-file/tip/touchevents.html
5.6 The touchmove event
The target of this event must be the same Element on which the touch point started when it was first placed on the surface, even if the touch point has since moved outside the interactive area of the target element.

(In reply to comment #3)
> Yes, this is the case. TouchTarget should not change before finger is released. In the current EventHandler.cc, for example, the touchTarget  for TouchRelease and TouchMove is always the node given by TouchPressed.
> Yes, the touchTarget could be removed during touch moves. But since it is ref counted, so as long as eventHandler maintains a pointer to it, it will not be GCed. And subsequent events send to it should do nothing. 
> 
> (In reply to comment #2)
> > > For TouchRelease and TouchMove events, the touchTarget is always the same as the TouchPressed event.
> > 
> > Is it really the case? Page content may move, so another element will be at the touch position. The original element may even be removed already.

[Adam Barth]

Comment on attachment 121025 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

We won't be able to land this patch with this line in the ChangeLog.  In cases like this, we usually replace this line with a sentence saying that that patch is a performance optimization only and shouldn't have any behavior changes.

[Min Qin]

Created attachment 122981 [details]
Patch

[Min Qin]

ChangeLog fixed

[Adam Barth]

Comment on attachment 122981 [details]
Patch

Thanks.

[WebKit Review Bot]

Comment on attachment 122981 [details]
Patch

Clearing flags on attachment: 122981

Committed r105453: <http://trac.webkit.org/changeset/105453>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Allan Sandfeld Jensen]

We already have code that avoids moving the frame on hittests that are read-only. Was this really necessary in trunk to fix this bug?

While this patch is still an optimization. It makes it impossible to improve the :active handling to for instance stop showing :active if the finger moves off the target.

:active is used to show the user what is activated, but once the finger ends off the target this link will no longer be activated, and thus the :active feedback to user will be wrong.

[Min Qin]

Why is it hard to show :active? Since the touchTarget won't change, the last hittest results could be reused as finger moves off the target. So your :active link won't change during a touch down->touch move->touch release

On some pages, doing a hittest on each touchMove can be really expensive. the espn blog page is a good example. It invokes a layout each time when a move happens. each layout could take more than 500 msec to do.

And this change also avoids the problem that finger could move to another element after touchDown. One good example for this is scrolling http://www.nme.com/reviews/young-knives/11931 on android browser. Touch the first paragraph and start scrolling upwards. When finger reaches the video area, flickering will appear as a new hittest will return a different pagePoint as the condition of if statement becomes true.

if (m_frame != doc->frame()) {		
            // pagePoint should always be relative to the target elements contai
ning frame.		
            pagePoint = documentPointForWindowPoint(doc->frame(), point.pos());		
}



(In reply to comment #11)
> We already have code that avoids moving the frame on hittests that are read-only. Was this really necessary in trunk to fix this bug?
> 
> While this patch is still an optimization. It makes it impossible to improve the :active handling to for instance stop showing :active if the finger moves off the target.
> 
> :active is used to show the user what is activated, but once the finger ends off the target this link will no longer be activated, and thus the :active feedback to user will be wrong.

[Min Qin]

And one more thing on finger moves off the target:

From the spec: https://dvcs.w3.org/hg/webevents/raw-file/tip/touchevents.html:
5.6 The touchmove event
The target of this event must be the same Element on which the touch point started when it was first placed on the surface, even if the touch point has since moved outside the interactive area of the target element.

Even if the finger is off the target, the :active should still remain on the touch down target. Until touch is released. So user should still see the original touch target is activated.

(In reply to comment #11)
> We already have code that avoids moving the frame on hittests that are read-only. Was this really necessary in trunk to fix this bug?
> 
> While this patch is still an optimization. It makes it impossible to improve the :active handling to for instance stop showing :active if the finger moves off the target.
> 
> :active is used to show the user what is activated, but once the finger ends off the target this link will no longer be activated, and thus the :active feedback to user will be wrong.

[Allan Sandfeld Jensen]

(In reply to comment #12)
> Why is it hard to show :active? Since the touchTarget won't change, the last hittest results could be reused as finger moves off the target. So your :active link won't change during a touch down->touch move->touch release
> 
The point was to cancel :active and :hover if the activated object no longer is going to be activated by the current gesture. You make a good case for optimizing touch move, but are activated and hovered elements updated on TouchReleased or TouchCancelled?

I believe maintaining the active- and hovered elements is usually done by the hitTest. If hitTest is not performed on touch-release the active or hovered elements will stick to the last hitTested result. Actually hover already stick, but I was going to fix that, but will need a hit-test on touch-release/cancel to do so.

[Yong Li]

 	        Document* doc = node->document(); 
3264	 	        if (!doc) 
3265	 	            continue; 

This check is moved to if (pointState == PlatformTouchPoint::TouchPressed) block. And we no longer check it for the other two cases.

I know the node is locked by a RefPtr, however that doesn't prevent it from being evicted from the document. Will this cause potential crash?

[Yong Li]

Bug 81958 is created for the regression.