Bug 44089

Summary: [Qt] Making effective use of Document::nodesFromRect
Product: WebKit Reporter: Antonio Gomes <tonikitoo>
Component: WebKit QtAssignee: Antonio Gomes <tonikitoo>
Status: RESOLVED FIXED    
Severity: Enhancement CC: ademar, andre.pedralho, ariya.hidayat, benjamin, christian.webkit, dbates, gmak, hausmann, kenneth, klobag, luiz, ossy, webkit-ews
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on: 40197, 44088, 46336, 46492, 46580, 48450    
Bug Blocks: 46039    
Attachments:
Description Flags
patch v1
none
patch v2
none
patch v3 (it will fail in Qt-EWS until bug 44088 lands)
kenneth: review-
patch v4 - addressed kenneth's comments (it will fail until bug 44088 is in)
kenneth: review+
(committed r71393, r=kenneth,anttik) Updating last patch against ToT, and changed it to use the Platform Plugin QWebTouchModifier instead of QWebSettings. none

Description Antonio Gomes 2010-08-16 20:36:03 PDT
nodesFromRect is the interface to make use of in order to explore the rect-based hit testing benefits.

Bug is about it.
Comment 1 Antonio Gomes 2010-08-16 20:38:29 PDT
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.
Comment 2 Early Warning System Bot 2010-08-16 20:52:39 PDT
Attachment 64549 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3726278
Comment 3 Antonio Gomes 2010-08-16 20:58:40 PDT
(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
Comment 4 Antti Koivisto 2010-08-17 01:43:15 PDT
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?
Comment 5 Antonio Gomes 2010-08-17 07:57:30 PDT
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
Comment 6 Antonio Gomes 2010-08-17 08:00:53 PDT
FYI, also I've also started a QTouchEvent support as well. But it can come in a follow up.
Comment 7 Early Warning System Bot 2010-08-17 08:14:00 PDT
Attachment 64591 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3753334
Comment 8 Antonio Gomes 2010-08-18 08:28:50 PDT
*** Bug 36111 has been marked as a duplicate of this bug. ***
Comment 9 Ariya Hidayat 2010-08-22 21:47:19 PDT
Antonio, do you know why it does not build for Qt EWS?
Comment 10 Csaba Osztrogon√°c 2010-08-23 00:48:03 PDT
(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'
Comment 11 Csaba Osztrogon√°c 2010-08-23 00:49:48 PDT
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.
Comment 12 Antonio Gomes 2010-08-24 07:39:48 PDT
Comment on attachment 64591 [details]
patch v2

Clearing the review flag. New version coming, cleaner and supporting inner frames.
Comment 13 Antonio Gomes 2010-09-02 15:03:17 PDT
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).
Comment 14 Kenneth Rohde Christiansen 2010-09-13 00:22:53 PDT
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?
Comment 15 Antonio Gomes 2010-09-18 08:57:45 PDT
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.
Comment 16 Antonio Gomes 2010-09-18 09:35:52 PDT
QtTestBrowser test for the feature happening in bug 46039.
Comment 17 Kenneth Rohde Christiansen 2010-09-18 15:48:21 PDT
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?
Comment 18 Antonio Gomes 2010-09-18 15:55:37 PDT
(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?
Comment 19 Kenneth Rohde Christiansen 2010-09-18 15:58:53 PDT
we need some way to make sure they dont get out of sync? test or something else
Comment 20 Andre Pedralho 2010-11-04 10:38:46 PDT
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.
Comment 21 Andre Pedralho 2010-11-04 11:41:55 PDT
Adding myself to the CC list.
Comment 22 Kenneth Rohde Christiansen 2010-11-04 12:25:14 PDT
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?
Comment 23 Kenneth Rohde Christiansen 2010-11-04 12:30:05 PDT
Can't some of this code be moved into webcore somehow, so that we can share it across webkit1 and 2
Comment 24 Andre Pedralho 2010-11-04 12:46:31 PDT
(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.
Comment 25 Kenneth Rohde Christiansen 2010-11-04 12:48:48 PDT
isClickableElement and indCandidatePointForTouch seems like good candidates... not indCandidatePointForTouch - that method can easily live in WebKit and be duplicated
Comment 26 Kenneth Rohde Christiansen 2010-11-04 13:26:51 PDT
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.
Comment 27 Antonio Gomes 2010-11-04 15:46:02 PDT
(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.
Comment 28 Kenneth Rohde Christiansen 2010-11-04 15:48:52 PDT
(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!
Comment 29 Kenneth Rohde Christiansen 2010-11-04 16:32:17 PDT
> > 
> > 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().
Comment 30 Antonio Gomes 2010-11-04 16:54:35 PDT
(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.
Comment 31 Antonio Gomes 2010-11-04 22:09:58 PDT
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>
Comment 32 Antonio Gomes 2010-11-04 22:28:39 PDT
(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
Comment 33 Ademar Reis 2010-11-11 13:18:04 PST
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.
Comment 34 Ademar Reis 2010-11-12 10:05:42 PST
Adding several bug dependencies (see comment #33)
Comment 35 Ademar Reis 2010-11-12 12:25:09 PST
Revision r71393 cherry-picked into qtwebkit-2.1 with commit 3650536 <http://gitorious.org/webkit/qtwebkit/commit/3650536>