Search cancel button is hard to activate with a tap gesture even if touch adjustment is enabled.
Created attachment 153575 [details] Patch
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.
I have uploaded my patch for hasDefaultEventHandler to bug #92093, if accepted the patch would also solve half of this bug.
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
(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.
Created attachment 154115 [details] Patch
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.
Given that 92093 may require further discussion, I'd like to proceed with this fix in parallel.
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 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.
Created attachment 154662 [details] Patch
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 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 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 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.
(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 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.
Created attachment 155000 [details] Patch
Comment on attachment 155000 [details] Patch Clearing flags on attachment: 155000 Committed r123919: <http://trac.webkit.org/changeset/123919>
All reviewed patches have been landed. Closing bug.