Bug 44089

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.

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

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

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

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

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

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

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

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

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 on attachment 64591 [details] patch v2 Clearing the review flag. New version coming, cleaner and supporting inner frames.

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

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.

QtTestBrowser test for the feature happening in bug 46039.

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?

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

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

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.

Adding myself to the CC list.

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?

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

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

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

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.

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

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

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

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

(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

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.

Adding several bug dependencies (see comment #33)

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