Summary: | [BlackBerry]Adjust fatfinger detection rect size | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tiancheng Jiang <tijiang> | ||||||||||
Component: | UI Events | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | allan.jensen, berto, mifenton, rwlbuis, tonikitoo, webkit.review.bot, yong.li.webkit | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Tiancheng Jiang
2013-02-01 12:47:18 PST
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> All reviewed patches have been landed. Closing bug. > 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? Reopening to attach new patch. 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> All reviewed patches have been landed. Closing bug. |