Changeset 181879 in webkit


Ignore:
Timestamp:
Mar 23, 2015 3:52:27 PM (9 years ago)
Author:
Brent Fulgham
Message:

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.

Location:
trunk/Source/WebCore
Files:
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r181878 r181879  
     12015-03-23  Brent Fulgham  <bfulgham@apple.com>
     2
     3        Scroll latching logic can get stuck in 'scrollable="no"' iframes
     4        https://bugs.webkit.org/show_bug.cgi?id=142789
     5        <rdar://problem/20129494>
     6
     7        Reviewed by Dean Jackson.
     8
     9        Clean up the EventHandler and latching code as follows:
     10        (1) Do not handle iframe elements as part of the normal latching logic. Instead, iframes should
     11            be evaluated during the 'platformCompleteWheelEvent' phase of processing as top-level scrolling
     12            frames.
     13        (2) Get rid of the ill-conceived notation that we should process non-mainframe and main-frame frames
     14            different.
     15        (3) Modify code to reflect that the scroll latching code really deals with overflow scrolling. Consequently,
     16            the 'findEnclosingScrollableContainer' was renamed to 'findEnclosingOverflowScroll' and does not
     17            treat iframe as a suitable target.
     18        (4) Do not create a latching state object when the container being evaluated is already scrolled to the
     19            extreme position in the direction of the mouse gesture. In this case, we want the enclosing frame
     20            to be the latching target.
     21        (5) Do not treat the state where the mouse wheel gesture has ended manual scrolling, but has not ended
     22            momentum scrolling, as an appropriate time to select a latching target.
     23
     24        * page/EventHandler.cpp:
     25        (WebCore::EventHandler::platformCompleteWheelEvent): Modify signature to remove unneeded argument.
     26        (WebCore::EventHandler::handleWheelEvent): Modify call to 'platformCompleteWheelEvent' to remove unused argument.
     27        * page/EventHandler.h:
     28        * page/mac/EventHandlerMac.mm:
     29        (WebCore::findEnclosingOverflowScroll): Renamed from 'findEnclosingScrollableContainer' and revised per the
     30        notes above.
     31        (WebCore::EventHandler::platformPrepareForWheelEvents): Remove mainFrame vs. non-mainFrame code paths and
     32        consolidate logic.
     33        (WebCore::EventHandler::platformCompleteWheelEvent): Remove unused argument. The wheel event target is no
     34        longer needed here, now that iframes are not processed by this code.
     35        (WebCore::findEnclosingScrollableContainer): Deleted.
     36        * page/scrolling/ScrollLatchingState.cpp:
     37        (WebCore::ScrollLatchingState::setPreviousWheelScrolledElement:) Switch to move operator for passing
     38        a temporary RefPtr to the the function.
     39        * page/scrolling/ScrollLatchingState.h:
     40        * platform/PlatformWheelEvent.h:
     41        (WebCore::PlatformWheelEvent::useLatchedEventElement): Recognize 'phase=ended, momentum=none' as a state
     42        that should not cause latching state to be revised.
     43
    1442015-03-23  Anders Carlsson  <andersca@apple.com>
    245

  • trunk/Source/WebCore/page/EventHandler.cpp

    r181772 r181879  
    26132613}
    26142614
    2615 bool EventHandler::platformCompleteWheelEvent(const PlatformWheelEvent& event, Element*, ContainerNode*, ScrollableArea*)
     2615bool EventHandler::platformCompleteWheelEvent(const PlatformWheelEvent& event, ContainerNode*, ScrollableArea*)
    26162616{
    26172617    // We do another check on the frame view because the event handler can run JS which results in the frame getting destroyed.
    … …  
    27092709        scrollableArea->setScrolledProgrammatically(false);
    27102710
    2711     return platformCompleteWheelEvent(event, element.get(), scrollableContainer.get(), scrollableArea);
     2711    return platformCompleteWheelEvent(event, scrollableContainer.get(), scrollableArea);
    27122712}
    27132713

  • trunk/Source/WebCore/page/EventHandler.h

    r181687 r181879  
    207207    void platformPrepareForWheelEvents(const PlatformWheelEvent&, const HitTestResult&, RefPtr<Element>& eventTarget, RefPtr<ContainerNode>& scrollableContainer, ScrollableArea*&, bool& isOverWidget);
    208208    void platformRecordWheelEvent(const PlatformWheelEvent&);
    209     bool platformCompleteWheelEvent(const PlatformWheelEvent&, Element* eventTarget, ContainerNode* scrollableContainer, ScrollableArea*);
     209    bool platformCompleteWheelEvent(const PlatformWheelEvent&, ContainerNode* scrollableContainer, ScrollableArea*);
    210210    bool platformCompletePlatformWidgetWheelEvent(const PlatformWheelEvent&, const Widget&, ContainerNode* scrollableContainer);
    211211

  • trunk/Source/WebCore/page/mac/EventHandlerMac.mm

    r180974 r181879  
    3838#include "FrameLoader.h"
    3939#include "FrameView.h"
     40#include "HTMLDocument.h"
     41#include "HTMLIFrameElement.h"
    4042#include "KeyboardEvent.h"
    4143#include "MainFrame.h"
    … …  
    738740}
    739741
    740 static ContainerNode* findEnclosingScrollableContainer(ContainerNode* node)
     742static ContainerNode* findEnclosingOverflowScroll(ContainerNode* node)
    741743{
    742744    // Find the first node with a valid scrollable area starting with the current
    743745    // node and traversing its parents (or shadow hosts).
    744746    for (ContainerNode* candidate = node; candidate; candidate = candidate->parentOrShadowHostNode()) {
     747        if (is<HTMLIFrameElement>(candidate))
     748            continue;
     749
     750        if (is<HTMLHtmlElement>(candidate) || is<HTMLDocument>(candidate))
     751            return nullptr;
     752
    745753        RenderBox* box = candidate->renderBox();
    746754        if (box && box->canBeScrolledAndHasScrollableArea())
    … …  
    850858    scrollableContainer = nullptr;
    851859    scrollableArea = nullptr;
    852     if (!view || !view->frame().isMainFrame()) {
     860    if (!view)
    853861        scrollableContainer = wheelEventTarget;
    854         scrollableArea = view;
    855     } else {
     862    else {
    856863        if (eventTargetIsPlatformWidget(wheelEventTarget.get())) {
    857864            scrollableContainer = wheelEventTarget;
    858865            scrollableArea = scrollViewForEventTarget(wheelEventTarget.get());
    859866        } else {
    860             scrollableContainer = findEnclosingScrollableContainer(wheelEventTarget.get());
     867            scrollableContainer = findEnclosingOverflowScroll(wheelEventTarget.get());
    861868            if (scrollableContainer) {
    862869                if (RenderBox* box = scrollableContainer->renderBox()) {
    … …  
    866873                        scrollableArea = box->layer();
    867874                }
     875            } else {
     876                scrollableContainer = view->frame().document()->bodyOrFrameset();
     877                scrollableArea = view;
    868878            }
    869879        }
    … …  
    872882    ScrollLatchingState* latchingState = m_frame.mainFrame().latchingState();
    873883    if (wheelEvent.shouldConsiderLatching()) {
    874         m_frame.mainFrame().pushNewLatchingState();
    875         latchingState = m_frame.mainFrame().latchingState();
    876         if (scrollableArea && scrollableContainer)
    877             latchingState->setStartedGestureAtScrollLimit(scrolledToEdgeInDominantDirection(*scrollableContainer, *scrollableArea, wheelEvent.deltaX(), wheelEvent.deltaY()));
    878         else
    879             latchingState->setStartedGestureAtScrollLimit(false);
    880         latchingState->setWheelEventElement(wheelEventTarget);
    881         latchingState->setFrame(&m_frame);
    882         // FIXME: What prevents us from deleting this scrollable container while still holding a pointer to it?
    883         latchingState->setScrollableContainer(scrollableContainer);
    884         latchingState->setWidgetIsLatched(result.isOverWidget());
    885         isOverWidget = latchingState->widgetIsLatched();
    886         m_frame.mainFrame().wheelEventDeltaTracker()->beginTrackingDeltas();
     884        if (scrollableContainer && scrollableArea) {
     885            bool startingAtScrollLimit = scrolledToEdgeInDominantDirection(*scrollableContainer, *scrollableArea, wheelEvent.deltaX(), wheelEvent.deltaY());
     886            if (!startingAtScrollLimit) {
     887                m_frame.mainFrame().pushNewLatchingState();
     888                latchingState = m_frame.mainFrame().latchingState();
     889                latchingState->setStartedGestureAtScrollLimit(false);
     890                latchingState->setWheelEventElement(wheelEventTarget);
     891                latchingState->setFrame(&m_frame);
     892                // FIXME: What prevents us from deleting this scrollable container while still holding a pointer to it?
     893                latchingState->setScrollableContainer(scrollableContainer);
     894                latchingState->setWidgetIsLatched(result.isOverWidget());
     895                isOverWidget = latchingState->widgetIsLatched();
     896                m_frame.mainFrame().wheelEventDeltaTracker()->beginTrackingDeltas();
     897            }
     898        }
    887899    } else if (wheelEvent.shouldResetLatching())
    888900        clearLatchedState();
    … …  
    924936}
    925937
    926 bool EventHandler::platformCompleteWheelEvent(const PlatformWheelEvent& wheelEvent, Element* wheelEventTarget, ContainerNode* scrollableContainer, ScrollableArea* scrollableArea)
     938bool EventHandler::platformCompleteWheelEvent(const PlatformWheelEvent& wheelEvent, ContainerNode* scrollableContainer, ScrollableArea* scrollableArea)
    927939{
    928940    // We do another check on the frame view because the event handler can run JS which results in the frame getting destroyed.
    … …  
    931943    ScrollLatchingState* latchingState = m_frame.mainFrame().latchingState();
    932944    if (wheelEvent.useLatchedEventElement() && latchingState && latchingState->scrollableContainer()) {
    933         view = frameViewForLatchingState(m_frame, latchingState);
    934         if (!view || !view->frame().isMainFrame()) {
    935             bool didHandleWheelEvent = view && view->wheelEvent(wheelEvent);
    936             if (scrollableContainer == latchingState->scrollableContainer()) {
    937                 // If we are just starting a scroll event, and have nowhere left to scroll, allow
    938                 // the enclosing frame to handle the scroll.
    939                 didHandleWheelEvent = !latchingState->startedGestureAtScrollLimit();
    940                 if (!didHandleWheelEvent)
    941                     m_frame.mainFrame().popLatchingState();
    942             }
    943 
    944             // If the platform widget is handling the event, we always want to return false
    945             if (view && scrollableArea == view && view->platformWidget())
    946                 didHandleWheelEvent = false;
    947            
    948             m_isHandlingWheelEvent = false;
    949             return didHandleWheelEvent;
    950         }
    951        
    952         if (scrollableArea && !latchingState->startedGestureAtScrollLimit() && scrollableContainer == latchingState->scrollableContainer()) {
    953             m_isHandlingWheelEvent = false;
    954 
    955             if (eventTargetIsPlatformWidget(wheelEventTarget))
    956                 return !latchingState->startedGestureAtScrollLimit();
    957 
     945
     946        m_isHandlingWheelEvent = false;
     947
     948        // WebKit2 code path
     949        if (!frameHasPlatformWidget(m_frame) && !latchingState->startedGestureAtScrollLimit() && scrollableContainer == latchingState->scrollableContainer() && scrollableArea && view != scrollableArea) {
     950            // If we did not start at the scroll limit, do not pass the event on to be handled by enclosing scrollable regions.
    958951            return true;
    959952        }
     953
     954        if (!latchingState->startedGestureAtScrollLimit())
     955            view = frameViewForLatchingState(m_frame, latchingState);
     956
     957        bool didHandleWheelEvent = view && view->wheelEvent(wheelEvent);
     958        if (scrollableContainer == latchingState->scrollableContainer()) {
     959            // If we are just starting a scroll event, and have nowhere left to scroll, allow
     960            // the enclosing frame to handle the scroll.
     961            didHandleWheelEvent = !latchingState->startedGestureAtScrollLimit();
     962        }
     963
     964        // If the platform widget is handling the event, we always want to return false.
     965        if (view && scrollableArea == view && view->platformWidget())
     966            didHandleWheelEvent = false;
     967       
     968        return didHandleWheelEvent;
    960969    }
    961970   

  • trunk/Source/WebCore/page/scrolling/ScrollLatchingState.cpp

    r173784 r181879  
    6161}
    6262
    63 void ScrollLatchingState::setPreviousWheelScrolledElement(PassRefPtr<Element> element)
     63void ScrollLatchingState::setPreviousWheelScrolledElement(RefPtr<Element>&& element)
    6464{
    6565    m_previousWheelScrolledElement = element;

  • trunk/Source/WebCore/page/scrolling/ScrollLatchingState.h

    r175149 r181879  
    5151
    5252    Element* previousWheelScrolledElement() { return m_previousWheelScrolledElement.get(); }
    53     void setPreviousWheelScrolledElement(PassRefPtr<Element>);
     53    void setPreviousWheelScrolledElement(RefPtr<Element>&&);
    5454   
    5555    ContainerNode* scrollableContainer() { return m_scrollableContainer.get(); }

  • trunk/Source/WebCore/platform/PlatformWheelEvent.h

    r173034 r181879  
    166166        {
    167167            return m_phase == PlatformWheelEventPhaseBegan || m_phase == PlatformWheelEventPhaseChanged
    168                 || m_momentumPhase == PlatformWheelEventPhaseBegan || m_momentumPhase == PlatformWheelEventPhaseChanged;
     168                || m_momentumPhase == PlatformWheelEventPhaseBegan || m_momentumPhase == PlatformWheelEventPhaseChanged
     169                || (m_phase == PlatformWheelEventPhaseEnded && m_momentumPhase == PlatformWheelEventPhaseNone);
    169170        }
    170171        bool shouldConsiderLatching() const
Note: See TracChangeset for help on using the changeset viewer.