Bug 91894

Summary: Search cancel button is hard to activate with a tap gesture even if touch adjustment is enabled.
Product: WebKit Reporter: Kevin Ellis <kevers>
Component: New BugsAssignee: Kevin Ellis <kevers>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, benjamin, gmak, mifenton, rjkroege, rniwa, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 92093, 92293    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Kevin Ellis 2012-07-20 12:45:33 PDT
Search cancel button is hard to activate with a tap gesture even if touch adjustment is enabled.
Comment 1 Kevin Ellis 2012-07-20 12:59:57 PDT
Created attachment 153575 [details]
Patch
Comment 2 Allan Sandfeld Jensen 2012-07-24 02:12:07 PDT
Good catch, but a few comments.

First off, I think the patch should be split into the part solving the problem with search-cancel, and one improving the distance algorithm.

Second, there is something odd about the additional check to catch the cancel-button. The algorithm will already traverse up the tree and detect the shadow ancestor-node is an input-element (because input elements are mouse-focusable). The problem is more likely that the cancel-button doesn't become a real candidate because it shares the same responding ancester as other shadow-dom elements under the input element. I think we might need something more specific to detect the cancel button (and other shadow-dom buttons), but why isn't the cancel button mouse-focusable?

Note, the 'FIXME' suggestion of implementing a hasDefaultEventHandler would solve the problem as well.
Comment 3 Allan Sandfeld Jensen 2012-07-24 04:08:36 PDT
I have uploaded my patch for hasDefaultEventHandler to bug #92093, if accepted the patch would also solve half of this bug.
Comment 4 Antonio Gomes 2012-07-24 08:41:44 PDT
Comment on attachment 153575 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        elements when controls are tightly packed horizontally.  Using a hybrid of distance to center
> +        and percent overlap results in better disambiguation.

FWIW, in blackberry we use the percent overlap only, but we could adopt a mixed solution as well.

> Source/WebCore/page/TouchAdjustment.cpp:77
> +    Node* ancestorNode = node->shadowAncestorNode();

should not you use ::shadowHost instead?

see bug 91966
Comment 5 Kevin Ellis 2012-07-24 08:59:44 PDT
(In reply to comment #4)
> (From update of attachment 153575 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153575&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        elements when controls are tightly packed horizontally.  Using a hybrid of distance to center
> > +        and percent overlap results in better disambiguation.
> 
> FWIW, in blackberry we use the percent overlap only, but we could adopt a mixed solution as well.
> 
> > Source/WebCore/page/TouchAdjustment.cpp:77
> > +    Node* ancestorNode = node->shadowAncestorNode();
> 
> should not you use ::shadowHost instead?
> 
> see bug 91966

The fix in 92093 for this par of the problem is more generic.  I'm good with pulling out this part of the fix and blocking on the other fix.  There are two issues with this bug.  The first is incorrect pruning of the cancel button (fixed with the patch in 92093).  The second is a bias in the scoring, which is addressed by the hybrid scoring algorithm.
Comment 6 Kevin Ellis 2012-07-24 11:58:43 PDT
Created attachment 154115 [details]
Patch
Comment 7 Kevin Ellis 2012-07-24 12:02:32 PDT
Comment on attachment 153575 [details]
Patch

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

>> Source/WebCore/page/TouchAdjustment.cpp:77
>> +    Node* ancestorNode = node->shadowAncestorNode();
> 
> should not you use ::shadowHost instead?
> 
> see bug 91966

Done.
Comment 8 Kevin Ellis 2012-07-24 12:10:28 PDT
Given that 92093 may require further discussion, I'd like to proceed with this fix in parallel.
Comment 9 Allan Sandfeld Jensen 2012-07-25 07:48:55 PDT
Comment on attachment 154115 [details]
Patch

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

> Source/WebCore/page/TouchAdjustment.cpp:79
> +    Element* shadowHost = node->shadowHost();
> +    if (shadowHost && shadowHost->hasTagName(HTMLNames::inputTag))

I would love if this check was slightly more generic than just inputTag, but I guess this is acceptable as an interim solution. You might want to check if the input tag is disabled or readonly though.

> Source/WebCore/page/TouchAdjustment.cpp:275
> +// Uses a hybrid of distance to center and intersect ratio, normalizing each
> +// score between 0 and 1 and choosing the better score. The distance to
> +// centerline works best for disambiguating clicks on targets such as links,
> +// where the width may be significantly larger than the touch width. Using
> +// area of overlap in such cases can lead to a bias towards shorter links.
> +// Conversely, percentage of overlap can provide strong confidence in tapping
> +// on a small target, where the overlap is often quite high, and works well
> +// for tightly packed controls.
> +float hybridDistanceFunction(const IntPoint& touchHotspot, const IntRect& touchArea, const SubtargetGeometry& subtarget)
> +{
> +    IntRect rect = subtarget.boundingBox();
> +
> +    // Convert from frame coordinates to window coordinates.
> +    rect = subtarget.node()->document()->view()->contentsToWindow(rect);
> +   
> +    float touchWidth = touchArea.width();
> +    float touchHeight = touchArea.height();
> +    float distanceScale =  touchWidth * touchWidth + touchHeight * touchHeight;
> +    float distanceToCenterScore = rect.distanceSquaredFromCenterLineToPoint(touchHotspot) / distanceScale;
> +
> +    float targetArea = rect.size().area();
> +    rect.intersect(touchArea);
> +    float intersectArea = rect.size().area();
> +    float intersectionScore = 1 - intersectArea / targetArea;
> +
> +    return intersectionScore < distanceToCenterScore ? intersectionScore : distanceToCenterScore;
> +}
> +

I would love to see a separate test-case for this change. That was the primary reason I suggested splitting the patch. A test case could also help highlight exactly how and where this is most useful.

> Source/WebCore/page/TouchAdjustment.cpp:388
> +        } else if (distanceMetric - bestDistanceMetric < zeroTolerance) {
> +            if (snapTo(*it, touchHotspot, touchArea, adjustedPoint)) {
> +                if (node->isDescendantOf(targetNode)) {
> +                    // Try to always return the inner-most element.
> +                    targetPoint = adjustedPoint;
> +                    targetNode = node;
> +                    targetArea = it->boundingBox();
> +                } else {
> +                    // Minimize adjustment distance.
> +                    float dx = targetPoint.x() - touchHotspot.x();
> +                    float dy = targetPoint.y() - touchHotspot.y();
> +                    float bestDistance = dx * dx + dy * dy;
> +                    dx = adjustedPoint.x() - touchHotspot.x();
> +                    dy = adjustedPoint.y() - touchHotspot.y();
> +                    float distance = dx * dx + dy * dy;
> +                    if (distance < bestDistance) {
> +                        targetPoint = adjustedPoint;
> +                        targetNode = node;
> +                        targetArea = it->boundingBox();
> +                    }
> +                }

Are you sure this part is still needed? Or is this needed because the shadowHost->hasTagName() check matches too many non-responding siblings of the cancel-button?
Comment 10 Kevin Ellis 2012-07-25 08:55:24 PDT
Comment on attachment 154115 [details]
Patch

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

>> Source/WebCore/page/TouchAdjustment.cpp:79
>> +    if (shadowHost && shadowHost->hasTagName(HTMLNames::inputTag))
> 
> I would love if this check was slightly more generic than just inputTag, but I guess this is acceptable as an interim solution. You might want to check if the input tag is disabled or readonly though.

Can certainly add a check for disabled or readonly inputs.

>> Source/WebCore/page/TouchAdjustment.cpp:275
>> +
> 
> I would love to see a separate test-case for this change. That was the primary reason I suggested splitting the patch. A test case could also help highlight exactly how and where this is most useful.

Initially reluctant to split the patch since the fix to candidate pruning above only accounts for a touch accuracy improvement of roughly 5% to 30% (based on manually testing Chrome with a touch screen).  The rest of the benefit comes from improved scoring. 

Having said that, the scoring addresses other use cases and can look at adding separate tests and a separate "improved scoring" bug.  The distance to center scoring seems to be problematic when there very small targets inline with other controls.  A few tricky test cases would certainly help.

>> Source/WebCore/page/TouchAdjustment.cpp:388
>> +                }
> 
> Are you sure this part is still needed? Or is this needed because the shadowHost->hasTagName() check matches too many non-responding siblings of the cancel-button?

Yes, the tie break is necessary.  If the touch completely covers multiple controls, then there will be several controls with a zero score.  In fact, two of the existing tests fail without the tie-breaker as they use very large touch areas.
Comment 11 Kevin Ellis 2012-07-26 08:56:55 PDT
Created attachment 154662 [details]
Patch
Comment 12 Kevin Ellis 2012-07-26 08:59:35 PDT
Split out refinement to touch adjustment into 92293.  As a result, the test contains a few expected fails until the targeting issue is resolved.
Comment 13 Adam Barth 2012-07-27 01:16:34 PDT
Comment on attachment 153575 [details]
Patch

Cleared review? from obsolete attachment 153575 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).
Comment 14 Antonio Gomes 2012-07-27 06:36:23 PDT
Comment on attachment 154662 [details]
Patch

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

> Source/WebCore/page/TouchAdjustment.cpp:81
> +    Element* shadowHost = node->shadowHost();
> +    if (shadowHost && shadowHost->hasTagName(HTMLNames::inputTag)) {
> +        HTMLInputElement* input = static_cast<HTMLInputElement*>(shadowHost);
> +        if (!input->readOnly() && !input->disabled())
> +            return true;
> +    }

so any shadow content of an input will be a valid target?
Comment 15 Kevin Ellis 2012-07-27 06:56:39 PDT
Comment on attachment 154662 [details]
Patch

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

>> Source/WebCore/page/TouchAdjustment.cpp:81
>> +    }
> 
> so any shadow content of an input will be a valid target?

Yes, based on an inspection of the current set of shadow DOM elements inside the input tags, I believe that they should all be considered as candidates.  Until we have a more generic way of testing (see 92093), it seems safer to err on the side of too many candidates than too few. Having said that, I cannot think of a case where we have a shadow DOM element inside an input tag that should not be a candidate.
Comment 16 Antonio Gomes 2012-07-27 07:15:07 PDT
(In reply to comment #15)
> (From update of attachment 154662 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154662&action=review
> 
> >> Source/WebCore/page/TouchAdjustment.cpp:81
> >> +    }
> > 
> > so any shadow content of an input will be a valid target?
> 
> Yes, based on an inspection of the current set of shadow DOM elements inside the input tags, I believe that they should all be considered as candidates.  Until we have a more generic way of testing (see 92093), it seems safer to err on the side of too many candidates than too few. Having said that, I cannot think of a case where we have a shadow DOM element inside an input tag that should not be a candidate.

But we have a lot of "container" nodes/divs in input shadow trees. Let me think about it for a bit ...

Anyways, it is not that the solution would not work. I will see if we can be less generic here.
Comment 17 Kevin Ellis 2012-07-27 07:48:33 PDT
Comment on attachment 154662 [details]
Patch

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

>>>> Source/WebCore/page/TouchAdjustment.cpp:81
>>>> +    }
>>> 
>>> so any shadow content of an input will be a valid target?
>> 
>> Yes, based on an inspection of the current set of shadow DOM elements inside the input tags, I believe that they should all be considered as candidates.  Until we have a more generic way of testing (see 92093), it seems safer to err on the side of too many candidates than too few. Having said that, I cannot think of a case where we have a shadow DOM element inside an input tag that should not be a candidate.
> 
> But we have a lot of "container" nodes/divs in input shadow trees. Let me think about it for a bit ...
> 
> Anyways, it is not that the solution would not work. I will see if we can be less generic here.

This interim fix will be deprecated by node->willRespondToMouseClickEvents() in 92093.  Looks like it is progressing well and if it can land quickly, I'd be happy to pull out this shadowHost testing and simply check in the test case to ensure that "search cancel" stays fixed.  Will need to update the test expectations once 92293 (touch scoring) lands.
Comment 18 Kevin Ellis 2012-07-27 11:47:29 PDT
Created attachment 155000 [details]
Patch
Comment 19 WebKit Review Bot 2012-07-27 14:49:54 PDT
Comment on attachment 155000 [details]
Patch

Clearing flags on attachment: 155000

Committed r123919: <http://trac.webkit.org/changeset/123919>
Comment 20 WebKit Review Bot 2012-07-27 14:50:00 PDT
All reviewed patches have been landed.  Closing bug.