Bug 37609 - The TouchStationary state of WebCore::PlatformTouchPoint is not handled inside the touch event handler.
: The TouchStationary state of WebCore::PlatformTouchPoint is not handled insid...
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: Other All
: P2 Normal
Assigned To:
:
:
:
: 32485
  Show dependency treegraph
 
Reported: 2010-04-14 14:24 PST by
Modified: 2010-04-27 12:45 PST (History)


Attachments
Patch (3.77 KB, patch)
2010-04-14 17:48 PST, Ben Murdoch
no flags Review Patch | Details | Formatted Diff | Diff
Patch v2 (5.94 KB, patch)
2010-04-19 17:33 PST, Ben Murdoch
no flags Review Patch | Details | Formatted Diff | Diff
Patch with updated commentary re. TouchStationary. (1.83 KB, patch)
2010-04-26 15:09 PST, Ben Murdoch
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-04-14 14:24:55 PST
The TouchStationary state of WebCore::PlatformTouchPoint is not handled inside the touch event handler.
------- Comment #1 From 2010-04-14 14:40:00 PST -------
As discussed at the contributors meeting with Simon and Kim, we think it's best to remove the TouchStationary state.

Patch to follow.
------- Comment #2 From 2010-04-14 17:48:08 PST -------
Created an attachment (id=53389) [details]
Patch
------- Comment #3 From 2010-04-15 10:20:27 PST -------
Committed r57652: <http://trac.webkit.org/changeset/57652>
------- Comment #4 From 2010-04-15 10:36:17 PST -------
http://trac.webkit.org/changeset/57652 might have broken Qt Linux Release and Chromium Mac Release
------- Comment #5 From 2010-04-15 10:43:46 PST -------
Looking at break
------- Comment #6 From 2010-04-15 10:56:44 PST -------
OK, it's a warning as error on the Chromium Mac build that should be an easy fix, but there's also a QT layout test failure. Will roll back the patch entirely to figure out the layout test failure and get the tree green again.
------- Comment #7 From 2010-04-15 11:14:27 PST -------
Reverted r57652 for reason:

Caused a build break on Chromium Mac and Layout Test fail on Qt

Committed r57655: <http://trac.webkit.org/changeset/57655>
------- Comment #8 From 2010-04-16 11:55:31 PST -------
So the problem with this CL was two-fold:

1) Broke the Mac Chrome build: I removed the case statement referring to TouchStationary, but did not remove StationaryState from the WebTouchPoint::State enum, so the compiler raised a warning that not all enum values were considered in the switch inside toPlatformTouchPointState(). With warnings as errors, the build failed :(

2) Qt layout test failures: the QTouchEvent::TouchPoint is passed directly into the PlatformTouchPoint constructor from the qt API layer. Without checking for the TouchStationary case, the PlatformTouchPoint is then initialised without setting the m_state variable (which then defaults to the 0, TouchPressed value) so when DRT sends a stationary touch point, it gets interpreted as a new touch going down thus screwing everything up :)

To work round these issues, I'm thinking about instead adding a TouchUnsupported value to the PlatformTouchPoint::State enum (in addition to removing TouchStationary still). Then in the PlatformTouchPoint ctor assign the PlatformTouchPoint that state if it's not a press. move, release or cancel. That way the platform can pass us all sorts of types of touch point (that may or may not be specific to that platform) and we accept only the ones that WebCore is interested in. Then inside EventHandler::handleTouchEvent skip the touch point if it's TouchUnsupported.

What do you think?
------- Comment #9 From 2010-04-16 11:56:32 PST -------
I'll look into adding  a Cr-Mac EWS builder to prevent this kind of trouble in the future.
------- Comment #10 From 2010-04-16 14:00:21 PST -------
(In reply to comment #8)
> What do you think?

Sounds like a good plan :)
------- Comment #11 From 2010-04-19 17:33:56 PST -------
Created an attachment (id=53747) [details]
Patch v2

Here is a new patch implementing the TouchUnsupported state described above. I haven't been able to run the layout tests on Qt or try a compile on Mac due to being in the wrong place (because of a certain volcano) and not having the system I usually do. With this fix I don't foresee any issues but before landing I'd be really appreciative if someone could apply the patch and try it for me on those platforms.

Thanks!
------- Comment #12 From 2010-04-19 18:32:34 PST -------
I realize that there's one catch with not handling stationary touch points with the Qt implementation:

AFAICS it may happen that touch points become stationary, at which point with this patch they would disappear from the touch points in JavaScript. That's not intentional I think. Instead it would be better to map TouchStationary to TouchPressed.

What do you think?
------- Comment #13 From 2010-04-19 19:06:31 PST -------
I think the proper mapping for TouchStationary would be TouchMoved but now when I looked the whole thing again it makes sense to have the stationary state there: The stationary touchpoints are on the screen but have not changed in any way. Therefore they should be added to the touches list and maybe in the targetTouches but never to changedTouches. This is exactly what happens currently:

// touches should contain information about every touch currently on the screen.
if (point.state() != PlatformTouchPoint::TouchReleased)
   touches->append(touch);

// Now build up the correct list for changedTouches.
if (point.state() == PlatformTouchPoint::TouchReleased)
    releasedTouches->append(touch);
else if (point.state() == PlatformTouchPoint::TouchCancelled)
    cancelTouches->append(touch);
else if (point.state() == PlatformTouchPoint::TouchPressed)
    pressedTouches->append(touch);
else if (point.state() == PlatformTouchPoint::TouchMoved)
    movedTouches->append(touch);

Mapping the stationary state to pressed or moved would cause them to appear in wrong place (changedTouces of some type of touch event). Renaming it to unsupported would still keep the behavior same if the early continue is not added. In case of the early continue, the stationary points would not appear in the touches list which is also wrong. We need to be able to properly handle also the points that are on the screen but have not been changed in any way.
------- Comment #14 From 2010-04-20 10:07:47 PST -------
Hi Simon and Kim,

Thanks for the feedback, your comments all make sense.

The issue that we encountered that drove this bug was that on Android we were seeing a touch stationary point going into the event handler in the case of a single touch. This is due to the behavior on Android that when you move a touch beyond the boundaries of the WebView (e.g. over scrollbars) it is snapped back to the WebView boundaries. So the platform was sending move events that were getting mapped to stationary events in the platform layer as the co-ordinates matched the last event. Then with no explicit handler for the TouchStationary point in handleTouchEvent, no events were getting dispatched and the handler would return false. My original thinking was that as it wasn't handled in the event handler itself, we could remove it. Now given the attempts at this and the discussion we've had in this bug, I'm thinking that it is actually correct to leave the common code as it is and just fix this in the android platform layer. The snapping of co-ordinates is Android specific behavior and as Kim points out, there is in fact value in having a TouchStationary state in the multi-touch case.

So I guess the correct thing to do is mark this as resolved - wontfix?

Thanks, Ben
------- Comment #15 From 2010-04-21 18:13:25 PST -------
(From update of attachment 53389 [details])
Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 53389 [details] so that this bug does not appear in http://webkit.org/pending-commit.
------- Comment #16 From 2010-04-23 12:46:17 PST -------
OK - I think lack of response means we're good if we close this but one last question:

Is there value in sending a CL to add a comment to EventHandler::handleTouchEvent to record basically the discussion we've had in the bug that there is value in the TouchStationary state and to describe that is is not handled explicitly intentionally so as to avoid any confusion like this in the future? Not sure the webkit stance on comment-only patches but I think there could be some value in this one.

Thoughts?
------- Comment #17 From 2010-04-23 23:03:58 PST -------
Sorry for the late response. I agree with you on everything :)
And I also think that the confusion we had with the touchstationary state indicates that better commenting would be valuable.
------- Comment #18 From 2010-04-26 15:09:13 PST -------
Created an attachment (id=54337) [details]
Patch with updated commentary re. TouchStationary.

Here is a patch with some updated commentary on TouchStationary. What do you think?

Thanks, Ben
------- Comment #19 From 2010-04-26 15:35:12 PST -------
Looks good to me. That would have been enough to prevent the confusion we had.
------- Comment #20 From 2010-04-27 03:02:38 PST -------
(From update of attachment 54337 [details])
Thanks Ben!
------- Comment #21 From 2010-04-27 12:45:41 PST -------
(From update of attachment 54337 [details])
Clearing flags on attachment: 54337

Committed r58323: <http://trac.webkit.org/changeset/58323>
------- Comment #22 From 2010-04-27 12:45:49 PST -------
All reviewed patches have been landed.  Closing bug.