Changeset 126945 in webkit

Timestamp:

Aug 28, 2012, 6:05:41 PM (14 years ago)

Author:

leandrogracia@chromium.org

Message:

Content detection should not disrupt the page behaviour
​https://bugs.webkit.org/show_bug.cgi?id=94727

Reviewed by Adam Barth.

Source/WebCore:

Tested by WebViewTest::DetectContentAroundPosition.

• dom/Node.cpp:

(WebCore::Node::willRespondToTouchEvents): checks if a node listens to touch events. Very similar to willRespondToMouseClickEvents.
(WebCore):

• dom/Node.h:

(Node):

Source/WebKit/chromium:

Triggers content detection in the embedder on tap gestures and
add checks for the appropriate event listeners in order to prevent
triggering content detection when it would disrupt the page's behaviour.

• src/WebViewImpl.cpp:

(WebKit::WebViewImpl::handleGestureEvent):
(WebKit::WebViewImpl::detectContentOnTouch):

• tests/WebViewTest.cpp:
• tests/data/content_listeners.html: Added.

Location:

trunk/Source

Files:

• WebCore/ChangeLog (modified) (1 diff)
• WebCore/dom/Node.cpp (modified) (1 diff)
• WebCore/dom/Node.h (modified) (1 diff)
• WebKit/chromium/ChangeLog (modified) (1 diff)
• WebKit/chromium/src/WebViewImpl.cpp (modified) (6 diffs)
• WebKit/chromium/tests/WebViewTest.cpp (modified) (2 diffs)
• WebKit/chromium/tests/data/content_listeners.html (added)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-08-28  Leandro Gracia Gil  <leandrogracia@chromium.org>
+
+        Content detection should not disrupt the page behaviour
+        https://bugs.webkit.org/show_bug.cgi?id=94727
+
+        Reviewed by Adam Barth.
+
+        Tested by WebViewTest::DetectContentAroundPosition.
+
+        * dom/Node.cpp:
+        (WebCore::Node::willRespondToTouchEvents): checks if a node listens to touch events. Very similar to willRespondToMouseClickEvents.
+        (WebCore):
+        * dom/Node.h:
+        (Node):
+
 2012-08-28  Simon Fraser  <simon.fraser@apple.com>
 

trunk/Source/WebCore/dom/Node.cpp

@@ -2766 +2766 @@
 }
 
+bool Node::willRespondToTouchEvents()
+{
+#if ENABLE(TOUCH_EVENTS)
+    if (disabled())
+        return false;
+    return hasEventListeners(eventNames().touchstartEvent) || hasEventListeners(eventNames().touchmoveEvent) || hasEventListeners(eventNames().touchcancelEvent) || hasEventListeners(eventNames().touchendEvent);
+#else
+    return false;
+#endif
+}
+
 #if ENABLE(MICRODATA)
 DOMSettableTokenList* Node::itemProp()

trunk/Source/WebCore/dom/Node.h

@@ -585 +585 @@
     virtual bool willRespondToMouseMoveEvents();
     virtual bool willRespondToMouseClickEvents();
+    virtual bool willRespondToTouchEvents();
 
     PassRefPtr<Element> querySelector(const AtomicString& selectors, ExceptionCode&);

trunk/Source/WebKit/chromium/ChangeLog

@@ +1 @@
+2012-08-28  Leandro Gracia Gil  <leandrogracia@chromium.org>
+
+        Content detection should not disrupt the page behaviour
+        https://bugs.webkit.org/show_bug.cgi?id=94727
+
+        Reviewed by Adam Barth.
+
+        Triggers content detection in the embedder on tap gestures and
+        add checks for the appropriate event listeners in order to prevent
+        triggering content detection when it would disrupt the page's behaviour.
+
+        * src/WebViewImpl.cpp:
+        (WebKit::WebViewImpl::handleGestureEvent):
+        (WebKit::WebViewImpl::detectContentOnTouch):
+        * tests/WebViewTest.cpp:
+        * tests/data/content_listeners.html: Added.
+
 2012-08-28  Sheriff Bot  <webkit.review.bot@gmail.com>
 

trunk/Source/WebKit/chromium/src/WebViewImpl.cpp

@@ -688 +688 @@
     switch (event.type) {
     case WebInputEvent::GestureFlingStart: {
+        m_client->cancelScheduledContentIntents();
         m_lastWheelPosition = WebPoint(event.x, event.y);
         m_lastWheelGlobalPosition = WebPoint(event.globalX, event.globalY);
@@ -703 +704 @@
         return false;
     case WebInputEvent::GestureTap: {
+        m_client->cancelScheduledContentIntents();
+        if (detectContentOnTouch(WebPoint(event.x, event.y), event.type))
+            return true;
+
         PlatformGestureEventBuilder platformEvent(mainFrameImpl()->frameView(), event);
         RefPtr<WebCore::PopupContainer> selectPopup;
@@ -723 +728 @@
             return false;
 
+        m_client->cancelScheduledContentIntents();
+        if (detectContentOnTouch(WebPoint(event.x, event.y), event.type))
+            return true;
+
         m_page->contextMenuController()->clearContextMenu();
         m_contextMenuAllowed = true;
@@ -731 +740 @@
     }
     case WebInputEvent::GestureTapDown: {
+        m_client->cancelScheduledContentIntents();
         // Queue a highlight animation, then hand off to regular handler.
 #if OS(LINUX)
@@ -738 +748 @@
         return mainFrameImpl()->frame()->eventHandler()->handleGestureEvent(platformEvent);
     }
+    case WebInputEvent::GestureDoubleTap:
     case WebInputEvent::GestureScrollBegin:
+    case WebInputEvent::GesturePinchBegin:
+        m_client->cancelScheduledContentIntents();
     case WebInputEvent::GestureScrollEnd:
     case WebInputEvent::GestureScrollUpdate:
-    case WebInputEvent::GestureDoubleTap:
-    case WebInputEvent::GesturePinchBegin:
     case WebInputEvent::GesturePinchEnd:
     case WebInputEvent::GesturePinchUpdate: {
@@ -3991 +4002 @@
         return false;
 
-    // FIXME: Should we not detect content intents in nodes that have event listeners?
+    // Ignore when tapping on links or nodes listening to click events, unless the click event is on the
+    // body element, in which case it's unlikely that the original node itself was intended to be clickable.
+    for (; node && !node->hasTagName(HTMLNames::bodyTag); node = node->parentNode()) {
+        if (node->isLink() || (touchType != WebInputEvent::GestureLongPress
+                && (node->willRespondToTouchEvents() || node->willRespondToMouseClickEvents()))) {
+            return false;
+        }
+    }
 
     WebContentDetectionResult content = m_client->detectContentAround(touchHit);

trunk/Source/WebKit/chromium/tests/WebViewTest.cpp

@@ -33 +33 @@
 
 #include "Document.h"
+#include "Element.h"
 #include "FrameTestHelpers.h"
 #include "FrameView.h"
 #include "HTMLDocument.h"
 #include "URLTestHelpers.h"
+#include "WebContentDetectionResult.h"
 #include "WebDocument.h"
+#include "WebElement.h"
 #include "WebFrame.h"
 #include "WebFrameClient.h"
 #include "WebFrameImpl.h"
+#include "WebInputEvent.h"
 #include "platform/WebSize.h"
 #include "WebViewClient.h"
@@ -419 +423 @@
 }
 
-}
+class ContentDetectorClient : public WebViewClient {
+public:
+    ContentDetectorClient() { reset(); }
+
+    virtual WebContentDetectionResult detectContentAround(const WebHitTestResult& hitTest) OVERRIDE
+    {
+        m_contentDetectionRequested = true;
+        return m_contentDetectionResult;
+    }
+
+    virtual void scheduleContentIntent(const WebURL& url) OVERRIDE
+    {
+        m_scheduledIntentURL = url;
+    }
+
+    virtual void cancelScheduledContentIntents() OVERRIDE
+    {
+        m_pendingIntentsCancelled = true;
+    }
+
+    void reset()
+    {
+        m_contentDetectionRequested = false;
+        m_pendingIntentsCancelled = false;
+        m_scheduledIntentURL = WebURL();
+        m_contentDetectionResult = WebContentDetectionResult();
+    }
+
+    bool contentDetectionRequested() const { return m_contentDetectionRequested; }
+    bool pendingIntentsCancelled() const { return m_pendingIntentsCancelled; }
+    const WebURL& scheduledIntentURL() const { return m_scheduledIntentURL; }
+    void setContentDetectionResult(const WebContentDetectionResult& result) { m_contentDetectionResult = result; }
+
+private:
+    bool m_contentDetectionRequested;
+    bool m_pendingIntentsCancelled;
+    WebURL m_scheduledIntentURL;
+    WebContentDetectionResult m_contentDetectionResult;
+};
+
+static bool tapElementById(WebView* webView, WebInputEvent::Type type, const WebString& id)
+{
+    ASSERT(webView);
+    RefPtr<WebCore::Element> element = static_cast<PassRefPtr<WebCore::Element> >(webView->mainFrame()->document().getElementById(id));
+    if (!element)
+        return false;
+
+    element->scrollIntoViewIfNeeded();
+    WebCore::IntPoint center = element->screenRect().center();
+
+    WebGestureEvent event;
+    event.type = type;
+    event.x = center.x();
+    event.y = center.y();
+
+    webView->handleInputEvent(event);
+    webkit_support::RunAllPendingMessages();
+    return true;
+}
+
+TEST_F(WebViewTest, DetectContentAroundPosition)
+{
+    URLTestHelpers::registerMockedURLFromBaseURL(WebString::fromUTF8(m_baseURL.c_str()), WebString::fromUTF8("content_listeners.html"));
+
+    ContentDetectorClient client;
+    WebView* webView = FrameTestHelpers::createWebViewAndLoad(m_baseURL + "content_listeners.html", true, 0, &client);
+    webView->resize(WebSize(500, 300));
+    webView->layout();
+    webkit_support::RunAllPendingMessages();
+
+    WebString clickListener = WebString::fromUTF8("clickListener");
+    WebString touchstartListener = WebString::fromUTF8("touchstartListener");
+    WebString mousedownListener = WebString::fromUTF8("mousedownListener");
+    WebString noListener = WebString::fromUTF8("noListener");
+    WebString link = WebString::fromUTF8("link");
+
+    // Ensure content detection is not requested for nodes listening to click,
+    // mouse or touch events when we do simple taps.
+    EXPECT_TRUE(tapElementById(webView, WebInputEvent::GestureTap, clickListener));
+    EXPECT_FALSE(client.contentDetectionRequested());
+    client.reset();
+
+    EXPECT_TRUE(tapElementById(webView, WebInputEvent::GestureTap, touchstartListener));
+    EXPECT_FALSE(client.contentDetectionRequested());
+    client.reset();
+
+    EXPECT_TRUE(tapElementById(webView, WebInputEvent::GestureTap, mousedownListener));
+    EXPECT_FALSE(client.contentDetectionRequested());
+    client.reset();
+
+    // Content detection should still work on click, mouse and touch event listeners for long taps
+    // as long as we're not tapping on links.
+    EXPECT_TRUE(tapElementById(webView, WebInputEvent::GestureLongPress, clickListener));
+    EXPECT_TRUE(client.contentDetectionRequested());
+    client.reset();
+
+    EXPECT_TRUE(tapElementById(webView, WebInputEvent::GestureLongPress, touchstartListener));
+    EXPECT_TRUE(client.contentDetectionRequested());
+    client.reset();
+
+    EXPECT_TRUE(tapElementById(webView, WebInputEvent::GestureLongPress, mousedownListener));
+    EXPECT_TRUE(client.contentDetectionRequested());
+    client.reset();
+
+    EXPECT_TRUE(tapElementById(webView, WebInputEvent::GestureLongPress, link));
+    EXPECT_FALSE(client.contentDetectionRequested());
+    client.reset();
+
+    // Content detection should work normally without these event listeners.
+    // The click listener in the body should be ignored as a special case.
+    EXPECT_TRUE(tapElementById(webView, WebInputEvent::GestureTap, noListener));
+    EXPECT_TRUE(client.contentDetectionRequested());
+    EXPECT_FALSE(client.scheduledIntentURL().isValid());
+
+    WebURL intentURL = toKURL(m_baseURL);
+    client.setContentDetectionResult(WebContentDetectionResult(WebRange(), WebString(), intentURL));
+    EXPECT_TRUE(tapElementById(webView, WebInputEvent::GestureTap, noListener));
+    EXPECT_TRUE(client.scheduledIntentURL() == intentURL);
+
+    // Tapping elsewhere should cancel the scheduled intent.
+    WebGestureEvent event;
+    event.type = WebInputEvent::GestureTap;
+    webView->handleInputEvent(event);
+    webkit_support::RunAllPendingMessages();
+    EXPECT_TRUE(client.pendingIntentsCancelled());
+    webView->close();
+}
+
+}