Skip to content

Commit

Permalink
REGRESSION (263995@main): preventDefault on wheel events can still re…
Browse files Browse the repository at this point in the history
…sult in page scrolling (fast/scrolling/mac/event-region-prevent-default-with-sublayer.html is a consistent failure)

https://bugs.webkit.org/show_bug.cgi?id=258741
rdar://111420041

Reviewed by Tim Horton.

263995@main broke the "only the first wheel event should be cancelable" logic such that if the page
called preventDefault() on a wheel event, we'd always time out the `m_waitingForBeganEventCondition`
condition, which results in us making a wheel event sequence that should be synchronous (i.e. page
can consume all events) into an async one (which can trigger scrolling).

This manifested as page scrolls in fast/scrolling/mac/event-region-prevent-default-with-sublayer.html.

There are two aspects to the fix. First, when we get back to `WebPageProxy::handleWheelEventReply()`
in the UI process with no nodeID (which means that the web process didn't use the event for main
thread scrolling for any node), we still call into the scrolling coordinator which calls into
`RemoteLayerTreeEventDispatcher::wheelEventHandlingCompleted()`. This flow eventually early
returns from `RemoteScrollingTreeMac::handleWheelEventAfterDefaultHandling()` if the nodeID is zero,
but this ensures that we always end up hitting WebPageProxy::wheelEventHandlingCompleted() which
is necessary.

Second, pull the code that notifies the m_waitingForBeganEventCondition into its own function that
is called on the main thread (previously we tried to notify the condition on the scrolling thread,
which was the waiting thread, which is why it always timed out).

* Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingTree.h:
(WebKit::RemoteScrollingTree::receivedEventAfterDefaultHandling):
* Source/WebKit/UIProcess/RemoteLayerTree/mac/RemoteLayerTreeEventDispatcher.cpp:
(WebKit::RemoteLayerTreeEventDispatcher::wheelEventHandlingCompleted):
* Source/WebKit/UIProcess/RemoteLayerTree/mac/RemoteScrollingTreeMac.h:
* Source/WebKit/UIProcess/RemoteLayerTree/mac/RemoteScrollingTreeMac.mm:
(WebKit::RemoteScrollingTreeMac::receivedEventAfterDefaultHandling):
(WebKit::RemoteScrollingTreeMac::handleWheelEventAfterDefaultHandling):
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::handleWheelEventReply):

Canonical link: https://commits.webkit.org/265781@main
  • Loading branch information
smfr committed Jul 5, 2023
1 parent 5b715db commit bdb0943
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ class RemoteScrollingTree : public WebCore::ScrollingTree {

virtual void willSendEventForDefaultHandling(const WebCore::PlatformWheelEvent&) { }
virtual void waitForEventDefaultHandlingCompletion(const WebCore::PlatformWheelEvent&) { }
virtual void receivedEventAfterDefaultHandling(const WebCore::PlatformWheelEvent&, std::optional<WebCore::WheelScrollGestureState>) { };
virtual WebCore::WheelEventHandlingResult handleWheelEventAfterDefaultHandling(const WebCore::PlatformWheelEvent&, WebCore::ScrollingNodeID, std::optional<WebCore::WheelScrollGestureState>) { return WebCore::WheelEventHandlingResult::unhandled(); }

RemoteScrollingCoordinatorProxy* scrollingCoordinatorProxy() const;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,9 @@ void RemoteLayerTreeEventDispatcher::wheelEventHandlingCompleted(const PlatformW

LOG_WITH_STREAM(WheelEvents, stream << "RemoteLayerTreeEventDispatcher::wheelEventHandlingCompleted " << wheelEvent << " - sending event to scrolling thread, node " << 0 << " gestureState " << gestureState);

if (auto scrollingTree = this->scrollingTree())
scrollingTree->receivedEventAfterDefaultHandling(wheelEvent, gestureState);

ScrollingThread::dispatch([protectedThis = Ref { *this }, wheelEvent, scrollingNodeID, gestureState] {
auto scrollingTree = protectedThis->scrollingTree();
if (!scrollingTree)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ class RemoteScrollingTreeMac final : public RemoteScrollingTree {
// "Default handling" here refers to sending the event to the web process for synchronous scrolling, and DOM event handling.
void willSendEventForDefaultHandling(const WebCore::PlatformWheelEvent&) override;
void waitForEventDefaultHandlingCompletion(const WebCore::PlatformWheelEvent&) override;
void receivedEventAfterDefaultHandling(const WebCore::PlatformWheelEvent&, std::optional<WebCore::WheelScrollGestureState>) override;

WheelEventHandlingResult handleWheelEventAfterDefaultHandling(const WebCore::PlatformWheelEvent&, WebCore::ScrollingNodeID, std::optional<WebCore::WheelScrollGestureState>) override;

void deferWheelEventTestCompletionForReason(WebCore::ScrollingNodeID, WebCore::WheelEventTestMonitor::DeferReason) override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -310,21 +310,37 @@
LOG_WITH_STREAM(Scrolling, stream << "RemoteScrollingTreeMac::waitForEventDefaultHandlingCompletion took " << (MonotonicTime::now() - startTime).milliseconds() << "ms - timed out " << !receivedEvent << " gesture state is " << gestureState());
}

void RemoteScrollingTreeMac::receivedEventAfterDefaultHandling(const WebCore::PlatformWheelEvent& wheelEvent, std::optional<WheelScrollGestureState> gestureState)
{
LOG_WITH_STREAM(Scrolling, stream << "RemoteScrollingTreeMac::receivedEventAfterDefaultHandling - " << wheelEvent << " gestureState " << gestureState);

ASSERT(isMainRunLoop());

if (!wheelEvent.isGestureStart())
return;

Locker locker { m_treeLock };

if (m_receivedBeganEventFromMainThread)
return;

setGestureState(gestureState);

m_receivedBeganEventFromMainThread = true;
m_waitingForBeganEventCondition.notifyOne();
}

WheelEventHandlingResult RemoteScrollingTreeMac::handleWheelEventAfterDefaultHandling(const PlatformWheelEvent& wheelEvent, ScrollingNodeID targetNodeID, std::optional<WheelScrollGestureState> gestureState)
{
LOG_WITH_STREAM(Scrolling, stream << "RemoteScrollingTreeMac::handleWheelEventAfterDefaultHandling - targetNodeID " << targetNodeID << " gestureState " << gestureState);

ASSERT(ScrollingThread::isCurrentThread());

if (!targetNodeID)
return WheelEventHandlingResult::unhandled();

Locker locker { m_treeLock };

if (!m_receivedBeganEventFromMainThread && wheelEvent.isGestureStart()) {
setGestureState(gestureState);

m_receivedBeganEventFromMainThread = true;
m_waitingForBeganEventCondition.notifyOne();
}

bool allowLatching = false;
OptionSet<WheelEventProcessingSteps> processingSteps;
if (gestureState.value_or(WheelScrollGestureState::Blocking) == WheelScrollGestureState::NonBlocking) {
Expand Down
8 changes: 3 additions & 5 deletions Source/WebKit/UIProcess/WebPageProxy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3363,11 +3363,9 @@ void WebPageProxy::handleWheelEventReply(const WebWheelEvent& event, ScrollingNo
MESSAGE_CHECK(m_process, wheelEventCoalescer().hasEventsBeingProcessed());

#if ENABLE(ASYNC_SCROLLING) && PLATFORM(MAC)
if (nodeID) {
if (auto* scrollingCoordinatorProxy = this->scrollingCoordinatorProxy()) {
scrollingCoordinatorProxy->wheelEventHandlingCompleted(platform(event), nodeID, gestureState);
return;
}
if (auto* scrollingCoordinatorProxy = this->scrollingCoordinatorProxy()) {
scrollingCoordinatorProxy->wheelEventHandlingCompleted(platform(event), nodeID, gestureState);
return;
}
#else
UNUSED_PARAM(event);
Expand Down

0 comments on commit bdb0943

Please sign in to comment.