When we do fatfinger, the detection rect size should not be expanded beyond the viewport size to detect any element not visible(out of the viewport)
Created attachment 186116 [details] Patch
Comment on attachment 186116 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186116&action=review TJ: could you test one thing: instead of doing all this, could you not pass HitTestRequest::IgnoreClipping to requestType? and see if it helps > Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:463 > getPaddings(topPadding, rightPadding, bottomPadding, leftPadding); > > + // Do not allow fat fingers detect anything not visible(ie outside of the viewport) > + adjustPaddings(contentViewportPos, topPadding, rightPadding, bottomPadding, leftPadding); not better to have a getAdjustedPaddings? (merge both methods)
Hi Antonio, I tried to remove the HitTestRequest::IgnoreClipping and it seems working. But I am wondering what is clipping the result rect and will this change break other test cases? (In reply to comment #2) > (From update of attachment 186116 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186116&action=review > > TJ: could you test one thing: instead of doing all this, could you not pass HitTestRequest::IgnoreClipping to requestType? and see if it helps > > > Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:463 > > getPaddings(topPadding, rightPadding, bottomPadding, leftPadding); > > > > + // Do not allow fat fingers detect anything not visible(ie outside of the viewport) > > + adjustPaddings(contentViewportPos, topPadding, rightPadding, bottomPadding, leftPadding); > > not better to have a getAdjustedPaddings? (merge both methods)
(In reply to comment #3) > Hi Antonio, I tried to remove the HitTestRequest::IgnoreClipping and it seems working. But I am wondering what is clipping the result rect and will this change break other test cases? Better! Ok, you might need to still clip the result rect, yes.
Created attachment 186398 [details] Patch
Comment on attachment 186398 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186398&action=review Much simpler :) > Source/WebKit/blackberry/Api/WebPage.cpp:3941 > + PlatformWheelEvent wheelEvent(mouseEvent.documentViewportPosition(), mouseEvent.screenPosition(), where does documentViewportPosition come from? :) > Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:284 > + frameContentPos = Platform::pointClampedToRect(frameContentPos, viewportRect); if 'point' is out of the 'rect', what does it return? 0,0, -1,-1. clicking can still be sent to this coords.
We decide to do the transfer the document position in libwebview. they all fix one bug but might make mors sense if i seperate it in another patch. For the pointClampedtoRect function, if the point is outside the rect, we move the point inside the rect and return it. (In reply to comment #6) > (From update of attachment 186398 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186398&action=review > > Much simpler :) > > > Source/WebKit/blackberry/Api/WebPage.cpp:3941 > > + PlatformWheelEvent wheelEvent(mouseEvent.documentViewportPosition(), mouseEvent.screenPosition(), > > where does documentViewportPosition come from? :) > > > Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:284 > > + frameContentPos = Platform::pointClampedToRect(frameContentPos, viewportRect); > > if 'point' is out of the 'rect', what does it return? > > 0,0, -1,-1. clicking can still be sent to this coords.
Comment on attachment 186398 [details] Patch Thanks Antonio :)
Comment on attachment 186398 [details] Patch Clearing flags on attachment: 186398 Committed r141797: <http://trac.webkit.org/changeset/141797>
All reviewed patches have been landed. Closing bug.
Reopening to attach new patch.
Created attachment 188423 [details] Patch
Comment on attachment 188423 [details] Patch Clearing flags on attachment: 188423 Committed r143783: <http://trac.webkit.org/changeset/143783>
> void FatFingers::getNodesFromRect(Document* document, const IntPoint& contentPos, ListHashSet<RefPtr<Node> >& intersectedNodes) > { > unsigned topPadding, rightPadding, bottomPadding, leftPadding; > - getPaddings(topPadding, rightPadding, bottomPadding, leftPadding); > + IntPoint contentViewportPos = m_webPage->mapFromContentsToViewport(m_contentPos); > + // Do not allow fat fingers detect anything not visible(ie outside of the viewport) > + adjustPaddings(contentViewportPos, topPadding, rightPadding, bottomPadding, leftPadding); Is this correct? adjustPaddings() doesn't seem to be defined anywhere, shouldn't it be getAdjustedPaddings() instead?
Yes it should be getAdjustedPaddings(), a mistake when prepare the upstream patch. Thanks Alberto! (In reply to comment #15) > > void FatFingers::getNodesFromRect(Document* document, const IntPoint& contentPos, ListHashSet<RefPtr<Node> >& intersectedNodes) > > { > > unsigned topPadding, rightPadding, bottomPadding, leftPadding; > > - getPaddings(topPadding, rightPadding, bottomPadding, leftPadding); > > + IntPoint contentViewportPos = m_webPage->mapFromContentsToViewport(m_contentPos); > > + // Do not allow fat fingers detect anything not visible(ie outside of the viewport) > > + adjustPaddings(contentViewportPos, topPadding, rightPadding, bottomPadding, leftPadding); > > Is this correct? adjustPaddings() doesn't seem to be defined anywhere, > shouldn't it be getAdjustedPaddings() instead?
Created attachment 190066 [details] Patch
Comment on attachment 190066 [details] Patch LGTM.
Comment on attachment 190066 [details] Patch Clearing flags on attachment: 190066 Committed r143933: <http://trac.webkit.org/changeset/143933>