Bug 44089 – [Qt] Making effective use of Document::nodesFromRect

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Antonio Gomes]

nodesFromRect is the interface to make use of in order to explore the rect-based hit testing benefits.

Bug is about it.

[Antonio Gomes]

Created attachment 64549 [details]
patch v1

Patch adds a helper class (named TouchAdjuster) to improve tap actions on mobile touch devices. TouchAdjuster makes use of the newly added rect based hit test extension through the Document::nodesFromRect method. Given a rectangle as input, nodesFromRect returns a z-index ordered list of nodes whose boundaries intersect the rectangle.

Basically the TouchAdjuster class intercepts the QGraphicsSceneMouseEvent passed to both QWebPagePrivate::mouse{Press,Release}Event methods before they are passed down to WebCore. The goal is to infer the target click position. For that, a rectangle is built up using the event position as a center point and expanding it both horizontally and vertically, based on the values set in QWebSettings::hitTestPadding.

TouchAdjuster iterates over the list of nodes returned by nodesFromRect and picks the clickable one that has the largest intersection area with the hit test rectangle. The target position is then the center point of this intersection area.

In case of no clickable node intersects the hit test area, the click position is not altered.

TouchAdjuster is *only* working for QGraphicsWebView based views.

[Early Warning System Bot]

Attachment 64549 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3726278

[Antonio Gomes]

(In reply to comment #2)
> Attachment 64549 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/3726278

ignore it. It depends on patch from bug 44088

[Antti Koivisto]

Comment on attachment 64549 [details]
patch v1

Looks good. This might perhaps be slightly improved by treating the touch point as circle instead of rect when calculating the intersection size. Something for the future...

> +Element* QWebPagePrivate::TouchAdjuster::toElement(Node* node) const
> +{
> +    return dynamic_cast<Element*>(node);
> +}

dynamic_cast is not allowed in the WebKit. You should do isElementNode() and static_cast instead. 

I'm surprised this compiles for you. RTTI and exceptions should be disabled in the build as it causes bloat. Is it not?

[Antonio Gomes]

Created attachment 64591 [details]
patch v2

> > +Element* QWebPagePrivate::TouchAdjuster::toElement(Node* node) const
> > +{
> > +    return dynamic_cast<Element*>(node);
> > +}
> 
> dynamic_cast is not allowed in the WebKit. You should do isElementNode() and static_cast instead. 

Done.

> I'm surprised this compiles for you. RTTI and exceptions should be disabled in the build as it causes bloat. Is it not?

ExceptionCode is being widely used on WebKit/*, incluing Qt as you can see here http://pastebin.com/kkED4Dik

[Antonio Gomes]

FYI, also I've also started a QTouchEvent support as well. But it can come in a follow up.

[Early Warning System Bot]

Attachment 64591 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3753334

[Antonio Gomes]

*** Bug 36111 has been marked as a duplicate of this bug. ***

[Ariya Hidayat]

Antonio, do you know why it does not build for Qt EWS?

[Csaba Osztrogonác]

(In reply to comment #9)
> Antonio, do you know why it does not build for Qt EWS?

Because QWebSettings has no member named hitTestPadding. I checked,
it doesn't exist in Qt-4.6 and Qt-4.7 (ToT). Is this a new feature?

error message from EWS:
../../../WebKit/qt/Api/qwebpage.cpp:1394: error: 'class QWebSettings' has no member named 'hitTestPadding'
../../../WebKit/qt/Api/qwebpage.cpp:1395: error: 'class QWebSettings' has no member named 'hitTestPadding'

[Csaba Osztrogonác]

I got it, hitTestPadding will be added in:
https://bugs.webkit.org/show_bug.cgi?id=44088

EWS will fail on this patch until 44088 fixed.

[Antonio Gomes]

Comment on attachment 64591 [details]
patch v2

Clearing the review flag. New version coming, cleaner and supporting inner frames.

[Antonio Gomes]

Created attachment 66416 [details]
patch v3 (it will fail in Qt-EWS until bug 44088 lands)

Improvements from v2 (attachment 64591 [details]):

- inner frames working;
- scrolled content working (offset was not being considered).

[Kenneth Rohde Christiansen]

Comment on attachment 66416 [details]
patch v3 (it will fail in Qt-EWS until bug 44088 lands)

View in context: https://bugs.webkit.org/attachment.cgi?id=66416&action=prettypatch

> WebKit/qt/Api/qwebpage.cpp:1584
> +IntPoint QWebPagePrivate::TouchAdjuster::findCandidatePointForTouch(const IntPoint& touchPoint, Document* document) const
ForTouchPoint?

> WebKit/qt/Api/qwebpage.cpp:1614
> +        if (area(boundingRect) > area(biggerIntersectionRect)) {
I would remove the area methods and just do "int area = width * height;" above. It is a bit overkill and makes the code less obvious

> WebKit/qt/Api/qwebpage.cpp:1616
> +            biggerIntersectionRect = boundingRect;
largestIntersectionRect ? It is bigger than what?

> WebKit/qt/Api/qwebpage.cpp:1629
> +        // Adjust client coordinates' origin to be top left of iframe viewport.
How do you know it is an iframe here?

> WebKit/qt/Api/qwebpage.cpp:1632
> +        IntSize offset =  IntSize(rect->left(), rect->top());
offset_s_? how can an offset be a size?

> WebKit/qt/Api/qwebpage.cpp:1635
> +        HTMLFrameOwnerElement* owner = static_cast<HTMLFrameOwnerElement*>(clickedElement);
This will always work?

> WebKit/qt/Api/qwebpage.cpp:1637
> +        return findCandidatePointForTouch(newTouchPoint, subDocument);
childDocument?

> WebKit/qt/Api/qwebpage.cpp:1644
> +bool QWebPagePrivate::TouchAdjuster::isElementClickable(Element* element, RefPtr<NodeList> list) const
isClickableElement?

> WebKit/qt/Api/qwebpage.cpp:1653
> +        for (int i = 0; i < count && parent; i++) {
unsigned?

[Antonio Gomes]

Created attachment 68010 [details]
patch v4 - addressed kenneth's comments (it will fail until bug 44088 is in)

Comments addressed.

> > WebKit/qt/Api/qwebpage.cpp:1584
> > +IntPoint QWebPagePrivate::TouchAdjuster::findCandidatePointForTouch(const IntPoint& touchPoint, Document* document) const
> ForTouchPoint? 

Hum ... Not sure I left as is as a personal preference, but I can change that if you really prefer.

> > WebKit/qt/Api/qwebpage.cpp:1614
> > +        if (area(boundingRect) > area(biggerIntersectionRect)) {
> I would remove the area methods and just do "int area = width * height;" above. It is a bit overkill and makes the code less obvious

Done.

> > WebKit/qt/Api/qwebpage.cpp:1616
> > +            biggerIntersectionRect = boundingRect;
> largestIntersectionRect ? It is bigger than what?
> 
> > WebKit/qt/Api/qwebpage.cpp:1629
> > +        // Adjust client coordinates' origin to be top left of iframe viewport.
> How do you know it is an iframe here?

I meant inner frame, so either a frame or a iframe. nice catch.

> > WebKit/qt/Api/qwebpage.cpp:1632
> > +        IntSize offset =  IntSize(rect->left(), rect->top());
> offset_s_? how can an offset be a size?
> 
> > WebKit/qt/Api/qwebpage.cpp:1635
> > +        HTMLFrameOwnerElement* owner = static_cast<HTMLFrameOwnerElement*>(clickedElement);
> This will always work?

Yes, because we test isFrameOwner() before.

> 
> > WebKit/qt/Api/qwebpage.cpp:1637
> > +        return findCandidatePointForTouch(newTouchPoint, subDocument);
> childDocument?

Renamed.

> > WebKit/qt/Api/qwebpage.cpp:1644
> > +bool QWebPagePrivate::TouchAdjuster::isElementClickable(Element* element, RefPtr<NodeList> list) const
> isClickableElement?
> 
> > WebKit/qt/Api/qwebpage.cpp:1653
> > +        for (int i = 0; i < count && parent; i++) {
> unsigned?

Done.

[Antonio Gomes]

QtTestBrowser test for the feature happening in bug 46039.

[Kenneth Rohde Christiansen]

Comment on attachment 68010 [details]
patch v4 - addressed kenneth's comments (it will fail until bug 44088 is in)

View in context: https://bugs.webkit.org/attachment.cgi?id=68010&action=prettypatch

> WebKit/qt/Api/qwebpage.cpp:1685
> +    // NOTE: The implementation of this method has to be sync'ed with HitTestResult::regionFromPoint.

Isn't there a way to avoid this? Should it be a FIXME instead?

[Antonio Gomes]

(In reply to comment #17)
> (From update of attachment 68010 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68010&action=prettypatch
> 
> > WebKit/qt/Api/qwebpage.cpp:1685
> > +    // NOTE: The implementation of this method has to be sync'ed with HitTestResult::regionFromPoint.
> 
> Isn't there a way to avoid this? Should it be a FIXME instead?

As the code is currently in HitTestResult, there is no way to make sure these two methods are in sync.

In a follow up we might add a static method to HitTestResult like rectFromRect(IntPoint, unsigned hPadiding, unsigned vPadding). It would return the IntRect we need using the math in HitTestResult to build up the rect. They we would be always in sync.

Worth it?

[Kenneth Rohde Christiansen]

we need some way to make sure they dont get out of sync? test or something else

[Andre Pedralho]

Created attachment 72959 [details]
(committed r71393, r=kenneth,anttik) Updating last patch against ToT, and changed it to use the Platform Plugin QWebTouchModifier instead of QWebSettings.

[Andre Pedralho]

Adding myself to the CC list.

[Kenneth Rohde Christiansen]

Comment on attachment 72959 [details]
(committed r71393, r=kenneth,anttik) Updating last patch against ToT, and changed it to use the Platform Plugin QWebTouchModifier instead of QWebSettings.

Could you implement this for WebKit2/Qt as well?

[Kenneth Rohde Christiansen]

Can't some of this code be moved into webcore somehow, so that we can share it across webkit1 and 2

[Andre Pedralho]

(In reply to comment #23)
> Can't some of this code be moved into webcore somehow, so that we can share it across webkit1 and 2

The first problem here I could see is that we use the QtPlatformPlugin to specify the padding values and AFAIK we don't have access to it in WebCore.

[Kenneth Rohde Christiansen]

isClickableElement and indCandidatePointForTouch seems like good candidates... not indCandidatePointForTouch - that method can easily live in WebKit and be duplicated

[Kenneth Rohde Christiansen]

Comment on attachment 72959 [details]
(committed r71393, r=kenneth,anttik) Updating last patch against ToT, and changed it to use the Platform Plugin QWebTouchModifier instead of QWebSettings.

View in context: https://bugs.webkit.org/attachment.cgi?id=72959&action=review

Can;t the whole TouchAdjuster to move into WebCore? Looking at the code that seems quite possible.

> WebKit/qt/Api/qwebpage.cpp:1281
> +void QWebPagePrivate::adjustPointForClicking(QGraphicsSceneMouseEvent* ev, Document* startingDocument)

You dont need Document here... inside the method you can just do page->mainFrame()->document(). I find that a bit nicer... not using internal WebCore types in our Qt (internal) method definition more than needed.

[Antonio Gomes]

(In reply to comment #26)
> (From update of attachment 72959 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72959&action=review
> 
> Can;t the whole TouchAdjuster to move into WebCore? Looking at the code that seems quite possible.
> 
> > WebKit/qt/Api/qwebpage.cpp:1281
> > +void QWebPagePrivate::adjustPointForClicking(QGraphicsSceneMouseEvent* ev, Document* startingDocument)
> 
> You dont need Document here... inside the method you can just do page->mainFrame()->document(). I find that a bit nicer... not using internal WebCore types in our Qt (internal) method definition more than needed.

Document is needed. We use it for inner frames.

[Kenneth Rohde Christiansen]

(In reply to comment #27)
> (In reply to comment #26)
> > (From update of attachment 72959 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=72959&action=review
> > 
> > Can;t the whole TouchAdjuster to move into WebCore? Looking at the code that seems quite possible.
> > 
> > > WebKit/qt/Api/qwebpage.cpp:1281
> > > +void QWebPagePrivate::adjustPointForClicking(QGraphicsSceneMouseEvent* ev, Document* startingDocument)
> > 
> > You dont need Document here... inside the method you can just do page->mainFrame()->document(). I find that a bit nicer... not using internal WebCore types in our Qt (internal) method definition more than needed.
> 
> Document is needed. We use it for inner frames.

reread my comment :-) you can get the document, you already have the page!

[Kenneth Rohde Christiansen]

> > 
> > Document is needed. We use it for inner frames.
> 
> reread my comment :-) you can get the document, you already have the page!

There is no place it is being called with something else than page->mainFrame()->document().

[Antonio Gomes]

(In reply to comment #29)
> > > 
> > > Document is needed. We use it for inner frames.
> > 
> > reread my comment :-) you can get the document, you already have the page!
> 
> There is no place it is being called with something else than page->mainFrame()->document().

Sure thing. I thought you were referring to the recursive method we are adding. Will fix before land.

[Antonio Gomes]

Comment on attachment 72959 [details]
(committed r71393, r=kenneth,anttik) Updating last patch against ToT, and changed it to use the Platform Plugin QWebTouchModifier instead of QWebSettings.

Clearing review flags on attachment 72959 [details]

Committed r71393: <http://trac.webkit.org/changeset/71393>

[Antonio Gomes]

(In reply to comment #25)
> isClickableElement and indCandidatePointForTouch seems like good candidates... not indCandidatePointForTouch - that method can easily live in WebKit and be duplicated

File bug 49057 about it

[Ademar Reis]

I managed to backport/cherry-pick the changes to qtwebkit-2.1, but in order to do that I had to cherry-pick a few other patches (solving conflicts along the way).

Being so close to the release, I would surelly sleep better at night if someone could review my changes before I push them to qtwebkit-2.1. :-)

The stack of patches looks like this:

Enhance the hit testing to take a rectangle instead of a point
https://bugs.webkit.org/show_bug.cgi?id=40197
(already pushed to 2.1)

Using the Platform Plugin to define the default values for the padding of HitTestResult.
https://bugs.webkit.org/show_bug.cgi?id=48450
(already pushed to 2.1)

[Mac][DRT] Implement LayoutTestController::nodesFromRect
https://bugs.webkit.org/show_bug.cgi?id=46580

Make Document::nodesFromRect more flexible
https://bugs.webkit.org/show_bug.cgi?id=46336

document.nodesFromRect() needs to be removed from Document.idl
https://bugs.webkit.org/show_bug.cgi?id=46492

[Qt] Making effective use of nodesFromRect.
https://bugs.webkit.org/show_bug.cgi?id=44089
(this one)

Could someone please rubberstamp my changes? Although it's building fine, I may be missing an important patch or something.

I pushed the changes to a temporary branch: http://gitorious.org/+qtwebkit-developers/webkit/qtwebkit/commits/qtwebkit-2.1-wip.

[Ademar Reis]

Adding several bug dependencies (see comment #33)

[Ademar Reis]

Revision r71393 cherry-picked into qtwebkit-2.1 with commit 3650536 <http://gitorious.org/webkit/qtwebkit/commit/3650536>