Changeset 266016 in webkit

Timestamp:

Aug 21, 2020, 3:49:12 PM (5 years ago)

Author:

Simon Fraser

Message:

Add some FIXMEs in the EventHandler wheel event handling code for all the things that are wrong
​https://bugs.webkit.org/show_bug.cgi?id=215741

Reviewed by Wenson Hsieh.

There is much confusing code here. Trying to document the issues I've found.

• page/EventHandler.cpp:

(WebCore::EventHandler::handleWheelEvent):

• page/mac/EventHandlerMac.mm:

(WebCore::findEnclosingScrollableContainer):
(WebCore::EventHandler::determineWheelEventTarget):
(WebCore::EventHandler::processWheelEventForScrolling):

• page/scrolling/mac/ScrollingCoordinatorMac.mm:

(WebCore::ScrollingCoordinatorMac::handleWheelEvent):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• page/EventHandler.cpp (modified) (2 diffs)
• page/mac/EventHandlerMac.mm (modified) (4 diffs)
• page/scrolling/mac/ScrollingCoordinatorMac.mm (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-08-21  Simon Fraser  <simon.fraser@apple.com>
+
+        Add some FIXMEs in the EventHandler wheel event handling code for all the things that are wrong
+        https://bugs.webkit.org/show_bug.cgi?id=215741
+
+        Reviewed by Wenson Hsieh.
+
+        There is much confusing code here. Trying to document the issues I've found.
+
+        * page/EventHandler.cpp:
+        (WebCore::EventHandler::handleWheelEvent):
+        * page/mac/EventHandlerMac.mm:
+        (WebCore::findEnclosingScrollableContainer):
+        (WebCore::EventHandler::determineWheelEventTarget):
+        (WebCore::EventHandler::processWheelEventForScrolling):
+        * page/scrolling/mac/ScrollingCoordinatorMac.mm:
+        (WebCore::ScrollingCoordinatorMac::handleWheelEvent):
+
 2020-08-21  Rob Buis  <rbuis@igalia.com>
 

trunk/Source/WebCore/page/EventHandler.cpp

@@ -2918 +2918 @@
     document->hitTest(request, result);
 
+    // FIXME: Why do we have track all three of targetElement, scrollableContainer and ScrollableArea?
     RefPtr<Element> element = result.targetElement();
     RefPtr<ContainerNode> scrollableContainer;
     WeakPtr<ScrollableArea> scrollableArea;
     bool isOverWidget = result.isOverWidget();
+    
+    // FIXME: Using the value of isOverWidget from the latching state triggers double-recursion into subframes.
+    // FIXME: Despite doing this up-front search for the correct scrollable area, we dispatch events via elements which
+    // itself finds and tries to scroll overflow scrollers.
     determineWheelEventTarget(event, result, element, scrollableContainer, scrollableArea, isOverWidget);
 
@@ -2955 +2960 @@
         scrollableArea->setScrollShouldClearLatchedState(false);
 
+    // FIXME: processWheelEventForScrolling() is only called for FrameView scrolling, not overflow scrolling, which is confusing.
     bool handledEvent = processWheelEventForScrolling(event, scrollableContainer.get(), scrollableArea);
     processWheelEventForScrollSnap(event, scrollableArea);

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

@@ -808 +808 @@
             return candidate;
 
+        // FIXME: This needs to have the same axis bias that we use in other latching code.
         auto deltaX = wheelEvent.deltaX();
         auto deltaY = wheelEvent.deltaY();
@@ -975 +976 @@
                 scrollableArea = scrollableAreaForContainerNode(*scrollableContainer);
             else {
+                // FIXME: Why does this assume the body? What if we hit an iframe inside an overflow:scroll?
                 scrollableContainer = view->frame().document()->bodyOrFrameset();
                 scrollableArea = makeWeakPtr(static_cast<ScrollableArea&>(*view));
@@ -1010 +1012 @@
     }
 
+    // FIXME: This can use a stale laching state, before we just pushed or cleared.
     if (!wheelEvent.shouldResetLatching() && latchingState && latchingState->wheelEventElement()) {
         if (latchingIsLockedToPlatformFrame(m_frame))
@@ -1080 +1083 @@
         }
 
-        if (!latchingState->startedGestureAtScrollLimit())
+        if (!latchingState->startedGestureAtScrollLimit()) {
+            // FIXME: This set 'view' to a FrameView that is not this EventHandler's FrameView, which then gets scrolled from here, which is wrong.
             view = frameViewForLatchingState(m_frame, *latchingState);
+        }
 
         ASSERT(view);

trunk/Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm

@@ -83 +83 @@
 
     LOG_WITH_STREAM(Scrolling, stream << "ScrollingCoordinatorMac::handleWheelEvent - sending event to scrolling thread");
+    
+    // FIXME: Over on the scrolling thread, we'll hit-test the layers and possibly send the event to a node
+    // which we've already discounted on the main thread. This needs to target a specific node.
 
     RefPtr<ThreadedScrollingTree> threadedScrollingTree = downcast<ThreadedScrollingTree>(scrollingTree());