Changeset 141554 in webkit

Timestamp:

Feb 1, 2013 12:48:05 AM (11 years ago)

Author:

commit-queue@webkit.org

Message:

Touch disambiguation blacklist is not being queried properly
​https://bugs.webkit.org/show_bug.cgi?id=108222

Patch by Dan Alcantara <​dfalcantara@chromium.org> on 2013-02-01
Reviewed by Adam Barth.

TEST=WebFrameTest::DisambiguationPopupBlacklist

Source/WebCore:

Fix the blacklist so that we check it for the right nodes.
Add a chromium test to check that the blacklist is being built and used
correctly. Also update the other DisambiguationPopup tests to
use the new page scale method.

• page/TouchDisambiguation.cpp:

(WebCore::findGoodTouchTargets):

Source/WebKit/chromium:

Fix the blacklist so that we check it for the right nodes.
Add a test to check that the blacklist is being built and used
correctly. Also update the other DisambiguationPopup tests to
use the new page scale method.

• tests/WebFrameTest.cpp:
• tests/data/disambiguation_popup_blacklist.html: Added.

Location:

trunk/Source

Files:

• WebCore/ChangeLog (modified) (1 diff)
• WebCore/page/TouchDisambiguation.cpp (modified) (2 diffs)
• WebKit/chromium/ChangeLog (modified) (1 diff)
• WebKit/chromium/tests/WebFrameTest.cpp (modified) (4 diffs)
• WebKit/chromium/tests/data/disambiguation_popup_blacklist.html (added)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2013-02-01  Dan Alcantara  <dfalcantara@chromium.org>
+
+        Touch disambiguation blacklist is not being queried properly
+        https://bugs.webkit.org/show_bug.cgi?id=108222
+
+        Reviewed by Adam Barth.
+
+        TEST=WebFrameTest::DisambiguationPopupBlacklist
+
+        Fix the blacklist so that we check it for the right nodes.
+        Add a chromium test to check that the blacklist is being built and used
+        correctly.  Also update the other DisambiguationPopup tests to
+        use the new page scale method.
+
+        * page/TouchDisambiguation.cpp:
+        (WebCore::findGoodTouchTargets):
+
 2013-01-31  Kentaro Hara  <haraken@chromium.org>
 

trunk/Source/WebCore/page/TouchDisambiguation.cpp

@@ -107 +107 @@
     HashSet<Node*> blackList;
     for (ListHashSet<RefPtr<Node> >::const_iterator it = hitResults.begin(); it != hitResults.end(); ++it) {
+        // Ignore any Nodes that can't be clicked on.
         RenderObject* renderer = it->get()->renderer();
-        if (!renderer)
+        if (!renderer || !it->get()->willRespondToMouseClickEvents())
             continue;
+
+        // Blacklist all of the Node's containers.
         for (RenderBlock* container = renderer->containingBlock(); container; container = container->containingBlock()) {
             Node* containerNode = container->node();
@@ -123 +126 @@
     for (ListHashSet<RefPtr<Node> >::const_iterator it = hitResults.begin(); it != hitResults.end(); ++it) {
         for (Node* node = it->get(); node; node = node->parentNode()) {
-            if (blackList.contains(it->get()))
+            if (blackList.contains(node))
                 continue;
             if (node->isDocumentNode() || node->hasTagName(HTMLNames::htmlTag) || node->hasTagName(HTMLNames::bodyTag))

trunk/Source/WebKit/chromium/ChangeLog

@@ +1 @@
+2013-02-01  Dan Alcantara  <dfalcantara@chromium.org>
+
+        Touch disambiguation blacklist is not being queried properly
+        https://bugs.webkit.org/show_bug.cgi?id=108222
+
+        Reviewed by Adam Barth.
+
+        TEST=WebFrameTest::DisambiguationPopupBlacklist
+
+        Fix the blacklist so that we check it for the right nodes.
+        Add a test to check that the blacklist is being built and used
+        correctly.  Also update the other DisambiguationPopup tests to
+        use the new page scale method.
+
+        * tests/WebFrameTest.cpp:
+        * tests/data/disambiguation_popup_blacklist.html: Added.
+
 2013-02-01  Eberhard Graether  <egraether@google.com>
 

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

@@ -1825 +1825 @@
     // only becomes available after the load begins.
     WebViewImpl* webViewImpl = static_cast<WebViewImpl*>(FrameTestHelpers::createWebViewAndLoad(m_baseURL + htmlFile, true, 0, &client));
+    webViewImpl->settings()->setApplyDeviceScaleFactorInCompositor(true);
+    webViewImpl->settings()->setApplyPageScaleFactorInCompositor(true);
     webViewImpl->resize(WebSize(1000, 1000));
     webViewImpl->layout();
@@ -1869 +1871 @@
     // only becomes available after the load begins.
     WebViewImpl* webViewImpl = static_cast<WebViewImpl*>(FrameTestHelpers::createWebViewAndLoad(m_baseURL + "disambiguation_popup_no_container.html", true, 0, &client));
+    webViewImpl->settings()->setApplyDeviceScaleFactorInCompositor(true);
+    webViewImpl->settings()->setApplyPageScaleFactorInCompositor(true);
     webViewImpl->resize(WebSize(1000, 1000));
     webViewImpl->layout();
@@ -1887 +1891 @@
     // only becomes available after the load begins.
     WebViewImpl* webViewImpl = static_cast<WebViewImpl*>(FrameTestHelpers::createWebViewAndLoad(m_baseURL + htmlFile, true, 0, &client));
+    webViewImpl->settings()->setApplyDeviceScaleFactorInCompositor(true);
+    webViewImpl->settings()->setApplyPageScaleFactorInCompositor(true);
     webViewImpl->resize(WebSize(1000, 1000));
     webViewImpl->layout();
@@ -1909 +1915 @@
         EXPECT_FALSE(client.triggered());
     }
+}
+
+TEST_F(WebFrameTest, DisambiguationPopupBlacklist)
+{
+    const unsigned viewportWidth = 500;
+    const unsigned viewportHeight = 1000;
+    const unsigned divHeight = 100;
+    const std::string htmlFile = "disambiguation_popup_blacklist.html";
+    registerMockedHttpURLLoad(htmlFile);
+
+    DisambiguationPopupTestWebViewClient client;
+
+    // Make sure we initialize to minimum scale, even if the window size
+    // only becomes available after the load begins.
+    WebViewImpl* webViewImpl = static_cast<WebViewImpl*>(FrameTestHelpers::createWebViewAndLoad(m_baseURL + htmlFile, true, 0, &client));
+    webViewImpl->settings()->setApplyDeviceScaleFactorInCompositor(true);
+    webViewImpl->settings()->setApplyPageScaleFactorInCompositor(true);
+    webViewImpl->resize(WebSize(viewportWidth, viewportHeight));
+    webViewImpl->layout();
+
+    // Click somewhere where the popup shouldn't appear.
+    client.resetTriggered();
+    webViewImpl->handleInputEvent(fatTap(viewportWidth / 2, 0));
+    EXPECT_FALSE(client.triggered());
+
+    // Click directly in between two container divs with click handlers, with children that don't handle clicks.
+    client.resetTriggered();
+    webViewImpl->handleInputEvent(fatTap(viewportWidth / 2, divHeight));
+    EXPECT_TRUE(client.triggered());
+
+    // The third div container should be blacklisted if you click on the link it contains.
+    client.resetTriggered();
+    webViewImpl->handleInputEvent(fatTap(viewportWidth / 2, divHeight * 3.25));
+    EXPECT_FALSE(client.triggered());
 }