Bug 108678

Summary: [BlackBerry]Adjust fatfinger detection rect size
Product: WebKit Reporter: Tiancheng Jiang <tijiang>
Component: UI EventsAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Tiancheng Jiang 2013-02-01 12:47:18 PST
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)
Comment 1 Tiancheng Jiang 2013-02-01 13:11:27 PST
Created attachment 186116 [details]
Patch
Comment 2 Antonio Gomes 2013-02-01 14:49:54 PST
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)
Comment 3 Tiancheng Jiang 2013-02-01 16:28:00 PST
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)
Comment 4 Antonio Gomes 2013-02-01 19:00:13 PST
(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.
Comment 5 Tiancheng Jiang 2013-02-04 09:22:04 PST
Created attachment 186398 [details]
Patch
Comment 6 Antonio Gomes 2013-02-04 09:57:59 PST
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.
Comment 7 Tiancheng Jiang 2013-02-04 10:17:45 PST
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 8 Yong Li 2013-02-04 11:23:54 PST
Comment on attachment 186398 [details]
Patch

Thanks Antonio :)
Comment 9 WebKit Review Bot 2013-02-04 13:14:10 PST
Comment on attachment 186398 [details]
Patch

Clearing flags on attachment: 186398

Committed r141797: <http://trac.webkit.org/changeset/141797>
Comment 10 WebKit Review Bot 2013-02-04 13:14:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Tiancheng Jiang 2013-02-14 14:17:52 PST
Reopening to attach new patch.
Comment 12 Tiancheng Jiang 2013-02-14 14:17:54 PST
Created attachment 188423 [details]
Patch
Comment 13 WebKit Review Bot 2013-02-22 13:26:08 PST
Comment on attachment 188423 [details]
Patch

Clearing flags on attachment: 188423

Committed r143783: <http://trac.webkit.org/changeset/143783>
Comment 14 WebKit Review Bot 2013-02-22 13:26:12 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Alberto Garcia 2013-02-25 01:16:00 PST
>  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?
Comment 16 Tiancheng Jiang 2013-02-25 07:49:41 PST
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?
Comment 17 Tiancheng Jiang 2013-02-25 08:16:27 PST
Reopening to attach new patch.
Comment 18 Tiancheng Jiang 2013-02-25 08:16:33 PST
Created attachment 190066 [details]
Patch
Comment 19 Rob Buis 2013-02-25 08:17:13 PST
Comment on attachment 190066 [details]
Patch

LGTM.
Comment 20 WebKit Review Bot 2013-02-25 09:01:11 PST
Comment on attachment 190066 [details]
Patch

Clearing flags on attachment: 190066

Committed r143933: <http://trac.webkit.org/changeset/143933>
Comment 21 WebKit Review Bot 2013-02-25 09:01:16 PST
All reviewed patches have been landed.  Closing bug.