TOUCH_ADJUSTMENT is too aggressive when snapping to large elements.

• Component: DOM
• Status: RESOLVED FIXED

[Kevin Ellis]

Currently when TOUCH_ADJUSTMENT is enabled, synthetic mouse events snap to the center of the targeted element.  This works well for snapping to small targets, but creates problems when the snap target is significantly larger than the touch area.

For example, in a wide input field the user has no control over where in the input field the text caret is positioned.

Toolbars can also be problematic if there is a sufficient border on the enclosing DIV to enable adjusting to the DIV rather than the intended button on a near miss.  In this case, snapping to the center of the DIV could result in synthetic click centered on the wrong button, which is potentially far removed from the touch area.

[Kevin Ellis]

Created attachment 152296 [details]
Patch

[Allan Sandfeld Jensen]

If the element is transformed and the quad therefore not rectilinear, are you sure this method will give you a point inside the quad?

Snapping to the center was specifically used because I didn't come up with a good method for finding a spot inside the intersection of the touch-point area and the quad.

[Allan Sandfeld Jensen]

Bonus question: Why are toolbars a problem? Are they replaced elements instead of shadow dom? 

The algorithm is supposed to work on the shadow dom result so it will adjust the point actually responding to the click.

[Kevin Ellis]

(In reply to comment #3)
> Bonus question: Why are toolbars a problem? Are they replaced elements instead of shadow dom? 
> 
> The algorithm is supposed to work on the shadow dom result so it will adjust the point actually responding to the click.

The toolbar issue is not shadow DOM related.  The issue arises if you miss the button sufficiently that there is no overlap with the intended button but still overlap with the border of the enclosing DIV.  In this case, the DIV element becomes the target element and the adjusted position was snapping to the center of the toolbar.  This can result in synthetic mouse events being delivered using a mouse position that is not near the touch area.

Other examples were snap to center is problematic is on the <canvas> tag and <select size=...> .  The <select> case comes up with the month-year selector on the date picker.  Tapping anywhere on the drop-down list, delivers mouse events that  result in selecting the middle element regardless of where on the list you tap.

That said, the existing adjustment algorithm works great for small targets.

[Kevin Ellis]

(In reply to comment #2)
> If the element is transformed and the quad therefore not rectilinear, are you sure this method will give you a point inside the quad?
> 
> Snapping to the center was specifically used because I didn't come up with a good method for finding a spot inside the intersection of the touch-point area and the quad.

The point is not guaranteed to be inside in the event that the transformed quad is not rectangular; however, I believe that the "cost" of a miss may be less than the cost of a synthetic click that is not close to the touch area.  The month-year drop-down on the calendar picker is a good example.

An alternate algorithm was to move further inside the bounds limited by the radius of the touch area rather than just inside the rectangular enclosure.  The reason this approach was not used is that it becomes very difficult to position the text caret at the start of an input field, more difficult to select the first or last element in a list, ...

The snapping could be adapted to snap to a quad if non-rectangular bounds are sufficiently common.

[Antonio Gomes]

FWIW, in blackberry, we snap to the center of the intersection area.

[Antonio Gomes]

If I understood the problem correct, arbitrarily snapping to the middle of the element can be very bad indeed.

Image a big button and one clicking on its far right portion. Then, if in the middle of it there is a fixed position element, snapping could hit the wrong target.

(In reply to comment #0)
> Currently when TOUCH_ADJUSTMENT is enabled, synthetic mouse events snap to the center of the targeted element.  This works well for snapping to small targets, but creates problems when the snap target is significantly larger than the touch area.
> 
> For example, in a wide input field the user has no control over where in the input field the text caret is positioned.
> 
> Toolbars can also be problematic if there is a sufficient border on the enclosing DIV to enable adjusting to the DIV rather than the intended button on a near miss.  In this case, snapping to the center of the DIV could result in synthetic click centered on the wrong button, which is potentially far removed from the touch area.

[Kevin Ellis]

(In reply to comment #6)
> FWIW, in blackberry, we snap to the center of the intersection area.

This is a very interesting alternative.

[Allan Sandfeld Jensen]

(In reply to comment #8)
> (In reply to comment #6)
> > FWIW, in blackberry, we snap to the center of the intersection area.
> 
> This is a very interesting alternative.

This is what I would prefer. The problem is that it is an intersection of a quad and and rect, not between two rects. It makes it geometrically complicated (though for the 99.9% case of untransformed elements it is trivial).

I have though of an alternative twist to your suggested patch. You could take the center of the quad, and snap that to the touch-area if it is outside. This would adjust to an edge of the touch-area in the direction of the center of the quad.

[Antonio Gomes]

Comment on attachment 152296 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        adjusted position unchanged if tapping within the element or to snap
> +        to a point just inside the nearest edge if the touch point lies 
> +        outside the bounds of the element.  This strategy works well for
> +        small as well as large targets.

As I said before, it has a weak point: If there is a fixed position element on top of the target's nearest edge?

Do we care?

[Kevin Ellis]

(In reply to comment #10)
> (From update of attachment 152296 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152296&action=review
> 
> > Source/WebCore/ChangeLog:16
> > +        adjusted position unchanged if tapping within the element or to snap
> > +        to a point just inside the nearest edge if the touch point lies 
> > +        outside the bounds of the element.  This strategy works well for
> > +        small as well as large targets.
> 
> As I said before, it has a weak point: If there is a fixed position element on top of the target's nearest edge?
> 
> Do we care?

Not sure that this is a problem.  Adjusting to the edge only if the initial point is outside the bounds.  A competing element at the boundary could end up handling the synthetic mouse event, but at least it would need to overlap the touch area.  Currently, it is possible for an element far removed from the touch area to receive the synthetic mouse events after adjustment.

Having said that, there is a problem in the patch whereby the element bounds and touch hotspot are in different coordinate systems.

[Allan Sandfeld Jensen]

(In reply to comment #11)
> Having said that, there is a problem in the patch whereby the element bounds and touch hotspot are in different coordinate systems.

They shouldn't be, there should be a transformation on the target area to screen coords. This transformation is for instance essential to zoom targeting.

Continuing from some of comments earlier:
I am rethinking the problem with transformations. If we hold on to your idea of only adjusting the target if it not already in the picked target area (using FloatQuad::contains for instance), then doing a potentially bad adjustment for rare corner cases is a smaller issue, since we in the worst case scenario only loose the touch adjustment when the point is outside of the area, but could not end up making an area completely un-hittable.

Currently non-rectangular transformations are not handled correctly during hit-detection or distance calculations either, so it still low priority to handle them fault-free here at a latter stage.

To conclude, my suggestion will be to use a combination of your idea of only adjusting the target if is not already in the targeted area, and if it is outside then use a similar idea as in the blackberry port and place the target in the center of the intersection of the bounding-box of the target area and the touch area.

[Kevin Ellis]

Created attachment 152786 [details]
Patch

[Kevin Ellis]

Revised strategy is as follows:

For rectilinear elements
  - Don't adjust if inside element bounds, but set target node.
  - Adjust to center of overlap between touch area and element bounds.
  - Fail if touch area and element bounds don't overlap.

For non-rectilinear elements
  - Don't adjust if inside quad boundary of element, but set target node.
  - Pull point towards center of element by a maximum of the touch radius.
  - Discard adjustment if candidate point is not inside quad.

Added a test for the non-rectilinear case.

[Allan Sandfeld Jensen]

(In reply to comment #14)
> Revised strategy is as follows:
> 
> For rectilinear elements
>   - Don't adjust if inside element bounds, but set target node.
>   - Adjust to center of overlap between touch area and element bounds.
>   - Fail if touch area and element bounds don't overlap.
> 
> For non-rectilinear elements
>   - Don't adjust if inside quad boundary of element, but set target node.
>   - Pull point towards center of element by a maximum of the touch radius.
>   - Discard adjustment if candidate point is not inside quad.
> 
> Added a test for the non-rectilinear case.

Good stuff! I was about to say that the strategy doesn't guarantee to adjust to a point in the quad, but you noticed that already.

I think it is almost overkill, but now that you have already done the work, it looks good to me.

[Antonio Gomes]

Comment on attachment 152786 [details]
Patch

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

> Source/WebCore/page/TouchAdjustment.cpp:286
> +    float cx = 0.25*(p1.x() + p2.x() + p3.x() + p4.x());
> +    float cy = 0.25*(p1.y() + p2.y() + p3.y() + p4.y());

space * scpace

> Source/WebCore/page/TouchAdjustment.cpp:290
> +    FloatSize v = center - touchPoint;
> +    float d = v.diagonalLength();

v and d are not the webkit style for var names.

> Source/WebCore/page/TouchAdjustment.cpp:295
> +    float r = sqrt(dx*dx + dy*dy);

ditto + spaces

[W. James MacLean]

Created attachment 153004 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 153004 [details]
Patch for landing

Clearing flags on attachment: 153004

Committed r122970: <http://trac.webkit.org/changeset/122970>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Lucas Forschler]

Mass move bugs into the DOM component.