- Revision
- 181879
- Author
- [email protected]
- Date
- 2015-03-23 15:52:27 -0700 (Mon, 23 Mar 2015)
Log 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.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (181878 => 181879)
--- trunk/Source/WebCore/ChangeLog 2015-03-23 22:50:39 UTC (rev 181878)
+++ trunk/Source/WebCore/ChangeLog 2015-03-23 22:52:27 UTC (rev 181879)
@@ -1,3 +1,46 @@
+2015-03-23 Brent Fulgham <[email protected]>
+
+ 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 <[email protected]>
Try to fix the iOS build.
Modified: trunk/Source/WebCore/page/EventHandler.cpp (181878 => 181879)
--- trunk/Source/WebCore/page/EventHandler.cpp 2015-03-23 22:50:39 UTC (rev 181878)
+++ trunk/Source/WebCore/page/EventHandler.cpp 2015-03-23 22:52:27 UTC (rev 181879)
@@ -2612,7 +2612,7 @@
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 @@
if (scrollableArea)
scrollableArea->setScrolledProgrammatically(false);
- return platformCompleteWheelEvent(event, element.get(), scrollableContainer.get(), scrollableArea);
+ return platformCompleteWheelEvent(event, scrollableContainer.get(), scrollableArea);
}
void EventHandler::clearLatchedState()
Modified: trunk/Source/WebCore/page/EventHandler.h (181878 => 181879)
--- trunk/Source/WebCore/page/EventHandler.h 2015-03-23 22:50:39 UTC (rev 181878)
+++ trunk/Source/WebCore/page/EventHandler.h 2015-03-23 22:52:27 UTC (rev 181879)
@@ -206,7 +206,7 @@
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)
Modified: trunk/Source/WebCore/page/mac/EventHandlerMac.mm (181878 => 181879)
--- trunk/Source/WebCore/page/mac/EventHandlerMac.mm 2015-03-23 22:50:39 UTC (rev 181878)
+++ trunk/Source/WebCore/page/mac/EventHandlerMac.mm 2015-03-23 22:52:27 UTC (rev 181879)
@@ -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 @@
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 @@
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 @@
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 @@
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;
Modified: trunk/Source/WebCore/page/scrolling/ScrollLatchingState.cpp (181878 => 181879)
--- trunk/Source/WebCore/page/scrolling/ScrollLatchingState.cpp 2015-03-23 22:50:39 UTC (rev 181878)
+++ trunk/Source/WebCore/page/scrolling/ScrollLatchingState.cpp 2015-03-23 22:52:27 UTC (rev 181879)
@@ -60,7 +60,7 @@
m_widgetIsLatched = isOverWidget;
}
-void ScrollLatchingState::setPreviousWheelScrolledElement(PassRefPtr<Element> element)
+void ScrollLatchingState::setPreviousWheelScrolledElement(RefPtr<Element>&& element)
{
m_previousWheelScrolledElement = element;
}
Modified: trunk/Source/WebCore/page/scrolling/ScrollLatchingState.h (181878 => 181879)
--- trunk/Source/WebCore/page/scrolling/ScrollLatchingState.h 2015-03-23 22:50:39 UTC (rev 181878)
+++ trunk/Source/WebCore/page/scrolling/ScrollLatchingState.h 2015-03-23 22:52:27 UTC (rev 181879)
@@ -50,7 +50,7 @@
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>);
Modified: trunk/Source/WebCore/platform/PlatformWheelEvent.h (181878 => 181879)
--- trunk/Source/WebCore/platform/PlatformWheelEvent.h 2015-03-23 22:50:39 UTC (rev 181878)
+++ trunk/Source/WebCore/platform/PlatformWheelEvent.h 2015-03-23 22:52:27 UTC (rev 181879)
@@ -165,7 +165,8 @@
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
{
