Bug 37609

As discussed at the contributors meeting with Simon and Kim, we think it's best to remove the TouchStationary state. Patch to follow.

Created attachment 53389 [details] Patch

Committed r57652: <http://trac.webkit.org/changeset/57652>

http://trac.webkit.org/changeset/57652 might have broken Qt Linux Release and Chromium Mac Release

Looking at break

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.

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>

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?

I'll look into adding a Cr-Mac EWS builder to prevent this kind of trouble in the future.

(In reply to comment #8) > What do you think? Sounds like a good plan :)

Created attachment 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!

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?

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.

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 on attachment 53389 [details] Patch Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 53389 [details] so that this bug does not appear in http://webkit.org/pending-commit.

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?

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.

Created attachment 54337 [details] Patch with updated commentary re. TouchStationary. Here is a patch with some updated commentary on TouchStationary. What do you think? Thanks, Ben

Looks good to me. That would have been enough to prevent the confusion we had.

Comment on attachment 54337 [details] Patch with updated commentary re. TouchStationary. Thanks Ben!

Comment on attachment 54337 [details] Patch with updated commentary re. TouchStationary. Clearing flags on attachment: 54337 Committed r58323: <http://trac.webkit.org/changeset/58323>

All reviewed patches have been landed. Closing bug.