Attachment 34574 Details for Bug 28023 – Scroll Fixes + Automated Test

[patch] Scroll Fixes + Automated Test

0001-2009-08-11-Brian-Weinstein-bweinstein-apple.com.patch (text/plain), 12.72 KB, created by Brian Weinstein on 2009-08-11 10:13:53 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Brian Weinstein

Created: 2009-08-11 10:13:53 PDT

Size: 12.72 KB

patch

obsolete

>From 09b434e8131e5c005e9380ea8f0f0307274fa3e5 Mon Sep 17 00:00:00 2001
>From: Brian Weinstein <bweinstein@apple.com>
>Date: Tue, 11 Aug 2009 10:07:58 -0700
>Subject: [PATCH] 2009-08-11  Brian Weinstein  <bweinstein@apple.com>
>
>        Reviewed by NOBODY (OOPS!).
>
>        Fixes two bugs about Pan Scrolling - Scrolling with middle mouse button doesn't
>        work in Expanded view on reader.google.com, and Can enter auto scroll from a non-scrollable area.
>        https://bugs.webkit.org/show_bug.cgi?id=28023
>        https://bugs.webkit.org/show_bug.cgi?id=24794
>
>        Added a new method to RenderBox seeing if the Box can be scrolled and actually has something to
>        scroll, and use that for pan scrolling.
>
>        Test: platform/win/fast/events/panScroll-nested-divs.html
>
>        * page/EventHandler.cpp:
>        (WebCore::EventHandler::handleMouseDraggedEvent):
>        (WebCore::EventHandler::updateAutoscrollRenderer):
>        (WebCore::EventHandler::handleMousePressEvent):
>        * rendering/RenderBox.cpp:
>        (WebCore::RenderBox::canBeScrolledAndHasScrollableArea):
>        * rendering/RenderBox.h:
>        * rendering/RenderLayer.cpp:
>        (WebCore::RenderLayer::scrollByRecursively):
>
>2009-08-11  Brian Weinstein  <bweinstein@apple.com>
>
>        Reviewed by NOBODY (OOPS!).
>
>        Added a Windows test for https://bugs.webkit.org/show_bug.cgi?id=28023.
>
>        * platform/win/fast/events/panScroll-nested-divs-expected.txt: Added.
>        * platform/win/fast/events/panScroll-nested-divs.html: Added.
>---
> LayoutTests/ChangeLog                              |    9 ++++
> .../fast/events/panScroll-nested-divs-expected.txt |    6 ++
> .../win/fast/events/panScroll-nested-divs.html     |   49 ++++++++++++++++++++
> WebCore/ChangeLog                                  |   24 ++++++++++
> WebCore/page/EventHandler.cpp                      |    6 +-
> WebCore/rendering/RenderBox.cpp                    |    5 ++
> WebCore/rendering/RenderBox.h                      |    1 +
> WebCore/rendering/RenderLayer.cpp                  |   21 +++++++-
> 8 files changed, 115 insertions(+), 6 deletions(-)
> create mode 100644 LayoutTests/platform/win/fast/events/panScroll-nested-divs-expected.txt
> create mode 100755 LayoutTests/platform/win/fast/events/panScroll-nested-divs.html
>
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index e9f8263..019f0fc 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,12 @@
>+2009-08-11  Brian Weinstein  <bweinstein@apple.com>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Added a Windows test for https://bugs.webkit.org/show_bug.cgi?id=28023.
>+
>+        * platform/win/fast/events/panScroll-nested-divs-expected.txt: Added.
>+        * platform/win/fast/events/panScroll-nested-divs.html: Added.
>+
> 2009-08-07  Mike Fenton  <mike.fenton@torchmobile.com>
> 
>         Reviewed by Adam Treat.
>diff --git a/LayoutTests/platform/win/fast/events/panScroll-nested-divs-expected.txt b/LayoutTests/platform/win/fast/events/panScroll-nested-divs-expected.txt
>new file mode 100644
>index 0000000..17f5f26
>--- /dev/null
>+++ b/LayoutTests/platform/win/fast/events/panScroll-nested-divs-expected.txt
>@@ -0,0 +1,6 @@
>+Panscrolling starting in the blue box should scroll the outer div.
>+Panscrolling outside the blue boxes should scroll the outer div.
>+Panscrolling starting in the blue box should scroll the outer div.
>+Test for bug 28023 This tests that pan scrolling propogates correctly up the DOM tree. On success, our scroll offset should be non-zero.
>+
>+Success! Div with overflow was scrolled
>diff --git a/LayoutTests/platform/win/fast/events/panScroll-nested-divs.html b/LayoutTests/platform/win/fast/events/panScroll-nested-divs.html
>new file mode 100755
>index 0000000..fc8e4ed
>--- /dev/null
>+++ b/LayoutTests/platform/win/fast/events/panScroll-nested-divs.html
>@@ -0,0 +1,49 @@
>+<html>
>+    <head>
>+        <title>Pan Scrolling Test</title>
>+    </head>
>+    <body>
>+        <script>
>+            if (layoutTestController) {
>+                layoutTestController.dumpAsText();
>+                layoutTestController.waitUntilDone();
>+            }
>+        </script>
>+        <div id="overflow" style="width:500px; height:300px; overflow:auto; border:2px solid red; padding:10px">
>+            <div style="height:200px; position:relative;">
>+                <div style="height:150px; border:1px blue solid; overflow:auto;">
>+                    Panscrolling starting in the blue box should scroll the outer div.
>+                </div>
>+                Panscrolling outside the blue boxes should scroll the outer div.
>+            </div>
>+            <div style="height:200px; position:relative;">
>+                <div style="height:150px; border:1px blue solid; overflow:auto;">
>+                    Panscrolling starting in the blue box should scroll the outer div.
>+                </div>
>+            </div>
>+        </div>
>+        <p>Test for <a href="https://bugs.webkit.org/show_bug.cgi?id=28023">bug 28023</a> This tests that pan scrolling
>+        propogates correctly up the DOM tree. On success, our scroll offset should be non-zero.</p>
>+        <div id="console"></div>
>+        <script>
>+            if (eventSender)
>+            {
>+                eventSender.mouseMoveTo(50, 50);
>+                eventSender.mouseDown(1);
>+                eventSender.mouseUp(1);
>+                eventSender.mouseMoveTo(50, 200);
>+                setTimeout(finished, 500);
>+            }
>+            
>+            function finished()
>+            {
>+                if (document.getElementById('overflow').scrollTop)
>+                    document.getElementById('console').innerHTML = "Success! Div with overflow was scrolled";
>+                else
>+                    document.getElementById('console').innerHTML = "Fail! Div with overflow was not scrolled";
>+                
>+                window.layoutTestController.notifyDone();
>+            }
>+        </script>
>+    </body>
>+</html>
>diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
>index e7bee6f..cf81037 100644
>--- a/WebCore/ChangeLog
>+++ b/WebCore/ChangeLog
>@@ -1,3 +1,27 @@
>+2009-08-11  Brian Weinstein  <bweinstein@apple.com>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Fixes two bugs about Pan Scrolling - Scrolling with middle mouse button doesn't 
>+        work in Expanded view on reader.google.com, and Can enter auto scroll from a non-scrollable area.
>+        https://bugs.webkit.org/show_bug.cgi?id=28023
>+        https://bugs.webkit.org/show_bug.cgi?id=24794
>+        
>+        Added a new method to RenderBox seeing if the Box can be scrolled and actually has something to
>+        scroll, and use that for pan scrolling.
>+
>+        Test: platform/win/fast/events/panScroll-nested-divs.html
>+
>+        * page/EventHandler.cpp:
>+        (WebCore::EventHandler::handleMouseDraggedEvent):
>+        (WebCore::EventHandler::updateAutoscrollRenderer):
>+        (WebCore::EventHandler::handleMousePressEvent):
>+        * rendering/RenderBox.cpp:
>+        (WebCore::RenderBox::canBeScrolledAndHasScrollableArea):
>+        * rendering/RenderBox.h:
>+        * rendering/RenderLayer.cpp:
>+        (WebCore::RenderLayer::scrollByRecursively):
>+
> 2009-08-07  Steve Block  <steveblock@google.com>
> 
>         Reviewed by Darin Adler.
>diff --git a/WebCore/page/EventHandler.cpp b/WebCore/page/EventHandler.cpp
>index fd64a8b..58e9edd 100644
>--- a/WebCore/page/EventHandler.cpp
>+++ b/WebCore/page/EventHandler.cpp
>@@ -430,7 +430,7 @@ bool EventHandler::handleMouseDraggedEvent(const MouseEventWithHitTestResults& e
>         // If the selection is contained in a layer that can scroll, that layer should handle the autoscroll
>         // Otherwise, let the bridge handle it so the view can scroll itself.
>         RenderObject* renderer = targetNode->renderer();
>-        while (renderer && (!renderer->isBox() || !toRenderBox(renderer)->canBeProgramaticallyScrolled(false))) {
>+        while (renderer && (!renderer->isBox() || !toRenderBox(renderer)->canBeScrolledAndHasScrollableArea())) {
>             if (!renderer->parent() && renderer->node() == renderer->document() && renderer->document()->ownerElement())
>                 renderer = renderer->document()->ownerElement()->renderer();
>             else
>@@ -720,7 +720,7 @@ void EventHandler::updateAutoscrollRenderer()
>     if (Node* nodeAtPoint = hitTest.innerNode())
>         m_autoscrollRenderer = nodeAtPoint->renderer();
> 
>-    while (m_autoscrollRenderer && (!m_autoscrollRenderer->isBox() || !toRenderBox(m_autoscrollRenderer)->canBeProgramaticallyScrolled(false)))
>+    while (m_autoscrollRenderer && (!m_autoscrollRenderer->isBox() || !toRenderBox(m_autoscrollRenderer)->canBeScrolledAndHasScrollableArea()))
>         m_autoscrollRenderer = m_autoscrollRenderer->parent();
> }
> 
>@@ -1160,7 +1160,7 @@ bool EventHandler::handleMousePressEvent(const PlatformMouseEvent& mouseEvent)
>     if (mouseEvent.button() == MiddleButton && !mev.isOverLink()) {
>         RenderObject* renderer = mev.targetNode()->renderer();
> 
>-        while (renderer && (!renderer->isBox() || !toRenderBox(renderer)->canBeProgramaticallyScrolled(false))) {
>+        while (renderer && (!renderer->isBox() || !toRenderBox(renderer)->canBeScrolledAndHasScrollableArea())) {
>             if (!renderer->parent() && renderer->node() == renderer->document() && renderer->document()->ownerElement())
>                 renderer = renderer->document()->ownerElement()->renderer();
>             else
>diff --git a/WebCore/rendering/RenderBox.cpp b/WebCore/rendering/RenderBox.cpp
>index 122be2f..907db4a 100644
>--- a/WebCore/rendering/RenderBox.cpp
>+++ b/WebCore/rendering/RenderBox.cpp
>@@ -410,6 +410,11 @@ bool RenderBox::scroll(ScrollDirection direction, ScrollGranularity granularity,
>         return b->scroll(direction, granularity, multiplier);
>     return false;
> }
>+
>+bool RenderBox::canBeScrolledAndHasScrollableArea() const
>+{
>+   return canBeProgramaticallyScrolled(false) && (scrollHeight() != clientHeight() || scrollWidth() != clientWidth());
>+}
>     
> bool RenderBox::canBeProgramaticallyScrolled(bool) const
> {
>diff --git a/WebCore/rendering/RenderBox.h b/WebCore/rendering/RenderBox.h
>index f4ad5e9..921443a 100644
>--- a/WebCore/rendering/RenderBox.h
>+++ b/WebCore/rendering/RenderBox.h
>@@ -230,6 +230,7 @@ public:
>     virtual int verticalScrollbarWidth() const;
>     int horizontalScrollbarHeight() const;
>     virtual bool scroll(ScrollDirection, ScrollGranularity, float multiplier = 1.0f);
>+    bool canBeScrolledAndHasScrollableArea() const;
>     virtual bool canBeProgramaticallyScrolled(bool) const;
>     virtual void autoscroll();
>     virtual void stopAutoscroll() { }
>diff --git a/WebCore/rendering/RenderLayer.cpp b/WebCore/rendering/RenderLayer.cpp
>index 00f827b..5f08055 100644
>--- a/WebCore/rendering/RenderLayer.cpp
>+++ b/WebCore/rendering/RenderLayer.cpp
>@@ -997,6 +997,9 @@ void RenderLayer::panScrollFromPoint(const IntPoint& sourcePoint)
> 
> void RenderLayer::scrollByRecursively(int xDelta, int yDelta)
> {
>+    if (!xDelta && !yDelta)
>+        return;
>+
>     bool restrictedByLineClamp = false;
>     if (renderer()->parent())
>         restrictedByLineClamp = renderer()->parent()->style()->lineClamp() >= 0;
>@@ -1006,17 +1009,29 @@ void RenderLayer::scrollByRecursively(int xDelta, int yDelta)
>         int newOffsetY = scrollYOffset() + yDelta;
>         scrollToOffset(newOffsetX, newOffsetY);
> 
>-        // If this layer can't do the scroll we ask its parent
>+        // If this layer can't do the scroll we ask the next layer up that can scroll to try
>         int leftToScrollX = newOffsetX - scrollXOffset();
>         int leftToScrollY = newOffsetY - scrollYOffset();
>         if ((leftToScrollX || leftToScrollY) && renderer()->parent()) {
>-            renderer()->parent()->enclosingLayer()->scrollByRecursively(leftToScrollX, leftToScrollY);
>+            RenderObject* nextRenderer = renderer()->parent();
>+            while (nextRenderer) {
>+                if (nextRenderer->isBox() && toRenderBox(nextRenderer)->canBeScrolledAndHasScrollableArea()) {
>+                    nextRenderer->enclosingLayer()->scrollByRecursively(leftToScrollX, leftToScrollY);
>+                    break;
>+                }
>+                nextRenderer = nextRenderer->parent();
>+            }
>+
>             Frame* frame = renderer()->document()->frame();
>             if (frame)
>                 frame->eventHandler()->updateAutoscrollRenderer();
>         }
>-    } else if (renderer()->view()->frameView())
>+    } else if (renderer()->view()->frameView()) {
>+        // If we are here, we were called on a renderer that can be programatically scrolled, but doesn't
>+        // have an overflow clip. Which means that it is a document node that can be scrolled.
>         renderer()->view()->frameView()->scrollBy(IntSize(xDelta, yDelta));
>+        // FIXME: If we didn't scroll the whole way, do we want to try looking at the frames ownerElement?
>+    }
> }
> 
> 
>-- 
>1.6.1
>

Flags:

eric: review+
eric: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 28023: 34157 | 34214 | 34328 | 34330 | 34495 | 34571 | 34572 | 34574