Select best target for tap gesture.

• Component: UI Events
• Status: RESOLVED FIXED

[Allan Sandfeld Jensen]

All the mobile browsers use area-based hit testing for the tap-gesture (touch click). The code for the rect-based hit testing is already in WebCore, but the code for selecting which of the returned nodes should be the target is not.

Since there are different philosophies and implementations for this, the code should be flexible enough to accommodate different approaches and be skippable by those that have existing implementations on a different layer.

[Allan Sandfeld Jensen]

Created attachment 127359 [details]
Math patch

An extension of the math for the graphics classes.

[Kenneth Rohde Christiansen]

Comment on attachment 127359 [details]
Math patch

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

> Source/WebCore/platform/graphics/FloatPoint.h:246
> +inline float FloatPoint::distanceToPoint(const FloatPoint& p) const

wouldnt calling it 'other' be more consistent?

> Source/WebCore/platform/graphics/FloatQuad.h:92
> +    FloatPoint center() const { return FloatPoint((m_p1.x()+m_p2.x()+m_p3.x()+m_p4.x())/4., (m_p1.y()+m_p2.y()+m_p3.y()+m_p4.y())/4.); }

There are some coding style violations here. Did you run the script locally?

[Allan Sandfeld Jensen]

Created attachment 127360 [details]
Meat Patch

[Allan Sandfeld Jensen]

Created attachment 127364 [details]
Math Patch2

Coding style.

[Allan Sandfeld Jensen]

(In reply to comment #2)
> There are some coding style violations here. Did you run the script locally?

Yes, but apparently it doesn't quite work in git mode. I needed to check the changes in work-dir.

[Kenneth Rohde Christiansen]

Adam, if anyone working on Chrome are interested in this, can you cc them?

[Simon Hausmann]

*** Bug 49057 has been marked as a duplicate of this bug. ***

[Adam Barth]

@Grace, I wanted to CC olilan, but I didn't know his bugzilla account.

[Antonio Gomes]

Comment on attachment 127360 [details]
Meat Patch

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

Good stuff. See Webkit/blackberry/WebKitSupport/FatFingers.cpp|h for reference.

see some comments bellow...

> Source/WebCore/page/EventHandler.cpp:2400
> +    HitTestRequest::HitTestRequestType hitType = HitTestRequest::ReadOnly | HitTestRequest::Active;
> +    IntRect touchRect(center-radius, IntSize(radius.width()*2, radius.height()*2));

is not it better to use this (HitTestResults.h):

154	// Formula:
155	// x = p.x() - rightPadding
156	// y = p.y() - topPadding
157	// width = leftPadding + rightPadding + 1
158	// height = topPadding + bottomPadding + 1
159	inline LayoutRect HitTestResult::rectForPoint(const LayoutPoint& point) const

> Source/WebCore/page/EventHandler.cpp:2411
> +    HitTestResultMetrics::HitList hitlist;
> +    HitTestResultMetrics::compileHitList(result, hitlist, HitTestResultMetrics::nodeRespondingToTapGesture);
> +    if (HitTestResultMetrics::findClosestNode(targetNode, targetPoint, center, touchRect, hitlist)) {

could not this part be encapsulated withing HitTestResultMetrics?

> Source/WebCore/platform/HitTestResultMetrics.cpp:44
> +            // ### implement hasDefaultEventHandler and use that instead of all of these above calls?

Use FIXME.

[Allan Sandfeld Jensen]

(In reply to comment #9)
> (From update of attachment 127360 [details])
> > Source/WebCore/page/EventHandler.cpp:2411
> > +    HitTestResultMetrics::HitList hitlist;
> > +    HitTestResultMetrics::compileHitList(result, hitlist, HitTestResultMetrics::nodeRespondingToTapGesture);
> > +    if (HitTestResultMetrics::findClosestNode(targetNode, targetPoint, center, touchRect, hitlist)) {
> 
> could not this part be encapsulated withing HitTestResultMetrics?
> 

It could, but the point was to allow for a configurable combination of NodeFilters and Metrics to use. By exposing the step-wise functions I believe it would be easier to reuse the code for different heuristics.

[Antonio Gomes]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 127360 [details] [details])
> > > Source/WebCore/page/EventHandler.cpp:2411
> > > +    HitTestResultMetrics::HitList hitlist;
> > > +    HitTestResultMetrics::compileHitList(result, hitlist, HitTestResultMetrics::nodeRespondingToTapGesture);
> > > +    if (HitTestResultMetrics::findClosestNode(targetNode, targetPoint, center, touchRect, hitlist)) {
> > 
> > could not this part be encapsulated withing HitTestResultMetrics?
> > 
> 
> It could, but the point was to allow for a configurable combination of NodeFilters and Metrics to use. By exposing the step-wise functions I believe it would be easier to reuse the code for different heuristics.

Do you have any other use in mind? Otherwise, it would be better to move it, I would say.

[Antonio Gomes]

Comment on attachment 127360 [details]
Meat Patch

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

> Source/WebCore/platform/HitTestResultMetrics.cpp:62
> +Node* nodeRespondingToTapGesture(Node* node)
> +{
> +    for (; node; node = node->parentOrHostNode()) {
> +        if (node->isLink()
> +            || node->isContentEditable()
> +            || node->isMouseFocusable())
> +            // ### implement hasDefaultEventHandler and use that instead of all of these above calls?
> +            return node;
> +        if (node->hasEventListeners() &&
> +                (node->hasEventListeners(eventNames().clickEvent)
> +                || node->hasEventListeners(eventNames().DOMActivateEvent)
> +                || node->hasEventListeners(eventNames().mousedownEvent)
> +                || node->hasEventListeners(eventNames().mouseupEvent)
> +                || node->hasEventListeners(eventNames().mousemoveEvent)
> +                 // Checking for focus events is not necessary since they can only fire on
> +                 // focusable elements which have been captured above.
> +            ))
> +            return node;
> +        if (node->renderStyle()) {
> +            // Accept nodes that has a CSS effect when touched.
> +            if (node->renderStyle()->affectedByActiveRules() || node->renderStyle()->affectedByHoverRules())
> +                return node;
> +        }
> +    }
> +    return 0;

can not traversing the DOM tree upwards like this be expensive and repetitive?

Also the event handling does bubble up and bubble down in the hierarchy to find an event target that can handle an event, iirc.

[Allan Sandfeld Jensen]

(In reply to comment #12)
> (From update of attachment 127360 [details])
> can not traversing the DOM tree upwards like this be expensive and repetitive?
> 
It can. The problem is that we need figure out if an element responds to touch-events or not, and since they can be handled by ancestors that requires checking them. It can be optimized to not check common ancestors more than once, or do width-first checking. There are several good options for optimizing this, but before doing that I want to ensure the functionality is correct and acceptable.

> Also the event handling does bubble up and bubble down in the hierarchy to find an event target that can handle an event, iirc.

The problem is that very little event-handling on the web cancels the events they handle. So just emitting them on the best target and set default to re-emit the event on the second best target would probably cause a lot of event-handlers to see multiple events. Alternatively we would need something to tell us if any handlers have been activated on the event, and use that to determine if it should be re-emitted on more targets. 

This code though is more similar to what I have seen in our N9 code, and in the Chromium, Android and Blackberry codebases.

[Allan Sandfeld Jensen]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (From update of attachment 127360 [details] [details] [details])
> > > > Source/WebCore/page/EventHandler.cpp:2411
> > > > +    HitTestResultMetrics::HitList hitlist;
> > > > +    HitTestResultMetrics::compileHitList(result, hitlist, HitTestResultMetrics::nodeRespondingToTapGesture);
> > > > +    if (HitTestResultMetrics::findClosestNode(targetNode, targetPoint, center, touchRect, hitlist)) {
> > > 
> > > could not this part be encapsulated withing HitTestResultMetrics?
> > > 
> > 
> > It could, but the point was to allow for a configurable combination of NodeFilters and Metrics to use. By exposing the step-wise functions I believe it would be easier to reuse the code for different heuristics.
> 
> Do you have any other use in mind? Otherwise, it would be better to move it, I would say.

Yes, one more at least. For scroll gestures I expect to filter on scrollable first, and then choose the most intersecting or smallest containing qualified node.

Of course you could still move the different combination of calls and just do the code reuse internally. I have no strong preference either way.

[Antonio Gomes]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 127360 [details] [details])
> > can not traversing the DOM tree upwards like this be expensive and repetitive?
> > 
> It can. The problem is that we need figure out if an element responds to touch-events or not, and since they can be handled by ancestors that requires checking them. It can be optimized to not check common ancestors more than once, or do width-first checking. There are several good options for optimizing this, but before doing that I want to ensure the functionality is correct and acceptable.

Alternatively, you could considering doing this:

HitTestResult::addNodeToRectBasedTestResult instead of stopping the rect-hittest if current node rect contains the "finger rect", you just keep going up in the *render* tree hierarchy.

HitTestResult::addNodeToRectBasedTestResult(..)
{
    (...)
    return !rect.contains(rectForPoint(pointInContainer)); <---!
}

Traversing the render tree can (should?) be faster than DOM tree.

[Allan Sandfeld Jensen]

(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (From update of attachment 127360 [details] [details] [details])
> > > can not traversing the DOM tree upwards like this be expensive and repetitive?
> > > 
> > It can. The problem is that we need figure out if an element responds to touch-events or not, and since they can be handled by ancestors that requires checking them. It can be optimized to not check common ancestors more than once, or do width-first checking. There are several good options for optimizing this, but before doing that I want to ensure the functionality is correct and acceptable.
> 
> Alternatively, you could considering doing this:
> 
> HitTestResult::addNodeToRectBasedTestResult instead of stopping the rect-hittest if current node rect contains the "finger rect", you just keep going up in the *render* tree hierarchy.
> 
> HitTestResult::addNodeToRectBasedTestResult(..)
> {
>     (...)
>     return !rect.contains(rectForPoint(pointInContainer)); <---!
> }
> 
> Traversing the render tree can (should?) be faster than DOM tree.

The render tree does not necessarily have the same structure as the DOM tree, so it can not make the same guarantees. As long as there are fast options for traversing the DOM tree, there is no reason to traverse the wrong tree.

[Allan Sandfeld Jensen]

Created attachment 128178 [details]
Patch

Combined patch, which also improves API and algorithmic performance.

[Kenneth Rohde Christiansen]

Comment on attachment 128178 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2368
> +        nodeForTapGestureAtTouchPoint(gestureEvent.position(), IntSize(gestureEvent.area().width() / 2, gestureEvent.area().height() / 2), adjustedPoint, targetNode);

Can you split out IntSize(gestureEvent.area().width() / 2, gestureEvent.area().height() / 2) to give it a proper name such as 'center' point. We might not always want to use the exact center as people tend to believe that they hit a tad higher.

> Source/WebCore/page/EventHandler.cpp:2383
> +bool EventHandler::handleGestureScrollUpdate(const PlatformGestureEvent& gestureEvent)

Why is this part of this patch?

> Source/WebCore/page/EventHandler.cpp:2385
> +    const float tickDivisor = (float)WheelEvent::tickMultiplier;

c++ casts?

> Source/WebCore/page/EventHandler.cpp:2395
> +void EventHandler::nodeForTapGestureAtTouchPoint(const IntPoint& center, const IntSize& radius, IntPoint& targetPoint, Node*& targetNode)

what node? activeableNode? targetNode ? It could be a bit more clear

> Source/WebCore/page/EventHandler.cpp:2398
> +    IntRect touchRect(center-radius, IntSize(radius.width()*2, radius.height()*2));

style spaces around *, - etc

> Source/WebCore/page/EventHandler.cpp:2401
> +    HitTestResult result = hitTestResultAtPoint(center, /*allowShadowContent*/ false, false, DontHitTestScrollbars, hitType, radius);

What does the second false represent?

> Source/WebCore/platform/HitTestResultMetrics.cpp:39
> +// Takes non-const Node* because isContentEditable is a non-const function.
> +bool nodeRespondsToTapGesture(Node* node)

what about drag and drop?

> Source/WebCore/platform/HitTestResultMetrics.cpp:64
> +static inline void appendAbsoluteQuadsForNodeToHitList(Node* node, HitList& hitlist)

append AbsoluteNodeQuadsToHitList?

> Source/WebCore/platform/HitTestResultMetrics.cpp:73
> +    const Vector<FloatQuad>::const_iterator itend = quads.end();

we normally just call it 'end' - let's be consistent

> Source/WebCore/platform/HitTestResultMetrics.cpp:89
> +    const HitTestResult::NodeSet::const_iterator itend = intersectedNodes.end();

end*

> Source/WebCore/platform/HitTestResultMetrics.cpp:96
> +        Node* responder = 0;

respondee ?

> Source/WebCore/platform/HitTestResultMetrics.cpp:137
> +// Returns the distance squared to the bounding box centerline..

trailing .

> Source/WebCore/platform/HitTestResultMetrics.cpp:140
> +    // Using the bounding box means the distance to centerline is less meaningfull for transformed rectangles

meaningful has only one l

> Source/WebCore/platform/HitTestResultMetrics.cpp:142
> +    // but calculating the distance to quads can be very expensive.
> +    IntRect rect = hitTestQuad.boundingBox();

Did you measure this? Is it really a problem? This is not that common anyways

> Source/WebCore/platform/HitTestResultMetrics.cpp:148
> +float overlapWithHitTestQuad(const LayoutPoint&, const LayoutRect& touchRect, const HitTestQuad& hitTestQuad)

Can the name make the return value more obvious? overlapRatio ?

> Source/WebCore/platform/HitTestResultMetrics.h:62
> +
> +typedef bool (*NodeFilter)(Node*);
> +bool nodeRespondsToTapGesture(Node*);
> +
> +void compileHitList(const HitTestResult&, HitList&, NodeFilter);
> +
> +typedef float (*Metric)(const LayoutPoint&, const LayoutRect&, const HitTestQuad&);
> +float distanceToHitTestQuad(const LayoutPoint& touchHotspot, const LayoutRect& touchArea, const HitTestQuad&);
> +float overlapWithHitTestQuad(const LayoutPoint& touchHotspot, const LayoutRect& touchArea, const HitTestQuad&);
> +
> +bool findNodeWithLowestMetric(Node*& targetNode, LayoutPoint& targetPoint, const LayoutPoint& touchHotspot, const LayoutRect& touchArea, HitList& hitlist, Metric);
> +bool findNodeWithHighestMetric(Node*& targetNode, LayoutPoint& targetPoint, const LayoutPoint& touchHotspot, const LayoutRect& touchArea, HitList& hitlist, Metric);
> +

Are all these supposed to be exposed?

> Source/WebCore/platform/graphics/FloatSize.h:101
> +    float area() const
> +    {
> +        return m_width * m_height;
> +    }

I don't know if area is a good name...

[WebKit Review Bot]

Comment on attachment 128178 [details]
Patch

Attachment 128178 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11558469

[Allan Sandfeld Jensen]

(In reply to comment #18)
> (From update of attachment 128178 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128178&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:2368
> > +        nodeForTapGestureAtTouchPoint(gestureEvent.position(), IntSize(gestureEvent.area().width() / 2, gestureEvent.area().height() / 2), adjustedPoint, targetNode);
> 
> Can you split out IntSize(gestureEvent.area().width() / 2, gestureEvent.area().height() / 2) to give it a proper name such as 'center' point. We might not always want to use the exact center as people tend to believe that they hit a tad higher.
> 
The code is ready for changing the IntSize touchArea to an IntRect touchSurface later, but at the moment the hotspot is the center, and no additional information is currently available from touch points.

> > Source/WebCore/page/EventHandler.cpp:2383
> > +bool EventHandler::handleGestureScrollUpdate(const PlatformGestureEvent& gestureEvent)
> 
> Why is this part of this patch?
> 
Because I refactored handleGestureEvent to separate handleGestureTap out, this is a byproduct of the refactoring.

> > Source/WebCore/page/EventHandler.cpp:2385
> > +    const float tickDivisor = (float)WheelEvent::tickMultiplier;
> 
> c++ casts?
> 
Why would you object cast an integral type? Besides it is not part of my patch, it has only been moved.

> > Source/WebCore/page/EventHandler.cpp:2395
> > +void EventHandler::nodeForTapGestureAtTouchPoint(const IntPoint& center, const IntSize& radius, IntPoint& targetPoint, Node*& targetNode)
> 
> what node? activeableNode? targetNode ? It could be a bit more clear
> 
> > Source/WebCore/page/EventHandler.cpp:2398
> > +    IntRect touchRect(center-radius, IntSize(radius.width()*2, radius.height()*2));
> 
> style spaces around *, - etc
> 
Good catch, for some reason this was not caught by the style-checker.

> > Source/WebCore/platform/HitTestResultMetrics.cpp:39
> > +// Takes non-const Node* because isContentEditable is a non-const function.
> > +bool nodeRespondsToTapGesture(Node* node)
> 
> what about drag and drop?
> 
Will be handled later. This is a patch for handling the tap gestures. Also to be handled later is scroll gestures and rotate gestures.


> > Source/WebCore/platform/HitTestResultMetrics.cpp:64
> > +static inline void appendAbsoluteQuadsForNodeToHitList(Node* node, HitList& hitlist)
> 
> append AbsoluteNodeQuadsToHitList?
> 
Sure.

> > Source/WebCore/platform/HitTestResultMetrics.cpp:73
> > +    const Vector<FloatQuad>::const_iterator itend = quads.end();
> 
> we normally just call it 'end' - let's be consistent
> 
Isn't that name likely to cause name collisions?

> > Source/WebCore/platform/HitTestResultMetrics.cpp:89
> > +    const HitTestResult::NodeSet::const_iterator itend = intersectedNodes.end();
> 
> end*
> 
> > Source/WebCore/platform/HitTestResultMetrics.cpp:96
> > +        Node* responder = 0;
> 
> respondee ?
> 
No it is the respondingNode, or responder for short.

> > Source/WebCore/platform/HitTestResultMetrics.cpp:142
> > +    // but calculating the distance to quads can be very expensive.
> > +    IntRect rect = hitTestQuad.boundingBox();
> 
> Did you measure this? Is it really a problem? This is not that common anyways
>
It is mathematically complicated and computationally much more expensive. On top of that the results returned from hittestresult which are the input to these functions are not based on quad-based hit-testing anyway. So if we desire better accuracy for transformed objects ALL rect-based hit-testing in the render-tree needs to rewritten. 
 
> > Source/WebCore/platform/HitTestResultMetrics.cpp:148
> > +float overlapWithHitTestQuad(const LayoutPoint&, const LayoutRect& touchRect, const HitTestQuad& hitTestQuad)
> 
> Can the name make the return value more obvious? overlapRatio ?
> 
AreaOfIntersection would perhaps be more accurate.

> > Source/WebCore/platform/HitTestResultMetrics.h:62
> > +
> > +typedef bool (*NodeFilter)(Node*);
> > +bool nodeRespondsToTapGesture(Node*);
> > +
> > +void compileHitList(const HitTestResult&, HitList&, NodeFilter);
> > +
> > +typedef float (*Metric)(const LayoutPoint&, const LayoutRect&, const HitTestQuad&);
> > +float distanceToHitTestQuad(const LayoutPoint& touchHotspot, const LayoutRect& touchArea, const HitTestQuad&);
> > +float overlapWithHitTestQuad(const LayoutPoint& touchHotspot, const LayoutRect& touchArea, const HitTestQuad&);
> > +
> > +bool findNodeWithLowestMetric(Node*& targetNode, LayoutPoint& targetPoint, const LayoutPoint& touchHotspot, const LayoutRect& touchArea, HitList& hitlist, Metric);
> > +bool findNodeWithHighestMetric(Node*& targetNode, LayoutPoint& targetPoint, const LayoutPoint& touchHotspot, const LayoutRect& touchArea, HitList& hitlist, Metric);
> > +
> 
> Are all these supposed to be exposed?
> 
Sort of. They are not currently used and could be hidden, but it does serve a purpose of making it possible for fat-finger code for other platforms to use it, even if they want to do these tests higher in the API. 

> > Source/WebCore/platform/graphics/FloatSize.h:101
> > +    float area() const
> > +    {
> > +        return m_width * m_height;
> > +    }
> 
> I don't know if area is a good name...

What else would just call the area of a rectangle?

[Simon Hausmann]

Comment on attachment 128178 [details]
Patch

I would love to see layout tests for this, too, to cover at least the basics. You could use Internals.idl for this, similar to what I tried when I looked at this issue:

https://github.com/tronical/webkit/commit/e80076f360e69511ce9291b98cb16228c06b4ac0

[Allan Sandfeld Jensen]

(In reply to comment #21)
> (From update of attachment 128178 [details])
> I would love to see layout tests for this, too, to cover at least the basics. You could use Internals.idl for this, similar to what I tried when I looked at this issue:
> 
> https://github.com/tronical/webkit/commit/e80076f360e69511ce9291b98cb16228c06b4ac0

Sounds like a good idea. I will add layout tests and fix the coding style, hopefully clarify the code too.

[Antonio Gomes]

When adding layout tests, it would be great to consider this codepath, and custom touch target detection, platform specific.

[Allan Sandfeld Jensen]

Created attachment 128484 [details]
Patch

Added automatic testing as suggested by Simon Hausmann. 

Corrected several cases of coding style problems, and removed functions not used outside of the code from the header file as suggested by Kenneth Rohde. 

Renamed the new file to better describe its new function.

Made the feature a compile-time define to avoid accidentally breaking other platforms with similar features implemented elsewhere and to fix compilation issues as reported by ReviewBot.

[Antonio Gomes]

Comment on attachment 128484 [details]
Patch

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

did not yet review the code change, just some questions after a brief look

> Source/JavaScriptCore/wtf/Platform.h:1039
> +#if PLATFORM(QT)

I think it should be like #if  !defined(ENABLE_TOUCH_ADJUSTMENT) && PLATFORM(QT) ?

> Source/WebCore/ChangeLog:25
> +        * Target.pri:

you are only adding it to one build system. I know it has the compile flag, but maybe it is a practice to add to all build systems?

> Source/WebCore/page/EventHandler.cpp:-2353
> -    case PlatformEvent::GestureTap: {
> -        // FIXME: Refactor this code to not hit test multiple times once hit testing has been corrected as suggested above.
> -        PlatformMouseEvent fakeMouseMove(gestureEvent.position(), gestureEvent.globalPosition(), NoButton, PlatformEvent::MouseMoved, /* clickCount */ 1, gestureEvent.shiftKey(), gestureEvent.ctrlKey(), gestureEvent.altKey(), gestureEvent.metaKey(), gestureEvent.timestamp());
> -        PlatformMouseEvent fakeMouseDown(gestureEvent.position(), gestureEvent.globalPosition(), LeftButton, PlatformEvent::MousePressed, /* clickCount */ 1, gestureEvent.shiftKey(), gestureEvent.ctrlKey(), gestureEvent.altKey(), gestureEvent.metaKey(), gestureEvent.timestamp());
> -        PlatformMouseEvent fakeMouseUp(gestureEvent.position(), gestureEvent.globalPosition(), LeftButton, PlatformEvent::MouseReleased, /* clickCount */ 1, gestureEvent.shiftKey(), gestureEvent.ctrlKey(), gestureEvent.altKey(), gestureEvent.metaKey(), gestureEvent.timestamp());
> -        mouseMoved(fakeMouseMove);
> -        handleMousePressEvent(fakeMouseDown);
> -        handleMouseReleaseEvent(fakeMouseUp);
> -        return true;

maybe one patch to move it to its own method , and them adding the #if touch_adjustment bits?

> Source/WebCore/page/EventHandler.cpp:-2361
> -        const float tickDivisor = (float)WheelEvent::tickMultiplier;
> -        // FIXME: Replace this interim implementation once the above fixme has been addressed.
> -        IntPoint point(gestureEvent.position().x(), gestureEvent.position().y());
> -        IntPoint globalPoint(gestureEvent.globalPosition().x(), gestureEvent.globalPosition().y());
> -        PlatformWheelEvent syntheticWheelEvent(point, globalPoint, gestureEvent.deltaX(), gestureEvent.deltaY(), gestureEvent.deltaX() / tickDivisor, gestureEvent.deltaY() / tickDivisor, ScrollByPixelWheelEvent, gestureEvent.shiftKey(), gestureEvent.ctrlKey(), gestureEvent.altKey(), gestureEvent.metaKey());
> -        handleWheelEvent(syntheticWheelEvent);

ditto

> Source/WebCore/page/EventHandler.cpp:2407
> +    HitTestResult result = hitTestResultAtPoint(touchCenter, /*allowShadowContent*/ false, /*ignoreClipping*/ false, DontHitTestScrollbars, hitType, touchRadius);

side note: "AtPoint" of this method does not make total sense anymore with the 'padding' addition

[Kenneth Rohde Christiansen]

Comment on attachment 128484 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2368
> +    #if ENABLE(TOUCH_ADJUSTMENT)

These are never indended! accoding to coding style... not even if nested. Also maybe it should be a USE as it is a feature, but you should probably verify that with someone else

> Source/WebCore/platform/TouchAdjustment.cpp:34
> +namespace WebCore {

It has namespace WebCore and it is in platform. WebCore/platform is supposed to move to Platform/ at some point and be used by WebCore and not the other way around. Maybe we should move this out of platform.

[Philippe Normand]

Comment on attachment 128484 [details]
Patch

Attachment 128484 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11610302

[WebKit Review Bot]

Comment on attachment 128484 [details]
Patch

Attachment 128484 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11600375

New failing tests:
touchadjustment/nested-touch.html
touchadjustment/touch-inlines.html
touchadjustment/event-triggered-widgets.html

[Allan Sandfeld Jensen]

Created attachment 128719 [details]
Patch

Moved touch-adjustment to WebCore/page and found out how to set JS-feature flags which should help fix builds on other platforms.

[Allan Sandfeld Jensen]

Created attachment 128728 [details]
Patch

Added expected results for chromium and resubmitting after blocking patch has landed.

[Allan Sandfeld Jensen]

Created attachment 128733 [details]
Patch

[Antonio Gomes]

Comment on attachment 128733 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2404
> +void EventHandler::nodeForTapGestureAtTouchPoint(const IntPoint& touchCenter, const IntSize& touchRadius, IntPoint& targetPoint, Node*& targetNode)

I think we can omit the "tap gesture" part in the name.

It is a public method, so it can be called directly. Ports like the BlackBerry do a target detection at mouse_down instead of at a tap gesture.

> Source/WebCore/page/EventHandler.cpp:2412
> +    findBestCandidateForGestureTap(targetNode, targetPoint, touchCenter, touchRect, result);

same here

> Source/WebCore/page/TouchAdjustment.cpp:47
> +    Node* m_node;

What guarantees here that the node does get destroyed somewhere else (by JS for example) and you have a dangling pointer.

> Source/WebCore/page/TouchAdjustment.cpp:56
> +bool nodeRespondsToTapGesture(Node* node)

again, not only for "tap gestures"

> Source/WebCore/page/TouchAdjustment.cpp:167
> +    IntRect rect = hitTestQuad.boundingBox();
> +
> +    return rect.distanceSquaredFromCenterLineToPoint(touchHotspot);

drop blank line here

> Source/WebCore/page/TouchAdjustment.cpp:222
> +void findBestCandidateForGestureTap(Node*& targetNode, LayoutPoint& targetPoint, const LayoutPoint& touchHotspot, const LayoutRect& touchArea, const HitTestResult& hitTestResult)

ditto (naming)

[Antonio Gomes]

Comment on attachment 128733 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:560
> +PassRefPtr<WebKitPoint> Internals::touchPositionAdjustedToBestNodeForGestureTap(long x, long y, long w, long h, Document* document, ExceptionCode& ec)
> +{
> +    if (!document || !document->frame()) {
> +        ec = INVALID_ACCESS_ERR;
> +        return 0;
> +    }
> +
> +    IntSize radius(w / 2, h / 2);
> +    IntPoint point(x + radius.width(), y + radius.height());
> +
> +    Node* targetNode;
> +    IntPoint adjustedPoint;
> +
> +    document->frame()->eventHandler()->nodeForTapGestureAtTouchPoint(point, radius, adjustedPoint, targetNode);
> +
> +    return WebKitPoint::create(adjustedPoint.x(), adjustedPoint.y());
> +}
> +
> +PassRefPtr<Node> Internals::touchNodeAdjustedToBestNodeForGestureTap(long x, long y, long w, long h, Document* document, ExceptionCode& ec)
> +{
> +    if (!document || !document->frame()) {
> +        ec = INVALID_ACCESS_ERR;
> +        return 0;
> +    }
> +
> +    IntSize radius(w / 2, h / 2);
> +    IntPoint point(x + radius.width(), y + radius.height());
> +
> +    Node* targetNode;
> +    IntPoint adjustedPoint;
> +
> +    document->frame()->eventHandler()->nodeForTapGestureAtTouchPoint(point, radius, adjustedPoint, targetNode);
> +
> +    return adoptRef(targetNode);

some common code that could be shared via a helper here.

[Allan Sandfeld Jensen]

(In reply to comment #32)
> (From update of attachment 128733 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128733&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:2404
> > +void EventHandler::nodeForTapGestureAtTouchPoint(const IntPoint& touchCenter, const IntSize& touchRadius, IntPoint& targetPoint, Node*& targetNode)
> 
> I think we can omit the "tap gesture" part in the name.
> 
> It is a public method, so it can be called directly. Ports like the BlackBerry do a target detection at mouse_down instead of at a tap gesture.
> 
The reason I added for-tap-gesture is to distinguish it from a function that adjust for scroll gesture or for text selection. This function in particular only looks for nodes that has the event-handlers that are emitted by the synthetic click emitted after a tap gesture, not for nodes that has scroll overflow or scroll event handlers.

It is the same reason I had the public API earlier where you could specify your own node-filter in case you wanted to use the same functions in a slightly different context.

I could change it to something like node-responding-to-synthetic-click, would that work for your case?

[Antonio Gomes]

> It is the same reason I had the public API earlier where you could specify your own node-filter in case you wanted to use the same functions in a slightly different context.
> 
> I could change it to something like node-responding-to-synthetic-click, would that work for your case?

Hum, not entirely sure yet.

Maybe node-responding-to-user-interaction or something more generic like best-clickable-target-for-point, clickable-node-for-point or adjustedNodeAndPositionForClicking?

[Allan Sandfeld Jensen]

Created attachment 129259 [details]
Patch

Renamed functions to not mention GestureTap. Changed the API in TouchAdjustment so it can also be used with the result from a Document::nodesFromRect call, and added handling and tests for <label> elements.

[WebKit Review Bot]

Attachment 129259 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/page/TouchAdjustment.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/page/TouchAdjustment.cpp:123:  Missing spaces around =  [whitespace/operators] [4]
Source/WebCore/page/TouchAdjustment.h:30:  Extra space before )  [whitespace/parens] [2]
Total errors found: 3 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 129259 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2405
> +void EventHandler::bestClickableNodeAtTouchPoint(const IntPoint& touchCenter, const IntSize& touchRadius, IntPoint& targetPoint, Node*& targetNode)

Isn't it For instead of At?

[Antonio Gomes]

Comment on attachment 129259 [details]
Patch

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

Looking really good! I have a question though:

> LayoutTests/touchadjustment/event-triggered-widgets.html:26
> +        do {
> +            pos.left += node.offsetLeft;
> +            pos.top += node.offsetTop;
> +        } while (node = node.offsetParent);

I think there is a JS helper for that somewhere.

>> Source/WebCore/page/EventHandler.cpp:2405
>> +void EventHandler::bestClickableNodeAtTouchPoint(const IntPoint& touchCenter, const IntSize& touchRadius, IntPoint& targetPoint, Node*& targetNode)
> 
> Isn't it For instead of At?

Agreed.

> Source/WebCore/page/TouchAdjustment.cpp:92
> +        if (enclosingBox && enclosingBox->scrollsOverflow())

I think scrollOverflow is not all you need here.

<div> content 0 </div>
<div id="scrollable1" style="position:relative; left: 50px; background: blue; color: #ffffff; padding: 4px; width: 100px; overflow: auto;">
     content 1
</div>
<div> content 2 </div>

In the above quoted HTML, that has a non-scrollable DIV with overflow:auto, do you get this DIV as the node-responding-to-scroll-gesture?

>> Source/WebCore/page/TouchAdjustment.cpp:123
>> +    for (unsigned int i=0; i < length; ++i) {
> 
> Missing spaces around =  [whitespace/operators] [4]

s/unsigned int/unsigned/g

[Antonio Gomes]

maybe renderbox::canBeScrolledAndHasScrollableArea

[Allan Sandfeld Jensen]

(In reply to comment #39)
> > Source/WebCore/page/TouchAdjustment.cpp:92
> > +        if (enclosingBox && enclosingBox->scrollsOverflow())
> 
> I think scrollOverflow is not all you need here.
> 
> <div> content 0 </div>
> <div id="scrollable1" style="position:relative; left: 50px; background: blue; color: #ffffff; padding: 4px; width: 100px; overflow: auto;">
>      content 1
> </div>
> <div> content 2 </div>
> 
> In the above quoted HTML, that has a non-scrollable DIV with overflow:auto, do you get this DIV as the node-responding-to-scroll-gesture?
> 
That function probably shouldn't have been in the patch. It is not yet hooked up to anything, and I can add to the problems that the event-listener it checks for is also wrong (is scrollEvent but should have been mousewheelEvent) ;)

So, we down to renaming functions with ..AtTouchPoint to ..ForTouchPoint, or ..ForTouch?

[Allan Sandfeld Jensen]

Created attachment 129887 [details]
Patch

Style fix and function renames.

[WebKit Review Bot]

Attachment 129887 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
LayoutTests/platform/chromium/test_expectations.txt:3442:  More specific entry on line 3442 overrides line 3133 fast/overflow/unreachable-overflow-rtl-bug.html  [test/expectations] [5]
Total errors found: 1 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Allan Sandfeld Jensen]

(In reply to comment #43)
> Attachment 129887 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
> LayoutTests/platform/chromium/test_expectations.txt:3442:  More specific entry on line 3442 overrides line 3133 fast/overflow/unreachable-overflow-rtl-bug.html  [test/expectations] [5]
> Total errors found: 1 in 41 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

False positive. Existing style bug in that file, or bug in check-webkit-style?

[Kenneth Rohde Christiansen]

Comment on attachment 129887 [details]
Patch

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

as I have seen this code and commented already in real life, it would be nice if someone else could do the actual review, anyway here are a few nits from skimming it.

> LayoutTests/touchadjustment/event-triggered-widgets.html:34
> +        var x = pos.left + element.clientWidth/2 - 1;
> +        var y = pos.top + element.clientHeight/2 - 1;

Does our coding style apply to JS? if so then spaces around /

> LayoutTests/touchadjustment/event-triggered-widgets.html:82
> +        testTouchHit('test1',testDirectTouch);

space after ,

> LayoutTests/touchadjustment/touch-inlines.html:61
> +        shouldBeEqualToString('adjustedNode.id', '');
> +
> +    }

unneeded newlines

> Source/WebCore/page/EventHandler.cpp:2398
> +        // For now we use the adjusted position to ensure the later redundant hit-tests hits the right node.

FIXME?

> Source/WebCore/platform/graphics/IntRect.cpp:135
>  
> +static inline int distanceToInterval(int pos, int start, int end)

Would be great for Darin Adler or Hyatt to look at these IntRect additions

> Source/WebCore/rendering/RenderObject.cpp:2665
> +    // We will not render a new image when Active DOM is suspended

nit, dot at end

> Source/WebCore/testing/Internals.cpp:566
> +PassRefPtr<WebKitPoint> Internals::touchPositionAdjustedToBestClickableNode(long x, long y, long w, long h, Document* document, ExceptionCode& ec)

I think it would be more webkit like to use width instead of w

> Source/WebCore/testing/Internals.h:113
> +    PassRefPtr<WebKitPoint> touchPositionAdjustedToBestClickableNode(long x, long y, long w, long h, Document*, ExceptionCode&);

I find this "best clickable node" weird. What is a best clickable node?

> Source/WebKit2/UIProcess/WebPageProxy.h:388
> +    void handlePotentialActivation(const WebCore::IntPoint&, const WebCore::IntSize&);

Maybe it is not so obvious what the arguments represent?

[Allan Sandfeld Jensen]

Comment on attachment 129887 [details]
Patch

Unrelated change in patch

[Allan Sandfeld Jensen]

(In reply to comment #45)
> > Source/WebCore/rendering/RenderObject.cpp:2665
> > +    // We will not render a new image when Active DOM is suspended
> 
> nit, dot at end
> 
How did that get in there? This is a change from a completely different patch. It shouldn't be here at all.

[Allan Sandfeld Jensen]

(In reply to comment #45)
> (From update of attachment 129887 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129887&action=review
> 
> > LayoutTests/touchadjustment/event-triggered-widgets.html:34
> > +        var x = pos.left + element.clientWidth/2 - 1;
> > +        var y = pos.top + element.clientHeight/2 - 1;
> 
> Does our coding style apply to JS? if so then spaces around /
> 
I don't think so, but I can clean it up.

> 
> > Source/WebCore/page/EventHandler.cpp:2398
> > +        // For now we use the adjusted position to ensure the later redundant hit-tests hits the right node.
> 
> FIXME?
> 
I believe that follows a FIXME, and just explains what we do until that FIXME is resolved.

> > Source/WebCore/testing/Internals.cpp:566
> > +PassRefPtr<WebKitPoint> Internals::touchPositionAdjustedToBestClickableNode(long x, long y, long w, long h, Document* document, ExceptionCode& ec)
> 
> I think it would be more webkit like to use width instead of w
> 
True, but the same file already had other APIs where they used these very short names, so I tried to follow the local coding-style. I can take a look again and at least make the new functions follow webkit coding style closer.

> > Source/WebCore/testing/Internals.h:113
> > +    PassRefPtr<WebKitPoint> touchPositionAdjustedToBestClickableNode(long x, long y, long w, long h, Document*, ExceptionCode&);
> 
> I find this "best clickable node" weird. What is a best clickable node?
> 
It is intentionally vague, because it really represent the node that has is computed to be the 'best' according to an unspecified metric. Currently it is distance to central-line, but it could be anything.

[Allan Sandfeld Jensen]

Created attachment 130620 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 130620 [details]
Patch

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

I would like Darin Adler or Hyatt to have a look at the *Rect additions.

> Source/WebCore/platform/graphics/FloatRect.cpp:167
> +FloatSize FloatRect::differenceFromCenterLineToPoint(const FloatPoint& point) const
> +{

These methods seems rather specialized and I am not sure they are generally useful for FloatRects. Maybe they should be free functions instead?

[Simon Hausmann]

Comment on attachment 130620 [details]
Patch

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

In general I really like this patch. I have a couple of nitpicks and questions :)

> Source/WebCore/page/TouchAdjustment.cpp:42
> +    HitTestQuad(Node* node, const FloatQuad& quad) : m_node(node), m_quad(quad) { }

Style nitpick: Initialized members should be on separate lines.

> Source/WebCore/page/TouchAdjustment.cpp:204
> +bool findNodeWithHighestMetric(Node*& targetNode, LayoutPoint& targetPoint, const LayoutPoint& touchHotspot, const LayoutRect& touchArea, HitList& hitlist, Metric metric)

Even though this is complete, it seems like this is an unused function. IMHO we shouldn't add dead code.

> Source/WebCore/platform/graphics/FloatPoint.h:80
> +    explicit FloatPoint(FloatSize size) : m_x(size.width()), m_y(size.height()) { }

This doesn't seem like a natural constructor to me. I think it's better to have it clearer on the caller side that you intend to convert the width and height of the size to a point, because on the caller side the context/reason may be clearer and easier to understand. Where do you call this constructor, btw?

> Source/WebCore/platform/graphics/FloatPoint.h:143
> +    float distanceSquaredToPoint(const FloatPoint&) const;

Is there any particular reason that these functions have their definitions outside of the class declaration, and for example expandedTo below has it right inside?

[Antonio Gomes]

> > Source/WebCore/platform/graphics/FloatPoint.h:80
> > +    explicit FloatPoint(FloatSize size) : m_x(size.width()), m_y(size.height()) { }
> 
> This doesn't seem like a natural constructor to me. I think it's better to have it clearer on the caller side that you intend to convert the width and height of the size to a point, because on the caller side the context/reason may be clearer and easier to understand. Where do you call this constructor, btw?

there is even a toSize method in FloatPoint, iirc.

[Allan Sandfeld Jensen]

(In reply to comment #51)
> (From update of attachment 130620 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130620&action=review
> 
> > Source/WebCore/page/TouchAdjustment.cpp:204
> > +bool findNodeWithHighestMetric(Node*& targetNode, LayoutPoint& targetPoint, const LayoutPoint& touchHotspot, const LayoutRect& touchArea, HitList& hitlist, Metric metric)
> 
> Even though this is complete, it seems like this is an unused function. IMHO we shouldn't add dead code.
> 
Good point. It is intended for later use, but is currently dead. The rest of its siblings has already been removed as well.

> > Source/WebCore/platform/graphics/FloatPoint.h:80
> > +    explicit FloatPoint(FloatSize size) : m_x(size.width()), m_y(size.height()) { }
> 
> This doesn't seem like a natural constructor to me. I think it's better to have it clearer on the caller side that you intend to convert the width and height of the size to a point, because on the caller side the context/reason may be clearer and easier to understand. Where do you call this constructor, btw?

It was added because IntPoint had a similar constructor from IntSize. The real issue is that mathematical vectors are in WebKit sometimes represented using *Size classes, and sometimes using *Point classes. This constructor essentially takes a FloatVector in FloatSize form and converts it to a FloatVector in FloatPoint form (I have an experimental patch that introduces the vector classes to clarify this, but it is a huge change). 

> 
> > Source/WebCore/platform/graphics/FloatPoint.h:143
> > +    float distanceSquaredToPoint(const FloatPoint&) const;
> 
> Is there any particular reason that these functions have their definitions outside of the class declaration, and for example expandedTo below has it right inside?

Yes, it uses the inline operators that are declared after the class, so it implementation needs to be after the declaration of the inline operators.

[Allan Sandfeld Jensen]

(In reply to comment #52)
> > > Source/WebCore/platform/graphics/FloatPoint.h:80
> > > +    explicit FloatPoint(FloatSize size) : m_x(size.width()), m_y(size.height()) { }
> > 
> > This doesn't seem like a natural constructor to me. I think it's better to have it clearer on the caller side that you intend to convert the width and height of the size to a point, because on the caller side the context/reason may be clearer and easier to understand. Where do you call this constructor, btw?
> 
> there is even a toSize method in FloatPoint, iirc.

But there is no toPoint() metod in FloatSize, and there can not one be since FloatPoint.h includes FloatSize.h, which means FloatSize.h can not include FloatPoint.h. So only FloatPoint.h can implement FloatSize -> FloatPoint functions.

[Allan Sandfeld Jensen]

Created attachment 131032 [details]
Patch

Style changes, switched to consistently use Int types for touch points and removed dead functions only included for completeness.

[WebKit Review Bot]

Attachment 131032 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebKit2/WebProcess/WebPage/WebPage.h:566:  The parameter name "point" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Allan Sandfeld Jensen]

Created attachment 131034 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 131032 [details]
Patch

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

> LayoutTests/touchadjustment/html-label-expected.txt:6
> +
> +
> +
> +Tests if labels are treated as clickable if the input they control is.

why all these newlines in the tests?

> Source/WebCore/page/TouchAdjustment.cpp:40
> +class HitTestQuad {

I find these names confusing, why not just make a QuadNodeMap ? It would make the code easier to understand. This is more like a QuadForHitTest

> Source/WebCore/page/TouchAdjustment.cpp:167
> +float distanceToHitTestQuad(const IntPoint& touchHotspot, const IntRect&, const HitTestQuad& hitTestQuad)

why not be specific? distanceSquaredToQuadBoundingBoxCenterLine() ?

> Source/WebCore/page/TouchAdjustment.cpp:170
> +    // but calculating the distance to quads can be very expensive, and we do not want to calculcate

calculate*

[Allan Sandfeld Jensen]

(In reply to comment #58)
> (From update of attachment 131032 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131032&action=review
> 
> > LayoutTests/touchadjustment/html-label-expected.txt:6
> > +
> > +
> > +
> > +Tests if labels are treated as clickable if the input they control is.
> 
> why all these newlines in the tests?
> 
It is to avoid the hit testing hitting the two control elements below the test that needs to be there, 'description' and 'console'. It is a very crude space, but it keeps the tests simple without also making them all depend on absolute position for instance.

> > Source/WebCore/page/TouchAdjustment.cpp:40
> > +class HitTestQuad {
> 
> I find these names confusing, why not just make a QuadNodeMap ? It would make the code easier to understand. This is more like a QuadForHitTest
> 
A believe a Vector is faster than a Map, when we don't need the Map functionality. I use a special class because I find named parameters more  readable than using a pair template. I admit the name is not optimal so I will rename it QuadForHitTest.

> > Source/WebCore/page/TouchAdjustment.cpp:167
> > +float distanceToHitTestQuad(const IntPoint& touchHotspot, const IntRect&, const HitTestQuad& hitTestQuad)
> 
> why not be specific? distanceSquaredToQuadBoundingBoxCenterLine() ?
> 
Sure. I am will rename the function to something more specific.

> > Source/WebCore/page/TouchAdjustment.cpp:170
> > +    // but calculating the distance to quads can be very expensive, and we do not want to calculcate
> 
> calculate*

I will tell the team here to add a spell-checking to QtCreator.

[Allan Sandfeld Jensen]

Created attachment 131334 [details]
Patch

[Antonio Gomes]

Comment on attachment 131334 [details]
Patch

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

It looks generally good to me. A few comments and we are good to go. I would like see a final version with better name, then the r-. Logic seems fine.

Mainly, the term 'HitTest' spread around in many methods does not read well to me all the time. Same with "Quad".

> Source/WebCore/page/EventHandler.cpp:2449
> +    IntRect touchRect(touchCenter - touchRadius, IntSize(touchRadius.width() * 2, touchRadius.height() * 2));
> +    targetNode = 0;
> +
> +    HitTestResult result = hitTestResultAtPoint(touchCenter, /*allowShadowContent*/ false, /*ignoreClipping*/ false, DontHitTestScrollbars, hitType, touchRadius);

Are sure you do not want to move this line down below to "HitTestResult result = ..." line, and use HitTestResult::rectForPoint(const LayoutPoint& point)?

Also note a "1px" difference in your calculation that I explained in this method body.

> Source/WebCore/page/TouchAdjustment.cpp:41
> +class QuadForHitTest {

rename it as per kenneth suggestion. Maybe something like TouchTargetGeometry or something like this?

> Source/WebCore/page/TouchAdjustment.cpp:117
> +    const unsigned int length = intersectedNodes.length();

'unsigned' would be enough here.

> Source/WebCore/page/TouchAdjustment.cpp:173
> +float distanceSquaredToQuadCenterLine(const IntPoint& touchHotspot, const IntRect&, const QuadForHitTest& quadForHitTest)

remove the unused parameter or name it and add PARAM_UNUSED(xxx) with a comment about any follow up action.

> Source/WebCore/page/TouchAdjustment.cpp:186
> +bool findNodeWithLowestMetric(Node*& targetNode, IntPoint& targetPoint, const IntPoint& touchHotspot, const IntRect& touchArea, QuadNodeList& hitTestList, Metric metric)

Even with the comment I still found 'metrics' is not the best word here.  No better suggestion though.

> Source/WebCore/testing/Internals.cpp:584
> +    document->frame()->eventHandler()->bestClickableNodeForTouchPoint(point, radius, adjustedPoint, targetNode);
> +
> +    return WebKitPoint::create(adjustedPoint.x(), adjustedPoint.y());

I would remove the blank line here

> Source/WebCore/testing/Internals.cpp:602
> +    document->frame()->eventHandler()->bestClickableNodeForTouchPoint(point, radius, adjustedPoint, targetNode);
> +
> +    return adoptRef(targetNode);

ditto

> LayoutTests/touchadjustment/nested-touch.html:37
> +    function findAbsolutePosition(node) {

I am sure there a helper for it. Could you be sure we are not duplicating.

[Allan Sandfeld Jensen]

(In reply to comment #61)
> (From update of attachment 131334 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131334&action=review
> 
> It looks generally good to me. A few comments and we are good to go. I would like see a final version with better name, then the r-. Logic seems fine.
> 
> Mainly, the term 'HitTest' spread around in many methods does not read well to me all the time. Same with "Quad".
> 
Okay.

> > Source/WebCore/page/EventHandler.cpp:2449
> > +    IntRect touchRect(touchCenter - touchRadius, IntSize(touchRadius.width() * 2, touchRadius.height() * 2));
> > +    targetNode = 0;
> > +
> > +    HitTestResult result = hitTestResultAtPoint(touchCenter, /*allowShadowContent*/ false, /*ignoreClipping*/ false, DontHitTestScrollbars, hitType, touchRadius);
> 
> Are sure you do not want to move this line down below to "HitTestResult result = ..." line, and use HitTestResult::rectForPoint(const LayoutPoint& point)?
> 
> Also note a "1px" difference in your calculation that I explained in this method body.
> 
In general I want to avoid rectForPoint, because it is used by Render-code, and is generally wrong (it doesn't account for transformations). So right now it is good to grep for and everywhere that uses it has buggy rect-based hit-testing. I hope at some point to get rid of it, so I don't want to use it.

> > Source/WebCore/page/TouchAdjustment.cpp:41
> > +class QuadForHitTest {
> 
> rename it as per kenneth suggestion. Maybe something like TouchTargetGeometry or something like this?
> 
This was renamed by kenneth suggestion, but sure. It is actually a struct of sub-target areas (absolute-quads) and their corresponding target nodes, but that would probably be a bit too long name.

> > Source/WebCore/page/TouchAdjustment.cpp:117
> > +    const unsigned int length = intersectedNodes.length();
> 
> 'unsigned' would be enough here.
> 
Is that really the webcore coding style? Implicit 'int' is really old deprecated C style.

> > Source/WebCore/page/TouchAdjustment.cpp:173
> > +float distanceSquaredToQuadCenterLine(const IntPoint& touchHotspot, const IntRect&, const QuadForHitTest& quadForHitTest)
> 
> remove the unused parameter or name it and add PARAM_UNUSED(xxx) with a comment about any follow up action.
> 
Of course.

> > Source/WebCore/page/TouchAdjustment.cpp:186
> > +bool findNodeWithLowestMetric(Node*& targetNode, IntPoint& targetPoint, const IntPoint& touchHotspot, const IntRect& touchArea, QuadNodeList& hitTestList, Metric metric)
> 
> Even with the comment I still found 'metrics' is not the best word here.  No better suggestion though.
> 
Metric is math term for ways to calculate distances, but I can see the problem. It is not that well-known a term.

> > Source/WebCore/testing/Internals.cpp:584
> > +    document->frame()->eventHandler()->bestClickableNodeForTouchPoint(point, radius, adjustedPoint, targetNode);
> > +
> > +    return WebKitPoint::create(adjustedPoint.x(), adjustedPoint.y());
> 
> I would remove the blank line here
> 
> > Source/WebCore/testing/Internals.cpp:602
> > +    document->frame()->eventHandler()->bestClickableNodeForTouchPoint(point, radius, adjustedPoint, targetNode);
> > +
> > +    return adoptRef(targetNode);
> 
> ditto
> 
Check anc check.

> > LayoutTests/touchadjustment/nested-touch.html:37
> > +    function findAbsolutePosition(node) {
> 
> I am sure there a helper for it. Could you be sure we are not duplicating.

I don't think there is an official DOM API for it. This is a pretty standard helper function, but do you think there is an internal api for it?

[Allan Sandfeld Jensen]

Created attachment 132572 [details]
Patch

Renames, coding style and code clean-up in central algorithm.

[WebKit Review Bot]

Comment on attachment 132572 [details]
Patch

Clearing flags on attachment: 132572

Committed r111185: <http://trac.webkit.org/changeset/111185>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because it broke the Qt ARM build.
z

[Allan Sandfeld Jensen]

(In reply to comment #66)
> Reopen, because it broke the Qt ARM build.
> z

Build should be fixed r111198 

Apparently the Node::renderStyle() was declared inline outside of Node.h.

[Allan Sandfeld Jensen]

Build-bot for ARM green again.