Implement touch-action pan-x/pan-y handling on the main thread

Implement touch-action pan-x/pan-y handling on the main thread.

This extends the touch-action support on the main thread to handle and combine pan-x and pan-y values appropriately when computing the effective action to send to the embedder.

BUG=321643
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164108

[gnana, 2013-12-10 19:14:49]

Please have a look at this.

[gnana, 2013-12-10 19:16:44]

Please have a look at this patch.

[Rick Byers, 2013-12-12 20:47:24]

Thanks, this is the right idea!  I have a few suggestions to clean it up a
bit...

Rick

https://codereview.chromium.org/103823003/diff/20001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/103823003/diff/20001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3990: effectiveTouchAction = action;
As esprehn pointed out reviewing my CL for this, we should immediately return
whenever effectiveTouchAction becomes None.  Perhaps just add a test and 'break'
for this at the bottom.

https://codereview.chromium.org/103823003/diff/20001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3991: } else if (action == TouchActionPanX) {
This seems correct, but it's pretty verbose and hard to scale as we add more
flags.   I think this could be simplified and made more general by writing a
helper function like:

TouchAction intersectTouchAction(TouchAction ta1, TouchAction ta2)
{
  if (ta1 == None || ta2 == None)
    return None;
  if (ta1 == Auto)
    return ta2;
  if (ta2 == Auto)
    return ta1;
  if (!(ta1 & ta2))
    return None;
  return ta1 & ta2;
}

This logic could be slightly simpler if we renumbered None to be 0 and Auto to
be 1 (instead of the other way around as it is currently), but I don't think it
really matters (and having the default be value 0 is kind of nice).

https://codereview.chromium.org/103823003/diff/20001/Source/web/tests/data/to...
File Source/web/tests/data/touch-action-overflow.html (right):

https://codereview.chromium.org/103823003/diff/20001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-overflow.html:28: <div class='scroll ta-panx'
style='height:75px'>
These two tests really test the same code paths as tests in 'simple' (just
intersecting pan-x with auto or pan-y).  I think the key thing to test here is
that the scroller behaves as a boundary for the pan values (not just for 'none'
as tested above).  So for example, have an outer div with ta-panx and a scroller
with ta-pany.  One case like that is probably sufficient since there really
isn't distinct code paths involved here.

https://codereview.chromium.org/103823003/diff/20001/Source/web/tests/data/to...
File Source/web/tests/data/touch-action-simple.html (right):

https://codereview.chromium.org/103823003/diff/20001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-simple.html:11: <div class='ta-panx'
expected-action='pan-x'>Simple div with touch-action: pan-x</div>
Please add a separate test case / input file for the pan cases.  These are no
longer really "simple" and breaking it up makes it easier to prioritize and
debug failures.

I don't have a strong opinion about the overflow scroll cases - they could
either stay in the scroll test case or move to the pan test case.

https://codereview.chromium.org/103823003/diff/20001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-simple.html:35: Explicitly setting
touch-action: pan-y does nothing to prevent propagation of none
It's probably overkill to list each possible combination here, but we should try
to hit some of the key different branches in the code (most of which aren't
covered here).  So I'd suggest maybe:
 - outer=ta-none, inner=ta-panx (as you have above)
 - outer=ta-pany, inner=none
 - outer=ta-panx, inner=ta-pany (as you have below)
 - outer=ta-panxy, inner=ta-auto
 - outer=ta-panx, inner=ta-panxy
 - outer=ta-panxy, inner=ta-pany
 - maybe one 3-level case, like outer=ta-panx middle=ta-auto inner=ta-pany

https://codereview.chromium.org/103823003/diff/20001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-simple.html:45: <div class='ta-panx'
style='height: 0; margin-bottom: 50px'>
You don't need the 'height:- margin-bottom: 50px' hack in this case.  The point
of that hack is to ensure that a hit-testing bug that picked up the outer
element instead of the inner one wouldn't produce the same result as what we
expect.  In this case (and many of the other ones I suggested above) the
expected action is different from that of the outer div, so they can just have a
normal containment relationship without risking any such bug.

https://codereview.chromium.org/103823003/diff/20001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-simple.html:47: Explicitly setting
touch-action: pan-y does nothing to prevent propagation of pan-x resulting to
none
I don't think the 'prevent propagation of...' part of the description still
applies to this case.  I'd simplify the description of this test to something
like "Disjoint flags intersect to none".

https://codereview.chromium.org/103823003/diff/20001/Source/web/tests/data/to...
File Source/web/tests/data/touch-action-tests.css (right):

https://codereview.chromium.org/103823003/diff/20001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-tests.css:46: [expected-action=pan-x
pan-y]::before {
Chrome claims this selector syntax is invalid.  Putting "pan-x pan-y" in quotes
seems to fix it.  Alternately it's probably simpler to just invent a token like
'pan-x-y' for this case.

[gnana, 2013-12-13 16:52:53]

Thanks for review comments. I have applied the comments.

https://codereview.chromium.org/103823003/diff/20001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/103823003/diff/20001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3990: effectiveTouchAction = action;
On 2013/12/12 20:47:24, Rick Byers wrote:
> As esprehn pointed out reviewing my CL for this, we should immediately return
> whenever effectiveTouchAction becomes None.  Perhaps just add a test and
'break'
> for this at the bottom.

Done.

https://codereview.chromium.org/103823003/diff/20001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3991: } else if (action == TouchActionPanX) {
On 2013/12/12 20:47:24, Rick Byers wrote:
> This seems correct, but it's pretty verbose and hard to scale as we add more
> flags.   I think this could be simplified and made more general by writing a
> helper function like:
> 
> TouchAction intersectTouchAction(TouchAction ta1, TouchAction ta2)
> {
>   if (ta1 == None || ta2 == None)
>     return None;
>   if (ta1 == Auto)
>     return ta2;
>   if (ta2 == Auto)
>     return ta1;
>   if (!(ta1 & ta2))
>     return None;
>   return ta1 & ta2;
> }
> 
> This logic could be slightly simpler if we renumbered None to be 0 and Auto to
> be 1 (instead of the other way around as it is currently), but I don't think
it
> really matters (and having the default be value 0 is kind of nice).

Done.

https://codereview.chromium.org/103823003/diff/20001/Source/web/tests/data/to...
File Source/web/tests/data/touch-action-overflow.html (right):

https://codereview.chromium.org/103823003/diff/20001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-overflow.html:28: <div class='scroll ta-panx'
style='height:75px'>
On 2013/12/12 20:47:24, Rick Byers wrote:
> These two tests really test the same code paths as tests in 'simple' (just
> intersecting pan-x with auto or pan-y).  I think the key thing to test here is
> that the scroller behaves as a boundary for the pan values (not just for
'none'
> as tested above).  So for example, have an outer div with ta-panx and a
scroller
> with ta-pany.  One case like that is probably sufficient since there really
> isn't distinct code paths involved here.

Done.

https://codereview.chromium.org/103823003/diff/20001/Source/web/tests/data/to...
File Source/web/tests/data/touch-action-simple.html (right):

https://codereview.chromium.org/103823003/diff/20001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-simple.html:11: <div class='ta-panx'
expected-action='pan-x'>Simple div with touch-action: pan-x</div>
On 2013/12/12 20:47:24, Rick Byers wrote:
> Please add a separate test case / input file for the pan cases.  These are no
> longer really "simple" and breaking it up makes it easier to prioritize and
> debug failures.
> 
> I don't have a strong opinion about the overflow scroll cases - they could
> either stay in the scroll test case or move to the pan test case.

Done.

https://codereview.chromium.org/103823003/diff/20001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-simple.html:35: Explicitly setting
touch-action: pan-y does nothing to prevent propagation of none
On 2013/12/12 20:47:24, Rick Byers wrote:
> It's probably overkill to list each possible combination here, but we should
try
> to hit some of the key different branches in the code (most of which aren't
> covered here).  So I'd suggest maybe:
>  - outer=ta-none, inner=ta-panx (as you have above)
>  - outer=ta-pany, inner=none
>  - outer=ta-panx, inner=ta-pany (as you have below)
>  - outer=ta-panxy, inner=ta-auto
>  - outer=ta-panx, inner=ta-panxy
>  - outer=ta-panxy, inner=ta-pany
>  - maybe one 3-level case, like outer=ta-panx middle=ta-auto inner=ta-pany

Done.

https://codereview.chromium.org/103823003/diff/20001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-simple.html:45: <div class='ta-panx'
style='height: 0; margin-bottom: 50px'>
On 2013/12/12 20:47:24, Rick Byers wrote:
> You don't need the 'height:- margin-bottom: 50px' hack in this case.  The
point
> of that hack is to ensure that a hit-testing bug that picked up the outer
> element instead of the inner one wouldn't produce the same result as what we
> expect.  In this case (and many of the other ones I suggested above) the
> expected action is different from that of the outer div, so they can just have
a
> normal containment relationship without risking any such bug.

Done.

https://codereview.chromium.org/103823003/diff/20001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-simple.html:47: Explicitly setting
touch-action: pan-y does nothing to prevent propagation of pan-x resulting to
none
On 2013/12/12 20:47:24, Rick Byers wrote:
> I don't think the 'prevent propagation of...' part of the description still
> applies to this case.  I'd simplify the description of this test to something
> like "Disjoint flags intersect to none".

Done.

https://codereview.chromium.org/103823003/diff/20001/Source/web/tests/data/to...
File Source/web/tests/data/touch-action-tests.css (right):

https://codereview.chromium.org/103823003/diff/20001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-tests.css:46: [expected-action=pan-x
pan-y]::before {
On 2013/12/12 20:47:24, Rick Byers wrote:
> Chrome claims this selector syntax is invalid.  Putting "pan-x pan-y" in
quotes
> seems to fix it.  Alternately it's probably simpler to just invent a token
like
> 'pan-x-y' for this case.

Done.

[Rick Byers, 2013-12-13 22:17:34]

LGTM with nit - thanks!

+eseidel for OWNERS

https://codereview.chromium.org/103823003/diff/80001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/103823003/diff/80001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3906: TouchAction
EventHandler::intersectTouchAction(TouchAction action, TouchAction
effectiveTouchAction)
nit: since the two parameters are equivalent (the function is symmetric on it's
inputs) I'd just call them 'action1' and 'action2'.  This makes it feel to me
like there's something special about the 2nd arg.

[gnana, 2013-12-16 11:41:09]

Thanks rick.
Applied review comment.

https://codereview.chromium.org/103823003/diff/80001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/103823003/diff/80001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3906: TouchAction
EventHandler::intersectTouchAction(TouchAction action, TouchAction
effectiveTouchAction)
On 2013/12/13 22:17:35, Rick Byers wrote:
> nit: since the two parameters are equivalent (the function is symmetric on
it's
> inputs) I'd just call them 'action1' and 'action2'.  This makes it feel to me
> like there's something special about the 2nd arg.

Done.

[eseidel, 2013-12-18 16:44:18]

lgtm

[commit-bot: I haz the power, 2013-12-18 17:12:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/103823003/100001

[commit-bot: I haz the power, 2013-12-18 18:17:27]

Change committed as 164108