Bug 108678 – [BlackBerry]Adjust fatfinger detection rect size

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Tiancheng Jiang]

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)

[Tiancheng Jiang]

Created attachment 186116 [details]
Patch

[Antonio Gomes]

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)

[Tiancheng Jiang]

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)

[Antonio Gomes]

(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.

[Tiancheng Jiang]

Created attachment 186398 [details]
Patch

[Antonio Gomes]

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.

[Tiancheng Jiang]

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.

[Yong Li]

Comment on attachment 186398 [details]
Patch

Thanks Antonio :)

[WebKit Review Bot]

Comment on attachment 186398 [details]
Patch

Clearing flags on attachment: 186398

Committed r141797: <http://trac.webkit.org/changeset/141797>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tiancheng Jiang]

Reopening to attach new patch.

[Tiancheng Jiang]

Created attachment 188423 [details]
Patch

[WebKit Review Bot]

Comment on attachment 188423 [details]
Patch

Clearing flags on attachment: 188423

Committed r143783: <http://trac.webkit.org/changeset/143783>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alberto Garcia]

>  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?

[Tiancheng Jiang]

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?

[Tiancheng Jiang]

Reopening to attach new patch.

[Tiancheng Jiang]

Created attachment 190066 [details]
Patch

[Rob Buis]

Comment on attachment 190066 [details]
Patch

LGTM.

[WebKit Review Bot]

Comment on attachment 190066 [details]
Patch

Clearing flags on attachment: 190066

Committed r143933: <http://trac.webkit.org/changeset/143933>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.