Fix gesture scrolling when the target-element of scroll-begin is removed
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 28 Nov 2012 16:06:11 +0000 (16:06 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 28 Nov 2012 16:06:11 +0000 (16:06 +0000)
https://bugs.webkit.org/show_bug.cgi?id=103355

Patch by Sadrul Habib Chowdhury <sadrul@chromium.org> on 2012-11-28
Reviewed by Antonio Gomes.

Source/WebCore:

When a touch-scroll starts, the node under the touch-point gets latched, and
subsequent scroll-update events are dispatched to that node. But if the node is
removed while the gesture event is in progress (e.g. in a dynamically updated
list), then the scrolling stops, although there are enough elements to still
enable scrolling. So instead of latching on to the node immediately under the
touch point, latch on to the scrollable node under the touch point.

Test: fast/events/touch/gesture/touch-gesture-scroll-remove-node.html

* page/EventHandler.cpp:
(WebCore::getClosestScrollableNodeInDocumentIfPossible):
(WebCore):
(WebCore::EventHandler::handleWheelEvent):

LayoutTests:

Added a test that starts a touch-scroll, removes the initial element under the
touch-point, and scrolls some more, to test that the scrolling happens correctly
after the element is removed.

* fast/events/touch/gesture/touch-gesture-scroll-remove-node-expected.txt: Added.
* fast/events/touch/gesture/touch-gesture-scroll-remove-node.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-remove-node-expected.txt [new file with mode: 0644]
LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-remove-node.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/EventHandler.cpp

index 53a001d..d116a09 100644 (file)
@@ -1,3 +1,17 @@
+2012-11-28  Sadrul Habib Chowdhury  <sadrul@chromium.org>
+
+        Fix gesture scrolling when the target-element of scroll-begin is removed
+        https://bugs.webkit.org/show_bug.cgi?id=103355
+
+        Reviewed by Antonio Gomes.
+
+        Added a test that starts a touch-scroll, removes the initial element under the
+        touch-point, and scrolls some more, to test that the scrolling happens correctly
+        after the element is removed.
+
+        * fast/events/touch/gesture/touch-gesture-scroll-remove-node-expected.txt: Added.
+        * fast/events/touch/gesture/touch-gesture-scroll-remove-node.html: Added.
+
 2012-11-28  Jussi Kukkonen  <jussi.kukkonen@intel.com>
 
         [EFL][WK2] add flaky results after r135935
diff --git a/LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-remove-node-expected.txt b/LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-remove-node-expected.txt
new file mode 100644 (file)
index 0000000..34d5149
--- /dev/null
@@ -0,0 +1,29 @@
+This tests gesture event scrolling of a div where the element the scrolling starts on is removed from an onscroll event handler.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+begin scroll
+scroll up
+PASS scroller.scrollTop is 96
+PASS scroller.scrollLeft is 0
+PASS wheelCount is 1
+PASS scrollCount is 1
+PASS typeof cache[2] is typeof undefined
+scroll down
+PASS scroller.scrollTop is 0
+PASS scroller.scrollLeft is 0
+PASS wheelCount is 2
+PASS scrollCount is 2
+1
+2
+3
+4
+5
+6
+7
+8
+9
+10
+11
+
diff --git a/LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-remove-node.html b/LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-remove-node.html
new file mode 100644 (file)
index 0000000..0c633b5
--- /dev/null
@@ -0,0 +1,197 @@
+<!DOCTYPE=html>
+<html>
+<head>
+<script src="../../../js/resources/js-test-pre.js"></script>
+<script src="resources/gesture-helpers.js"></script>
+<style>
+  #spacing {
+    width: 200px;
+    height: 120px;
+  }
+  #scroller {
+    border: 1px solid #777;
+    box-sizing: content-box;
+    height: 300px;
+    margin: 20px;
+    overflow-x: hidden;
+    overflow-y: auto;
+    width: 100px
+  }
+  .spacer {
+    box-sizing: border-box;
+    display: block;
+    overflow: hidden;
+    visibility: hidden;
+  }
+  .list-item {
+    line-height: 30px;
+    text-align: center;
+  }
+</style>
+</head>
+<body onload="runTest();">
+  <div id="spacing"></div>
+  <div id="scroller">
+    <div id = "top-spacer" class="spacer"></div>
+
+    <div id = "bottom-spacer" class="spacer"></div>
+  </div>
+<script>
+var cache = {};
+
+/* One more that the maximum that can be fully fit on screen. */
+var maxVisibleItems = 11;
+
+var listSize = 300;
+
+var itemHeight = 30;
+
+function $(name) {
+  return document.getElementById(name);
+}
+
+function onScroll(e) {
+  redraw();
+}
+
+function redraw() {
+  // Create new items and add to cache.
+  var top = $('scroller').scrollTop;
+  var first = Math.floor(top / 30);
+  if (first > listSize - maxVisibleItems)
+    first = listSize - maxVisibleItems;
+  var newItems = [];
+  for (var i = 0; i < maxVisibleItems; i++) {
+    var index = first + i;
+    if (cache[index] == undefined) {
+      var element = document.createElement('div');
+      element.className = 'list-item';
+      element.textContent = String(index+1);
+      element.data = {};
+      element.data.index = index;
+      cache[index] = element;
+      newItems.push(element);
+    }
+  }
+  // Remove items that are not visible.
+  for (var i = 0; i < first; i++) {
+    if (cache[i])
+      delete cache[i];
+  }
+  for (var i = first + maxVisibleItems; i < listSize; i++) {
+    if (cache[i])
+      delete cache[i];
+  }
+  // Pad spacer elements to preserve height of scrollable list.
+  $('top-spacer').style.height = (first * itemHeight) + 'px';
+  $('bottom-spacer').style.height =
+      ((listSize - maxVisibleItems - first) * 30) + 'px';
+
+  // Remove off-screen elements.
+  var anchor = $('bottom-spacer');
+  var parent = $('scroller');
+  for (var item = $('top-spacer').nextSibling; item != anchor; ) {
+    if (!item.classList || !item.classList.contains('list-item')) {
+      item = item.nextSibling;
+      continue;
+    }
+    if (!cache[item.data.index]) {
+      var next = item.nextSibling;
+      scroller.removeChild(item);
+      item = next;
+    } else {
+      item = item.nextSibling;
+    }
+  }
+
+  // Insert new on-screen elements.
+  var referenceElement = $('top-spacer').nextSibling;
+  for (var i = 0; i < newItems.length; i++) {
+    var index = newItems[i].data.index;
+    while (referenceElement != anchor && (!referenceElement.classList ||
+           !referenceElement.classList.contains('list-item') ||
+           referenceElement.data.index < index)) {
+      referenceElement = referenceElement.nextSibling;
+    }
+    parent.insertBefore(newItems[i], referenceElement);
+  }
+}
+
+$('scroller').addEventListener('scroll', onScroll);
+redraw();
+
+if (window.testRunner)
+    testRunner.waitUntilDone();
+
+var scrollCount = 0;
+var wheelCount = 0;
+var nextStep;
+var scrollIndex = 2;
+var startPositionX;
+var startPositionY;
+var scrollAmount;
+
+function firstStep() {
+    debug("begin scroll");
+    var item = cache[scrollIndex];
+    startPositionX = item.offsetLeft + 5;
+    startPositionY = item.offsetTop + 5;
+    eventSender.gestureScrollBegin(startPositionX, startPositionY);
+
+    var aboveScroller = scroller.offsetTop - 30;
+    scrollAmount = startPositionY - aboveScroller;
+    debug("scroll up");
+
+    nextStep = secondStep;
+
+    eventSender.gestureScrollUpdate(0, -scrollAmount);
+    shouldBe('scroller.scrollTop', '' + scrollAmount);
+    shouldBe('scroller.scrollLeft', '0');
+    shouldBe('wheelCount', '1');
+}
+
+function secondStep() {
+    shouldBe('scrollCount', '1');
+    shouldBe('typeof cache[' + scrollIndex + ']', 'typeof undefined');
+    debug("scroll down");
+
+    nextStep = thirdStep;
+
+    eventSender.gestureScrollUpdate(0, scrollAmount);
+    shouldBe('scroller.scrollTop', '0');
+    shouldBe('scroller.scrollLeft', '0');
+    shouldBe('wheelCount', '2');
+}
+
+function thirdStep() {
+    shouldBe('scrollCount', '2');
+    testRunner.notifyDone();
+}
+
+function runTest()
+{
+    var scroller = $('scroller');
+    scroller.addEventListener("scroll", function() {
+        ++scrollCount;
+        setTimeout(function() {
+            nextStep();
+        }, 0);
+    });
+    window.addEventListener("mousewheel", function() {
+        ++wheelCount;
+    });
+
+    if (window.eventSender && window.testRunner) {
+        description('This tests gesture event scrolling of a div where the element the scrolling starts on is removed from an onscroll event handler.');
+        if (checkTestDependencies()) {
+            firstStep();
+        } else {
+            exitIfNecessary();
+        }
+    } else {
+        debug("This test requires DumpRenderTree.");
+    }
+}
+</script>
+</body>
+</html>
index 6e30d5e..ed42e66 100644 (file)
@@ -1,3 +1,24 @@
+2012-11-28  Sadrul Habib Chowdhury  <sadrul@chromium.org>
+
+        Fix gesture scrolling when the target-element of scroll-begin is removed
+        https://bugs.webkit.org/show_bug.cgi?id=103355
+
+        Reviewed by Antonio Gomes.
+
+        When a touch-scroll starts, the node under the touch-point gets latched, and
+        subsequent scroll-update events are dispatched to that node. But if the node is
+        removed while the gesture event is in progress (e.g. in a dynamically updated
+        list), then the scrolling stops, although there are enough elements to still
+        enable scrolling. So instead of latching on to the node immediately under the
+        touch point, latch on to the scrollable node under the touch point.
+
+        Test: fast/events/touch/gesture/touch-gesture-scroll-remove-node.html
+
+        * page/EventHandler.cpp:
+        (WebCore::getClosestScrollableNodeInDocumentIfPossible):
+        (WebCore):
+        (WebCore::EventHandler::handleWheelEvent):
+
 2012-11-28  Anton Obzhirov  <a.obzhirov@samsung.com>
 
         Fix .libs/libwebkitgtk-3.0.so: undefined reference to `WebCore::validationMessageBadInputForNumberText().
index a213178..db7b143 100644 (file)
@@ -279,6 +279,20 @@ static inline bool scrollNode(float delta, ScrollGranularity granularity, Scroll
     return enclosingBox->scroll(delta < 0 ? negativeDirection : positiveDirection, granularity, absDelta, stopNode);
 }
 
+static Node* getClosestScrollableNodeInDocumentIfPossible(Node* node)
+{
+    Node* firstNode = node;
+    while (node) {
+        if (node->isDocumentNode())
+            return firstNode;
+        RenderObject* renderer = node->renderer();
+        if (renderer && renderer->isBox() && toRenderBox(renderer)->canBeScrolledAndHasScrollableArea())
+            return node;
+        node = node->parentNode();
+    }
+    return firstNode;
+}
+
 #if ENABLE(GESTURE_EVENTS)
 static inline bool shouldGesturesTriggerActive()
 {
@@ -2433,7 +2447,7 @@ bool EventHandler::handleWheelEvent(const PlatformWheelEvent& e)
 
     if (useLatchedWheelEventNode) {
         if (!m_latchedWheelEventNode) {
-            m_latchedWheelEventNode = result.innerNode();
+            m_latchedWheelEventNode = getClosestScrollableNodeInDocumentIfPossible(result.innerNode());
             m_widgetIsLatched = result.isOverWidget();
         }