Fix incorrect assumption about in-flow descendants of inlines in touch event rect...
authorleviw@chromium.org <leviw@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 3 Jan 2013 18:32:01 +0000 (18:32 +0000)
committerleviw@chromium.org <leviw@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 3 Jan 2013 18:32:01 +0000 (18:32 +0000)
https://bugs.webkit.org/show_bug.cgi?id=105970

Reviewed by Simon Fraser.

Source/WebCore:

Correcting the touch event target rect accumulation code to no longer incorrectly assume that
non-block renderers held only normal-flow children. The updated code will always walk the
complete renderer sub-tree, but only do the work of calculating the absolute rect when the
child won't necessarily fall inside its parent (floating, positioned, or transformed).

Tests: platform/chromium/fast/events/touch/compositor-touch-hit-rects.html updated to catch bug.

* page/scrolling/ScrollingCoordinator.cpp:
(WebCore::accumulateRendererTouchEventTargetRects): Walk all renderer sub-trees. Also keeping
track of the last added parent container rect to avoid adding redundant rectangles.
(WebCore::accumulateDocumentEventTargetRects): Avoiding adding empty rects.

LayoutTests:

* platform/chromium-linux/platform/chromium/fast/events/touch/compositor-touch-hit-rects-expected.txt: Updating expectations
* platform/chromium/fast/events/touch/compositor-touch-hit-rects.html: Updating test to check previously failing case where
an inline with a touch handler contains a non-normal-flow child. Also, fixing the test since it was incorrectly duplicated.
* platform/chromium/TestExpectations:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@138717 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/platform/chromium-win/platform/chromium/fast/events/touch/compositor-touch-hit-rects-expected.txt
LayoutTests/platform/chromium/TestExpectations
LayoutTests/platform/chromium/fast/events/touch/compositor-touch-hit-rects.html
Source/WebCore/ChangeLog
Source/WebCore/page/scrolling/ScrollingCoordinator.cpp

index d3d6ab5..97f779d 100644 (file)
@@ -1,3 +1,15 @@
+2013-01-02  Levi Weintraub  <leviw@chromium.org>
+
+        Fix incorrect assumption about in-flow descendants of inlines in touch event rect tracking
+        https://bugs.webkit.org/show_bug.cgi?id=105970
+
+        Reviewed by Simon Fraser.
+
+        * platform/chromium-linux/platform/chromium/fast/events/touch/compositor-touch-hit-rects-expected.txt: Updating expectations
+        * platform/chromium/fast/events/touch/compositor-touch-hit-rects.html: Updating test to check previously failing case where
+        an inline with a touch handler contains a non-normal-flow child. Also, fixing the test since it was incorrectly duplicated.
+        * platform/chromium/TestExpectations:
+
 2013-01-03  Terry Anderson  <tdanderson@chromium.org>
 
         Unreviewed gardening. Marking webaudio/automatic-pull-node.html as
index 17b3655..96b1865 100644 (file)
@@ -1,45 +1,15 @@
 This tests verifies the hit test regions given to the compositor. It can only be run in DumpRenderTree. The outputted rects should cover the hit test regions of all the listed elements.
 
 normalFlow[0]: (8, 72, 784, 10)
-normalFlow[1]: (8, 72, 784, 10)
-normalFlow[2]: (128, 72, 30, 10)
-normalFlow[3]: (8, 72, 784, 10)
-absoluteChildContainer[0]: (8, 162, 784, 10)
-absoluteChildContainer[1]: (8, 162, 784, 10)
-absoluteChildContainer[2]: (8, 162, 784, 10)
-absoluteChildContainer[3]: (118, 300, 140, 10)
-absoluteChildContainer[4]: (118, 300, 140, 10)
-relativeChildContainer[0]: (8, 272, 784, 10)
-relativeChildContainer[1]: (8, 272, 784, 10)
-relativeChildContainer[2]: (8, 272, 784, 10)
-relativeChildContainer[3]: (128, 472, 140, 10)
-overhangingContainer[0]: (8, 362, 784, 10)
-overhangingContainer[1]: (8, 362, 110, 80)
-overhangingContainer[2]: (8, 362, 110, 80)
-transformedChildContainer[0]: (62, 421, 661, 32)
-transformedChildContainer[1]: (62, 421, 661, 32)
-transformedChildContainer[2]: (8, 432, 769, 10)
-continuation[0]: (108, 502, 110, 10)
-normalFlow[0]: (8, 544, 769, 10)
-normalFlow[1]: (8, 544, 769, 10)
-normalFlow[2]: (128, 544, 30, 10)
-normalFlow[3]: (8, 544, 769, 10)
-absoluteChildContainer[0]: (118, 300, 140, 10)
+absoluteChildContainer[0]: (8, 102, 784, 10)
 absoluteChildContainer[1]: (118, 300, 140, 10)
-absoluteChildContainer[2]: (8, 634, 769, 10)
-absoluteChildContainer[3]: (8, 634, 769, 10)
-absoluteChildContainer[4]: (8, 634, 769, 10)
-relativeChildContainer[0]: (8, 744, 769, 10)
-relativeChildContainer[1]: (8, 744, 769, 10)
-relativeChildContainer[2]: (8, 744, 769, 10)
-relativeChildContainer[3]: (128, 944, 140, 10)
-overhangingContainer[0]: (8, 834, 769, 10)
-overhangingContainer[1]: (8, 834, 110, 80)
-overhangingContainer[2]: (8, 834, 110, 80)
-transformedChildContainer[0]: (62, 893, 661, 32)
-transformedChildContainer[1]: (62, 893, 661, 32)
-transformedChildContainer[2]: (8, 904, 769, 10)
-continuation[0]: (108, 974, 110, 10)
-This tests verifies the hit test regions given to the compositor. It can only be run in DumpRenderTree. The outputted rects should cover the hit test regions of all the listed elements.
-
+relativeChildContainer[0]: (8, 152, 784, 10)
+relativeChildContainer[1]: (128, 352, 140, 10)
+overhangingContainer[0]: (8, 202, 784, 10)
+overhangingContainer[1]: (8, 202, 110, 80)
+transformedChildContainer[0]: (63, 240, 674, 34)
+transformedChildContainer[1]: (8, 252, 784, 10)
+continuation[0]: (108, 302, 100, 10)
+inlineAbsoluteChildContainer[0]: (378, 300, 250, 10)
+inlineAbsoluteChildContainer[1]: (108, 352, 270, 10)
 
index daeac80..c1fe5e0 100644 (file)
@@ -4045,6 +4045,7 @@ webkit.org/b/101255 [ Mac Android ] fast/repaint/block-selection-gap-in-table-ce
 
 # Needs baselines
 Bug(thakis) [ Android Release ] fast/forms/zoomed-controls.html [ ImageOnlyFailure ]
+Bug(leviw) [ Android Win Mac Linux ] platform/chromium/fast/events/touch/compositor-touch-hit-rects.html [ Failure ]
 
 # Flaky on win7 since at least r162410
 webkit.org/b/99886 [ Win7 ] fast/dom/shadow/input-with-validation.html [ ImageOnlyFailure Pass ]
index c5087dc..ab1ffb7 100644 (file)
@@ -54,112 +54,15 @@ The outputted rects should cover the hit test regions of all the listed elements
                <div>causes a</div>
                continuation</b>
        </div>
-</div>
-<script>
-if (!window.testRunner)
-       return;
-
-window.testRunner.dumpAsText();
-
-function listener() { }
-
-function log(msg) {
-       var span = document.createElement("span");
-       document.getElementById("console").appendChild(span);
-    span.innerHTML = msg + '<br />';
-}
-
-function sortRects(a, b) {
-       return a.top - b.top;
-}
-
-function logRects(id) {
-       element = document.getElementById(id);
-       element.addEventListener('touchstart', listener, false);
-       rects = window.internals.touchEventTargetClientRects(document);
-       var sortedRects = new Array();
-       for (var i = 0; i < rects.length; ++i)
-               sortedRects[i] = rects[i];
-       sortedRects.sort(sortRects);
-       for (var i = 0; i < rects.length; ++i)
-               log(id + "[" + i + "]: (" + sortedRects[i].left + ", " + sortedRects[i].top + ", " + sortedRects[i].width + ", " + sortedRects[i].height + ")");
-       element.removeEventListener('touchstart', listener, false);
-}
-
-logRects("normalFlow");
-logRects("absoluteChildContainer");
-logRects("relativeChildContainer");
-logRects("overhangingContainer");
-logRects("transformedChildContainer");
-logRects("continuation");
-
-var testContainer = document.getElementById("tests");
-testContainer.parentNode.removeChild(testContainer);
-
-</script>
-</body>
-<!DOCTYPE html>
-<html>
-<head>
-<style>
-#transformedChild {
-       -webkit-transform: rotate3d(0.2, 1, 0, 35grad);
-}
-#absoluteChild {
-       position: absolute;
-       top: 300px;
-}
-#relativeChild {
-       position: relative;
-       top: 200px;
-}
-#overhangingContainer {
-       height: 10px;
-}
-#overhangingFloatingChild {
-       width: 100px;
-       float: left;
-}
-#tests {
-       font: 10px Ahem;
-}
-</style>
-</head>
-<body>
-<p id="description">This tests verifies the hit test regions given to the compositor. It can only be run in DumpRenderTree.
-The outputted rects should cover the hit test regions of all the listed elements.</p>
-<div id="console"></div>
-
-<div id="tests">
-       <div id="normalFlow">
-               Lorem ipsum
-               <span>sum</span>.
-       </div>
-       <div id="absoluteChildContainer">
-               Lorem ipsum
-               <span id="absoluteChild">Absolute child</span>
-       </div>
-       <div id="relativeChildContainer">
-               Lorem ipsum
-               <span id="relativeChild">Relative child</span>
-       </div>
-       <div id="overhangingContainer">
-               <div id="overhangingFloatingChild">Overhanging float overhanging float overhanging float overhanging float</div>
-       </div>
-       <div id="transformedChildContainer">
-               <div id="transformedChild">Transformed</div>
-       </div>
        <div>
-               <b id="continuation">This b tag
-               <div>causes a</div>
-               continuation</b>
+               <span id="inlineAbsoluteChildContainer">
+                       Inline with absolute child.
+                       <span id="absoluteChild">Absolute child in inline.</span>
+               </span>
        </div>
 </div>
 <script>
-if (!window.testRunner)
-       return;
 
-window.testRunner.dumpAsText();
 
 function listener() { }
 
@@ -186,15 +89,23 @@ function logRects(id) {
        element.removeEventListener('touchstart', listener, false);
 }
 
-logRects("normalFlow");
-logRects("absoluteChildContainer");
-logRects("relativeChildContainer");
-logRects("overhangingContainer");
-logRects("transformedChildContainer");
-logRects("continuation");
+function runTest() {
+       if (!window.testRunner)
+               return;
 
-var testContainer = document.getElementById("tests");
-testContainer.parentNode.removeChild(testContainer);
+       window.testRunner.dumpAsText();
+       logRects("normalFlow");
+       logRects("absoluteChildContainer");
+       logRects("relativeChildContainer");
+       logRects("overhangingContainer");
+       logRects("transformedChildContainer");
+       logRects("continuation");
+       logRects("inlineAbsoluteChildContainer");
+
+       var testContainer = document.getElementById("tests");
+       testContainer.parentNode.removeChild(testContainer);
+}
 
+runTest();
 </script>
 </body>
index 5ee06f2..496fe0a 100644 (file)
@@ -1,3 +1,22 @@
+2013-01-02  Levi Weintraub  <leviw@chromium.org>
+
+        Fix incorrect assumption about in-flow descendants of inlines in touch event rect tracking
+        https://bugs.webkit.org/show_bug.cgi?id=105970
+
+        Reviewed by Simon Fraser.
+
+        Correcting the touch event target rect accumulation code to no longer incorrectly assume that
+        non-block renderers held only normal-flow children. The updated code will always walk the
+        complete renderer sub-tree, but only do the work of calculating the absolute rect when the
+        child won't necessarily fall inside its parent (floating, positioned, or transformed).
+
+        Tests: platform/chromium/fast/events/touch/compositor-touch-hit-rects.html updated to catch bug.
+
+        * page/scrolling/ScrollingCoordinator.cpp:
+        (WebCore::accumulateRendererTouchEventTargetRects): Walk all renderer sub-trees. Also keeping
+        track of the last added parent container rect to avoid adding redundant rectangles.
+        (WebCore::accumulateDocumentEventTargetRects): Avoiding adding empty rects.
+
 2013-01-03  Joshua Bell  <jsbell@chromium.org>
 
         IndexedDB: Simplify IDBTransactionBackendImpl::scheduleTask usage
index 08a51b5..0ffd9c1 100644 (file)
@@ -179,15 +179,20 @@ Region ScrollingCoordinator::computeNonFastScrollableRegion(Frame* frame, const
 }
 
 #if ENABLE(TOUCH_EVENT_TRACKING)
-static void accumulateRendererTouchEventTargetRects(Vector<IntRect>& rects, const RenderObject* renderer)
+static void accumulateRendererTouchEventTargetRects(Vector<IntRect>& rects, const RenderObject* renderer, const IntRect& parentRect = IntRect())
 {
-    // FIXME: This method is O(N^2) as it walks the tree to the root for every renderer. RenderGeometryMap would fix this.
-    rects.append(enclosingIntRect(renderer->clippedOverflowRectForRepaint(0)));
-    if (renderer->isRenderBlock()) {
-        const RenderBlock* block = toRenderBlock(renderer);
-        for (RenderObject* child = block->firstChild(); child; child = child->nextSibling())
-            accumulateRendererTouchEventTargetRects(rects, child);
+    IntRect adjustedParentRect = parentRect;
+    if (parentRect.isEmpty() || renderer->isFloating() || renderer->isPositioned() || renderer->hasTransform()) {
+        // FIXME: This method is O(N^2) as it walks the tree to the root for every renderer. RenderGeometryMap would fix this.
+        IntRect r = enclosingIntRect(renderer->clippedOverflowRectForRepaint(0));
+        if (!r.isEmpty() && !parentRect.contains(r)) {
+            rects.append(r);
+            adjustedParentRect = r;
+        }
     }
+
+    for (RenderObject* child = renderer->firstChild(); child; child = child->nextSibling())
+        accumulateRendererTouchEventTargetRects(rects, child, adjustedParentRect);
 }
 
 static void accumulateDocumentEventTargetRects(Vector<IntRect>& rects, const Document* document)
@@ -203,8 +208,11 @@ static void accumulateDocumentEventTargetRects(Vector<IntRect>& rects, const Doc
             continue;
 
         if (touchTarget == document) {
-            if (RenderView* view = document->renderView())
-                rects.append(enclosingIntRect(view->clippedOverflowRectForRepaint(0)));
+            if (RenderView* view = document->renderView()) {
+                IntRect r = enclosingIntRect(view->clippedOverflowRectForRepaint(0));
+                if (!r.isEmpty())
+                    rects.append(r);
+            }
             return;
         }