Update touch-action hit-testing to match latest spec changes.

Update touch-action hit-testing to match latest spec changes. touch-action now
applies to all elements with both width AND height properties.

Also fix application to SVG elements.

BUG=356215, 247396
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171207

[Zeeshan Qureshi, 2014-04-01 00:36:21]

Rick, can you do a first pass. img / svg cover inline replaceable elements so
shouldn't need to add extra tests for buttons, etc.

I'm not sure if it's worth it adding extra tests for table columns / rows.

[Rick Byers, 2014-04-02 20:25:20]

This looks really good overall, thanks!  Just a couple minor things.

https://codereview.chromium.org/220343002/diff/1/Source/core/page/EventHandle...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/220343002/diff/1/Source/core/page/EventHandle...
Source/core/page/EventHandler.cpp:3950: if (renderer->isBox()) {
this should again look only at nodes to which touch action applies - probably
use visibleForTouchAction again here.

https://codereview.chromium.org/220343002/diff/1/Source/web/tests/TouchAction...
File Source/web/tests/TouchActionTest.cpp (right):

https://codereview.chromium.org/220343002/diff/1/Source/web/tests/TouchAction...
Source/web/tests/TouchActionTest.cpp:172: webView->resize(WebSize(800, 1600));
Check to see if this is really needed - I believe you'll get a pretty clear
ASSERT if you need to actually make it bigger.

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

https://codereview.chromium.org/220343002/diff/1/Source/web/tests/data/touch-...
Source/web/tests/data/touch-action-simple.html:54: <div style='height: 0;
margin-bottom: 50px'>
remove this div. Since you're not specifying a touch-action here I don't think
you need it (or if you do, it doesn't need any styling).

https://codereview.chromium.org/220343002/diff/1/Source/web/tests/data/touch-...
Source/web/tests/data/touch-action-simple.html:66: <div style='height: 0;
margin-bottom: 50px'>
ditto for above

https://codereview.chromium.org/220343002/diff/1/Source/web/tests/data/touch-...
Source/web/tests/data/touch-action-simple.html:74: </div>
probably need a simple column or row case

[Zeeshan Qureshi, 2014-04-04 01:12:30]

https://codereview.chromium.org/220343002/diff/1/Source/core/page/EventHandle...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/220343002/diff/1/Source/core/page/EventHandle...
Source/core/page/EventHandler.cpp:3950: if (renderer->isBox()) {
On 2014/04/02 20:25:20, Rick Byers wrote:
> this should again look only at nodes to which touch action applies - probably
> use visibleForTouchAction again here.

Done, the row / column case test this and I verified that it was failing
previously.

https://codereview.chromium.org/220343002/diff/1/Source/web/tests/TouchAction...
File Source/web/tests/TouchActionTest.cpp (right):

https://codereview.chromium.org/220343002/diff/1/Source/web/tests/TouchAction...
Source/web/tests/TouchActionTest.cpp:172: webView->resize(WebSize(800, 1600));
On 2014/04/02 20:25:20, Rick Byers wrote:
> Check to see if this is really needed - I believe you'll get a pretty clear
> ASSERT if you need to actually make it bigger.

This wasn't needed after all.

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

https://codereview.chromium.org/220343002/diff/1/Source/web/tests/data/touch-...
Source/web/tests/data/touch-action-simple.html:54: <div style='height: 0;
margin-bottom: 50px'>
On 2014/04/02 20:25:20, Rick Byers wrote:
> remove this div. Since you're not specifying a touch-action here I don't think
> you need it (or if you do, it doesn't need any styling).

Done.

https://codereview.chromium.org/220343002/diff/1/Source/web/tests/data/touch-...
Source/web/tests/data/touch-action-simple.html:66: <div style='height: 0;
margin-bottom: 50px'>
On 2014/04/02 20:25:20, Rick Byers wrote:
> ditto for above

Done.

https://codereview.chromium.org/220343002/diff/1/Source/web/tests/data/touch-...
Source/web/tests/data/touch-action-simple.html:74: </div>
On 2014/04/02 20:25:20, Rick Byers wrote:
> probably need a simple column or row case

Added.

[Rick Byers, 2014-04-04 01:22:27]

LGTM (with nit), thanks!

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

https://codereview.chromium.org/220343002/diff/40001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3945: // and exclude any prohibited actions.
For now this is trivial, but when we
nit: This "for now this is trivial" comment is old (from my first CL where we
had only auto/none), can you please remove it while you're here?

[Zeeshan Qureshi, 2014-04-04 01:37:30]

+jamesr for OWNERS.

[Zeeshan Qureshi, 2014-04-04 02:39:02]

+leviw for OWNERS

[leviw_travelin_and_unemployed, 2014-04-07 18:57:42]

https://codereview.chromium.org/220343002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/220343002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.h:90: bool visibleForTouchAction() const
OVERRIDE
I don't think this makes sense to live in RenderBox as a virtual method anymore,
at least as written. RenderBoxes will never be non-replaced inlines
(non-replaced inlines can flow through different lines, which means they aren't
a box). This should either only be checking isTableRow/isRenderTableCol, or all
this logic should simply be pulled into RenderObject as a non-virtual function.

[Zeeshan Qureshi, 2014-04-07 19:08:23]

On 2014/04/07 18:57:42, Levi wrote:
>
https://codereview.chromium.org/220343002/diff/60001/Source/core/rendering/Re...
> File Source/core/rendering/RenderBox.h (right):
> 
>
https://codereview.chromium.org/220343002/diff/60001/Source/core/rendering/Re...
> Source/core/rendering/RenderBox.h:90: bool visibleForTouchAction() const
> OVERRIDE
> I don't think this makes sense to live in RenderBox as a virtual method
anymore,
> at least as written. RenderBoxes will never be non-replaced inlines
> (non-replaced inlines can flow through different lines, which means they
aren't
> a box). This should either only be checking isTableRow/isRenderTableCol, or
all
> this logic should simply be pulled into RenderObject as a non-virtual
function.

I think putting it in RenderObject should still respect the spec, I'll do that.

[Rick Byers, 2014-04-07 19:41:27]

On 2014/04/07 19:24:08, Zeeshan Qureshi wrote:

This still LGTM, thanks!

[leviw_travelin_and_unemployed, 2014-04-07 22:21:45]

Works for me. LGTM.

[Zeeshan Qureshi, 2014-04-07 22:25:40]

+ojan for web/tests

[eseidel, 2014-04-09 22:12:05]

lgtm

[commit-bot: I haz the power, 2014-04-09 22:12:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zeeshanq@chromium.org/220343002/70001

[ojan, 2014-04-09 22:20:12]

lgtm

Please move the function body to the cpp file before committing. Sorry to keep
you waiting.

https://codereview.chromium.org/220343002/diff/70001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/220343002/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:955: bool visibleForTouchAction() const
This function body is long for a header file. Can you move it to the cpp file? I
expect the compiler will inline it either way since it's only called in one
place.

https://codereview.chromium.org/220343002/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:957: if (isInline() && !isReplaced())
Makes me a little sad that we can't use inheritance for this, but looking at the
places we call setInline and setReplaced...it's a total mess that doesn't match
our inheritance hierarchy, so lgtm.

[Zeeshan Qureshi, 2014-04-09 22:38:06]

The CQ bit was unchecked by zeeshanq@chromium.org

[Zeeshan Qureshi, 2014-04-09 23:39:41]

The CQ bit was checked by zeeshanq@chromium.org

[commit-bot: I haz the power, 2014-04-09 23:39:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zeeshanq@chromium.org/220343002/90001

[commit-bot: I haz the power, 2014-04-10 00:50:16]

Change committed as 171207