[webkit-reviews] review canceled: [Bug 91894] Search cancel button is hard to activate with a tap gesture even if touch adjustment is enabled. : [Attachment 154115] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jul 25 08:55:18 PDT 2012


Kevin Ellis <kevers at chromium.org> has canceled Kevin Ellis
<kevers at chromium.org>'s request for review:
Bug 91894: Search cancel button is hard to activate with a tap gesture even if
touch adjustment is enabled.
https://bugs.webkit.org/show_bug.cgi?id=91894

Attachment 154115: Patch
https://bugs.webkit.org/attachment.cgi?id=154115&action=review

------- Additional Comments from Kevin Ellis <kevers at chromium.org>
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.


More information about the webkit-reviews mailing list