[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