Bug 96677 – Do touch adjustment on GestureTapDown

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Rick Byers]

Since we now (as of bug 96060) can use GestureTapDown to trigger :active states, we should really use the same approach for hit testing GestureTapDown as GestureTap (to increase the chance they'll select the same element).  In particular we should be applying touch adjustment.

It's debatable whether we should go even further than this - i.e. maybe we should keep track of the element hit by TapDown and ensure the same done is used by Tap (either by using it, or deciding to convert a Tap into a TapCancel).  But here I'd like to take just the first step of using the same algorithm, so that most times (in the absence of the page moving under your finger), Tap will target the element activated/hilighted by TapDown.

[Rick Byers]

Created attachment 164199 [details]
Patch

[Rick Byers]

Note this patch depends on my changes in bug 96060 (just for merge context) and 96806.  I'll upload a combined patch for the sake of EWS.

[Rick Byers]

Created attachment 164200 [details]
Combined with dependant patches for the sake of EWS

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Rick Byers]

Created attachment 164446 [details]
Merge with trunk

[Rick Byers]

Allan / Antonio, can you please review this change for me?

[Antonio Gomes]

Comment on attachment 164446 [details]
Merge with trunk

Could you explain to me the flow of gesture events in:

1) touch down on a link (i.e. activatable), and scroll the page.
2) tap on a link (touch down / up within a reasonable tap time).

I want to see something like;

"for _1_, tapgesturedown is processed, no rect hit test is performed yet, then when scroll and let it go, gesturetapcancel. With the patch, a rect based hit test is performed when ..."

"for _2_ we touch down, no hit test, then when I lift my finger, we do a rect based hit test, find the target and send a 'tap'"

[Rick Byers]

Thanks Antonio.  Let me know how much of this should be in the ChangeLog, or comments.  Note that I'm describing the event flow for desktop chromium (the only platform that has GestureTapDownCancel at the moment), the gesture recognizer details may differ slightly for ports like Qt and Chromium Android.

(In reply to comment #7)
> (From update of attachment 164446 [details])
> Could you explain to me the flow of gesture events in:
> 
> 1) touch down on a link (i.e. activatable), and scroll the page.

Current behavior:
- GestureTapDown is received, do a point hit test, but no rect hit test
- finger moves enough, get get GestureTapDownCancel (point hit test with Release)
- get GestureScrollBegin, and as the finger moves GestureScrollUpdate and ultimately GestureScrollEnd

With this patch:
- On GestureTapDown, do the rect-based hittest, using the 'adjusted point' as the new input to the (now redundant) point-based hit test.
- otherwise identical to the above

> 2) tap on a link (touch down / up within a reasonable tap time).

Current behavior:
- GestureTapDown, do a point hit test with Active
- finger goes up within acceptable movement/time bounds: GestureTap, do a point hit test with Release, then a rect-based hit test and send the tap event to the resulting node.

With this patch:
- On GestureTapDown, do the rect-based hittest, using the 'adjusted point' as the new input to the (now redundant) point-based hit test.
- otherwise identical to the above


I see some potential options for improving this.  Being relatively new to this code I didn't want to change the architecture as part of this CL, but I'm happy to (in this or a future CL) if you think it makes sense.
 - Combine the point and rect based hit tests so that we only ever do one or the other.  I'm a little worried about making sure I get the identical semantics, but it shouldn't be too hard.
 - don't do another rect-based hit test on GestureTap, but instead re-use the result from GestureTapDown time.  It's debatable whether this is the behavior we want, i.e. when the page is changing under the finger (personally I think it is).

[Allan Sandfeld Jensen]

Comment on attachment 164446 [details]
Merge with trunk

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

> Tools/DumpRenderTree/chromium/TestRunner/EventSender.cpp:1194
> +        if (arguments.size() >= 4) {
> +            event.data.tapDown.width = static_cast<float>(arguments[2].toDouble());
> +            event.data.tapDown.height = static_cast<float>(arguments[3].toDouble());
> +        }

Perhaps these gesture evens could be using the touch-radius set by setTouchRadius? This way you wouldn't need to expand all the other TestRunners with another extension.

[Allan Sandfeld Jensen]

Do you have any plans or ideas for reusing the TapDown result of the touch-adjusted for link highlighting and later handling of Tap?

[Allan Sandfeld Jensen]

(In reply to comment #10)
> Do you have any plans or ideas for reusing the TapDown result of the touch-adjusted for link highlighting and later handling of Tap?

Aghh. Let me rephrase that: Resuing the result of touch-adjustment here in TapDown, when the same result is needed for link-highlighting and again in the actualy handling  of Tap.

[Rick Byers]

(In reply to comment #9)
> (From update of attachment 164446 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164446&action=review
> 
> > Tools/DumpRenderTree/chromium/TestRunner/EventSender.cpp:1194
> > +        if (arguments.size() >= 4) {
> > +            event.data.tapDown.width = static_cast<float>(arguments[2].toDouble());
> > +            event.data.tapDown.height = static_cast<float>(arguments[3].toDouble());
> > +        }
> 
> Perhaps these gesture evens could be using the touch-radius set by setTouchRadius? This way you wouldn't need to expand all the other TestRunners with another extension.

In general I don't think we want to assume that gesture bounding box == touch point radius.  For example, in chromium we currently set the bounding box of a tap gesture to include the small movements we inevitably get between touchstart and touchend (the idea being that if your finger wiggles during a tap, we want effectively the union of all the ellipses for the list of candidates).  That said, we might eliminate that, and TapDown is a pretty simple case that correlates quite closely to touchstart.

Still, if this is just about making the test infrastructure simpler, I'd prefer other approaches to solving that problem (eg. maybe we should have a cross-port mechanism for generating PlatformGestureEvents, rather than a port-specific WebGestureEvent).  Personally I like the purity of leaving gesture and touch events decoupled in WebKit (maybe in the future there should be some way to send a GestureTap with input other than touchscreen - eg. eye tracking for the disabled or something).

[Rick Byers]

(In reply to comment #11)
> (In reply to comment #10)
> > Do you have any plans or ideas for reusing the TapDown result of the touch-adjusted for link highlighting and later handling of Tap?
> 
> Aghh. Let me rephrase that: Resuing the result of touch-adjustment here in TapDown, when the same result is needed for link-highlighting and again in the actualy handling  of Tap.

I've thought about it a bit, but I'm not sure yet exactly what we should do.  There is some discussion here: http://code.google.com/p/chromium/issues/detail?id=132439.  The simplest thing would just be to save the target on TapDown here in EventHandler.cpp, and re-use the same target for Tap - perhaps similar to m_scrollGestureHandlingNode.

Is it OK if I file a separate bug to track this question?  It's lower priority in my mind (but still something I intend to drive to conclusion).

[Allan Sandfeld Jensen]

(In reply to comment #13)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > Do you have any plans or ideas for reusing the TapDown result of the touch-adjusted for link highlighting and later handling of Tap?
> > 
> > Aghh. Let me rephrase that: Resuing the result of touch-adjustment here in TapDown, when the same result is needed for link-highlighting and again in the actualy handling  of Tap.
> 
> I've thought about it a bit, but I'm not sure yet exactly what we should do.  There is some discussion here: http://code.google.com/p/chromium/issues/detail?id=132439.  The simplest thing would just be to save the target on TapDown here in EventHandler.cpp, and re-use the same target for Tap - perhaps similar to m_scrollGestureHandlingNode.
> 
> Is it OK if I file a separate bug to track this question?  It's lower priority in my mind (but still something I intend to drive to conclusion).

Of course, I just want to make sure you have thought about it so we don't end up in a situation where this patch would have to be redone to support it.

For my perspective I would like TapDown to be able to work together with the logic used by WebKit2 WebPage::highlightPotentialActivation, so we don't end up with even more redundant hit-tests. Hit tests are expensive, and especially area based ones, so just redoing it everywhere is not a winning strategy.

[Antonio Gomes]

> Current behavior:
> - GestureTapDown, do a point hit test with Active
> - finger goes up within acceptable movement/time bounds: GestureTap, do a point hit test with Release, then a rect-based hit test and send the tap event to the resulting node.
> 
> With this patch:
> - On GestureTapDown, do the rect-based hittest, using the 'adjusted point' as the new input to the (now redundant) point-based hit test.
> - otherwise identical to the above
> 
> 
> I see some potential options for improving this.  Being relatively new to this code I didn't want to change the architecture as part of this CL, but I'm happy to (in this or a future CL) if you think it makes sense.
>  - Combine the point and rect based hit tests so that we only ever do one or the other.  I'm a little worried about making sure I get the identical semantics, but it shouldn't be too hard.
>  - don't do another rect-based hit test on GestureTap, but instead re-use the result from GestureTapDown time.  It's debatable whether this is the behavior we want, i.e. when the page is changing under the finger (personally I think it is).

Thanks. That was exactly what I was looking for: how many and when we point/rect hit test for a given action.

I do see needs for unifications here, as you pointed out. For BlackBerry, we have a cache class called FatFingersResult: until another gesture action is started or we receive a TouchCancel, we keep using the same cached result, we so avoid hit testing as much as we can. 

I think we can go with what you have now, but definitively there is room for improvements here, specially perf-wise, given that hit tests can take up 0.3.

[Rick Byers]

Thanks guys.  I filed bug 97040 to track improving the perf here - at least merging redundant hit tests.  I also filed bug 97041 to track the question of how to handle  changes between TapDown and Tap (perhaps eliminating another hit test).

Anything else you'd like to discuss before putting this in the CQ?

[WebKit Review Bot]

Comment on attachment 164446 [details]
Merge with trunk

Clearing flags on attachment: 164446

Committed r128999: <http://trac.webkit.org/changeset/128999>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.