Scroll latching logic can get stuck in 'scrollable="no"' iframes
authorbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 23 Mar 2015 22:52:27 +0000 (22:52 +0000)
committerbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 23 Mar 2015 22:52:27 +0000 (22:52 +0000)
https://bugs.webkit.org/show_bug.cgi?id=142789
<rdar://problem/20129494>

Reviewed by Dean Jackson.

Clean up the EventHandler and latching code as follows:
(1) Do not handle iframe elements as part of the normal latching logic. Instead, iframes should
    be evaluated during the 'platformCompleteWheelEvent' phase of processing as top-level scrolling
    frames.
(2) Get rid of the ill-conceived notation that we should process non-mainframe and main-frame frames
    different.
(3) Modify code to reflect that the scroll latching code really deals with overflow scrolling. Consequently,
    the 'findEnclosingScrollableContainer' was renamed to 'findEnclosingOverflowScroll' and does not
    treat iframe as a suitable target.
(4) Do not create a latching state object when the container being evaluated is already scrolled to the
    extreme position in the direction of the mouse gesture. In this case, we want the enclosing frame
    to be the latching target.
(5) Do not treat the state where the mouse wheel gesture has ended manual scrolling, but has not ended
    momentum scrolling, as an appropriate time to select a latching target.

* page/EventHandler.cpp:
(WebCore::EventHandler::platformCompleteWheelEvent): Modify signature to remove unneeded argument.
(WebCore::EventHandler::handleWheelEvent): Modify call to 'platformCompleteWheelEvent' to remove unused argument.
* page/EventHandler.h:
* page/mac/EventHandlerMac.mm:
(WebCore::findEnclosingOverflowScroll): Renamed from 'findEnclosingScrollableContainer' and revised per the
notes above.
(WebCore::EventHandler::platformPrepareForWheelEvents): Remove mainFrame vs. non-mainFrame code paths and
consolidate logic.
(WebCore::EventHandler::platformCompleteWheelEvent): Remove unused argument. The wheel event target is no
longer needed here, now that iframes are not processed by this code.
(WebCore::findEnclosingScrollableContainer): Deleted.
* page/scrolling/ScrollLatchingState.cpp:
(WebCore::ScrollLatchingState::setPreviousWheelScrolledElement:) Switch to move operator for passing
a temporary RefPtr to the the function.
* page/scrolling/ScrollLatchingState.h:
* platform/PlatformWheelEvent.h:
(WebCore::PlatformWheelEvent::useLatchedEventElement): Recognize 'phase=ended, momentum=none' as a state
that should not cause latching state to be revised.

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

Source/WebCore/ChangeLog
Source/WebCore/page/EventHandler.cpp
Source/WebCore/page/EventHandler.h
Source/WebCore/page/mac/EventHandlerMac.mm
Source/WebCore/page/scrolling/ScrollLatchingState.cpp
Source/WebCore/page/scrolling/ScrollLatchingState.h
Source/WebCore/platform/PlatformWheelEvent.h

index cf1eb3d..5ae5d14 100644 (file)
@@ -1,3 +1,46 @@
+2015-03-23  Brent Fulgham  <bfulgham@apple.com>
+
+        Scroll latching logic can get stuck in 'scrollable="no"' iframes
+        https://bugs.webkit.org/show_bug.cgi?id=142789
+        <rdar://problem/20129494>
+
+        Reviewed by Dean Jackson.
+
+        Clean up the EventHandler and latching code as follows:
+        (1) Do not handle iframe elements as part of the normal latching logic. Instead, iframes should
+            be evaluated during the 'platformCompleteWheelEvent' phase of processing as top-level scrolling
+            frames.
+        (2) Get rid of the ill-conceived notation that we should process non-mainframe and main-frame frames
+            different.
+        (3) Modify code to reflect that the scroll latching code really deals with overflow scrolling. Consequently,
+            the 'findEnclosingScrollableContainer' was renamed to 'findEnclosingOverflowScroll' and does not
+            treat iframe as a suitable target.
+        (4) Do not create a latching state object when the container being evaluated is already scrolled to the
+            extreme position in the direction of the mouse gesture. In this case, we want the enclosing frame
+            to be the latching target.
+        (5) Do not treat the state where the mouse wheel gesture has ended manual scrolling, but has not ended
+            momentum scrolling, as an appropriate time to select a latching target.
+
+        * page/EventHandler.cpp:
+        (WebCore::EventHandler::platformCompleteWheelEvent): Modify signature to remove unneeded argument.
+        (WebCore::EventHandler::handleWheelEvent): Modify call to 'platformCompleteWheelEvent' to remove unused argument.
+        * page/EventHandler.h:
+        * page/mac/EventHandlerMac.mm:
+        (WebCore::findEnclosingOverflowScroll): Renamed from 'findEnclosingScrollableContainer' and revised per the
+        notes above.
+        (WebCore::EventHandler::platformPrepareForWheelEvents): Remove mainFrame vs. non-mainFrame code paths and
+        consolidate logic.
+        (WebCore::EventHandler::platformCompleteWheelEvent): Remove unused argument. The wheel event target is no
+        longer needed here, now that iframes are not processed by this code.
+        (WebCore::findEnclosingScrollableContainer): Deleted.
+        * page/scrolling/ScrollLatchingState.cpp:
+        (WebCore::ScrollLatchingState::setPreviousWheelScrolledElement:) Switch to move operator for passing
+        a temporary RefPtr to the the function.
+        * page/scrolling/ScrollLatchingState.h:
+        * platform/PlatformWheelEvent.h:
+        (WebCore::PlatformWheelEvent::useLatchedEventElement): Recognize 'phase=ended, momentum=none' as a state
+        that should not cause latching state to be revised.
+
 2015-03-23  Anders Carlsson  <andersca@apple.com>
 
         Try to fix the iOS build.
index 91a4bd2..9a25203 100644 (file)
@@ -2612,7 +2612,7 @@ void EventHandler::platformRecordWheelEvent(const PlatformWheelEvent& event)
     m_frame.mainFrame().wheelEventDeltaTracker()->recordWheelEventDelta(event);
 }
 
-bool EventHandler::platformCompleteWheelEvent(const PlatformWheelEvent& event, Element*, ContainerNode*, ScrollableArea*)
+bool EventHandler::platformCompleteWheelEvent(const PlatformWheelEvent& event, ContainerNode*, ScrollableArea*)
 {
     // We do another check on the frame view because the event handler can run JS which results in the frame getting destroyed.
     FrameView* view = m_frame.view();
@@ -2708,7 +2708,7 @@ bool EventHandler::handleWheelEvent(const PlatformWheelEvent& event)
     if (scrollableArea)
         scrollableArea->setScrolledProgrammatically(false);
 
-    return platformCompleteWheelEvent(event, element.get(), scrollableContainer.get(), scrollableArea);
+    return platformCompleteWheelEvent(event, scrollableContainer.get(), scrollableArea);
 }
 
 void EventHandler::clearLatchedState()
index f221b09..915c10a 100644 (file)
@@ -206,7 +206,7 @@ public:
 
     void platformPrepareForWheelEvents(const PlatformWheelEvent&, const HitTestResult&, RefPtr<Element>& eventTarget, RefPtr<ContainerNode>& scrollableContainer, ScrollableArea*&, bool& isOverWidget);
     void platformRecordWheelEvent(const PlatformWheelEvent&);
-    bool platformCompleteWheelEvent(const PlatformWheelEvent&, Element* eventTarget, ContainerNode* scrollableContainer, ScrollableArea*);
+    bool platformCompleteWheelEvent(const PlatformWheelEvent&, ContainerNode* scrollableContainer, ScrollableArea*);
     bool platformCompletePlatformWidgetWheelEvent(const PlatformWheelEvent&, const Widget&, ContainerNode* scrollableContainer);
 
 #if ENABLE(CSS_SCROLL_SNAP)
index 58fcb50..89a7cec 100644 (file)
@@ -37,6 +37,8 @@
 #include "Frame.h"
 #include "FrameLoader.h"
 #include "FrameView.h"
+#include "HTMLDocument.h"
+#include "HTMLIFrameElement.h"
 #include "KeyboardEvent.h"
 #include "MainFrame.h"
 #include "MouseEventWithHitTestResults.h"
@@ -737,11 +739,17 @@ unsigned EventHandler::accessKeyModifiers()
     return PlatformEvent::CtrlKey | PlatformEvent::AltKey;
 }
 
-static ContainerNode* findEnclosingScrollableContainer(ContainerNode* node)
+static ContainerNode* findEnclosingOverflowScroll(ContainerNode* node)
 {
     // Find the first node with a valid scrollable area starting with the current
     // node and traversing its parents (or shadow hosts).
     for (ContainerNode* candidate = node; candidate; candidate = candidate->parentOrShadowHostNode()) {
+        if (is<HTMLIFrameElement>(candidate))
+            continue;
+
+        if (is<HTMLHtmlElement>(candidate) || is<HTMLDocument>(candidate))
+            return nullptr;
+
         RenderBox* box = candidate->renderBox();
         if (box && box->canBeScrolledAndHasScrollableArea())
             return candidate;
@@ -849,15 +857,14 @@ void EventHandler::platformPrepareForWheelEvents(const PlatformWheelEvent& wheel
 
     scrollableContainer = nullptr;
     scrollableArea = nullptr;
-    if (!view || !view->frame().isMainFrame()) {
+    if (!view)
         scrollableContainer = wheelEventTarget;
-        scrollableArea = view;
-    } else {
+    else {
         if (eventTargetIsPlatformWidget(wheelEventTarget.get())) {
             scrollableContainer = wheelEventTarget;
             scrollableArea = scrollViewForEventTarget(wheelEventTarget.get());
         } else {
-            scrollableContainer = findEnclosingScrollableContainer(wheelEventTarget.get());
+            scrollableContainer = findEnclosingOverflowScroll(wheelEventTarget.get());
             if (scrollableContainer) {
                 if (RenderBox* box = scrollableContainer->renderBox()) {
                     if (is<RenderListBox>(*box))
@@ -865,25 +872,30 @@ void EventHandler::platformPrepareForWheelEvents(const PlatformWheelEvent& wheel
                     else
                         scrollableArea = box->layer();
                 }
+            } else {
+                scrollableContainer = view->frame().document()->bodyOrFrameset();
+                scrollableArea = view;
             }
         }
     }
     
     ScrollLatchingState* latchingState = m_frame.mainFrame().latchingState();
     if (wheelEvent.shouldConsiderLatching()) {
-        m_frame.mainFrame().pushNewLatchingState();
-        latchingState = m_frame.mainFrame().latchingState();
-        if (scrollableArea && scrollableContainer)
-            latchingState->setStartedGestureAtScrollLimit(scrolledToEdgeInDominantDirection(*scrollableContainer, *scrollableArea, wheelEvent.deltaX(), wheelEvent.deltaY()));
-        else
-            latchingState->setStartedGestureAtScrollLimit(false);
-        latchingState->setWheelEventElement(wheelEventTarget);
-        latchingState->setFrame(&m_frame);
-        // FIXME: What prevents us from deleting this scrollable container while still holding a pointer to it?
-        latchingState->setScrollableContainer(scrollableContainer);
-        latchingState->setWidgetIsLatched(result.isOverWidget());
-        isOverWidget = latchingState->widgetIsLatched();
-        m_frame.mainFrame().wheelEventDeltaTracker()->beginTrackingDeltas();
+        if (scrollableContainer && scrollableArea) {
+            bool startingAtScrollLimit = scrolledToEdgeInDominantDirection(*scrollableContainer, *scrollableArea, wheelEvent.deltaX(), wheelEvent.deltaY());
+            if (!startingAtScrollLimit) {
+                m_frame.mainFrame().pushNewLatchingState();
+                latchingState = m_frame.mainFrame().latchingState();
+                latchingState->setStartedGestureAtScrollLimit(false);
+                latchingState->setWheelEventElement(wheelEventTarget);
+                latchingState->setFrame(&m_frame);
+                // FIXME: What prevents us from deleting this scrollable container while still holding a pointer to it?
+                latchingState->setScrollableContainer(scrollableContainer);
+                latchingState->setWidgetIsLatched(result.isOverWidget());
+                isOverWidget = latchingState->widgetIsLatched();
+                m_frame.mainFrame().wheelEventDeltaTracker()->beginTrackingDeltas();
+            }
+        }
     } else if (wheelEvent.shouldResetLatching())
         clearLatchedState();
 
@@ -923,40 +935,37 @@ static FrameView* frameViewForLatchingState(Frame& frame, ScrollLatchingState* l
     return latchingState->frame() ? latchingState->frame()->view() : frame.view();
 }
 
-bool EventHandler::platformCompleteWheelEvent(const PlatformWheelEvent& wheelEvent, Element* wheelEventTarget, ContainerNode* scrollableContainer, ScrollableArea* scrollableArea)
+bool EventHandler::platformCompleteWheelEvent(const PlatformWheelEvent& wheelEvent, ContainerNode* scrollableContainer, ScrollableArea* scrollableArea)
 {
     // We do another check on the frame view because the event handler can run JS which results in the frame getting destroyed.
     FrameView* view = m_frame.view();
 
     ScrollLatchingState* latchingState = m_frame.mainFrame().latchingState();
     if (wheelEvent.useLatchedEventElement() && latchingState && latchingState->scrollableContainer()) {
-        view = frameViewForLatchingState(m_frame, latchingState);
-        if (!view || !view->frame().isMainFrame()) {
-            bool didHandleWheelEvent = view && view->wheelEvent(wheelEvent);
-            if (scrollableContainer == latchingState->scrollableContainer()) {
-                // If we are just starting a scroll event, and have nowhere left to scroll, allow
-                // the enclosing frame to handle the scroll.
-                didHandleWheelEvent = !latchingState->startedGestureAtScrollLimit();
-                if (!didHandleWheelEvent)
-                    m_frame.mainFrame().popLatchingState();
-            }
 
-            // If the platform widget is handling the event, we always want to return false
-            if (view && scrollableArea == view && view->platformWidget())
-                didHandleWheelEvent = false;
-            
-            m_isHandlingWheelEvent = false;
-            return didHandleWheelEvent;
+        m_isHandlingWheelEvent = false;
+
+        // WebKit2 code path
+        if (!frameHasPlatformWidget(m_frame) && !latchingState->startedGestureAtScrollLimit() && scrollableContainer == latchingState->scrollableContainer() && scrollableArea && view != scrollableArea) {
+            // If we did not start at the scroll limit, do not pass the event on to be handled by enclosing scrollable regions.
+            return true;
         }
-        
-        if (scrollableArea && !latchingState->startedGestureAtScrollLimit() && scrollableContainer == latchingState->scrollableContainer()) {
-            m_isHandlingWheelEvent = false;
 
-            if (eventTargetIsPlatformWidget(wheelEventTarget))
-                return !latchingState->startedGestureAtScrollLimit();
+        if (!latchingState->startedGestureAtScrollLimit())
+            view = frameViewForLatchingState(m_frame, latchingState);
 
-            return true;
+        bool didHandleWheelEvent = view && view->wheelEvent(wheelEvent);
+        if (scrollableContainer == latchingState->scrollableContainer()) {
+            // If we are just starting a scroll event, and have nowhere left to scroll, allow
+            // the enclosing frame to handle the scroll.
+            didHandleWheelEvent = !latchingState->startedGestureAtScrollLimit();
         }
+
+        // If the platform widget is handling the event, we always want to return false.
+        if (view && scrollableArea == view && view->platformWidget())
+            didHandleWheelEvent = false;
+        
+        return didHandleWheelEvent;
     }
     
     bool didHandleEvent = view ? view->wheelEvent(wheelEvent) : false;
index cbd3d6c..e39d99e 100644 (file)
@@ -60,7 +60,7 @@ void ScrollLatchingState::setWidgetIsLatched(bool isOverWidget)
     m_widgetIsLatched = isOverWidget;
 }
 
-void ScrollLatchingState::setPreviousWheelScrolledElement(PassRefPtr<Element> element)
+void ScrollLatchingState::setPreviousWheelScrolledElement(RefPtr<Element>&& element)
 {
     m_previousWheelScrolledElement = element;
 }
index 7b7dcad..b1ddde5 100644 (file)
@@ -50,7 +50,7 @@ public:
     void setWidgetIsLatched(bool isOverWidget);
 
     Element* previousWheelScrolledElement() { return m_previousWheelScrolledElement.get(); }
-    void setPreviousWheelScrolledElement(PassRefPtr<Element>);
+    void setPreviousWheelScrolledElement(RefPtr<Element>&&);
     
     ContainerNode* scrollableContainer() { return m_scrollableContainer.get(); }
     void setScrollableContainer(PassRefPtr<ContainerNode>);
index 1f11fed..3c622ea 100644 (file)
@@ -165,7 +165,8 @@ namespace WebCore {
         bool useLatchedEventElement() const
         {
             return m_phase == PlatformWheelEventPhaseBegan || m_phase == PlatformWheelEventPhaseChanged
-                || m_momentumPhase == PlatformWheelEventPhaseBegan || m_momentumPhase == PlatformWheelEventPhaseChanged;
+                || m_momentumPhase == PlatformWheelEventPhaseBegan || m_momentumPhase == PlatformWheelEventPhaseChanged
+                || (m_phase == PlatformWheelEventPhaseEnded && m_momentumPhase == PlatformWheelEventPhaseNone);
         }
         bool shouldConsiderLatching() const
         {