Bug 232376 – [GLIB] twitch.tv forces synchronous scrolling

• Component: Scrolling
• Status: RESOLVED FIXED

[Chris Lord]

It seems smooth scrolling doesn't work for overflow areas when using sync scrolling (the default on GTK). No issues with async scrolling. You can see this on https://www.twitch.tv/ with mouse-wheel or keyboard initiated scrolling.

[Chris Lord]

Hmm, actually it was just Twitch that mislead me, they seem to override scrolling behaviour somehow and it's smooth with async scrolling enabled and non-smooth with it disabled... Possibly CSSOM scrolling is always enabled with async scrolling?

[Chris Lord]

Retitling this bug - Alejandro García Castro and I have been looking at this, and the issue is that Twitch.tv draws a custom scroll indicator and hooks onto all scroll events to do this. It seems just the act of hooking onto these events causes the region to be marked for main-thread-first event handling, and so ends up disabling asynchronous scrolling entirely. Because the page render performance is terrible and frequently updates, the scrolling also feels terrible.

I can't tell if this is handled differently in Chrome because the page render performance is so much better that it isn't obvious. However, in Firefox, I know this isn't how it's handled (both from observation and from having worked on the code in the past). Scrolling the same page in Firefox is perfectly smooth, but you can see the scroll indicator updating in a jerky manner, presumably because that's the actual page update performance.

Assuming Firefox hasn't changed and I recall correctly, when handling events like this in Firefox, the main thread is given a grace period to block the event in the scroll handler and if it can't respond in that time, the event will be delivered regardless. If subsequent events are part of an ongoing async scroll gesture, they will be delivered straight to the equivalent of the scrolling thread.

While I need to see if this is covered in any related standards, I think it would be a good idea to match the behaviour in Firefox. This page, for example, has regular hundred-millisecond rendering pauses on even desktop-class hardware. It is practically unusable on low-power embedded devices and completely negates the point of async scrolling. It should not be so easy to disable async scrolling (it doesn't actually block any of the scroll events, it just listens to them to update its scroll indicator).

[Radar WebKit Bug Importer]

<rdar://problem/85247010>

[Simon Fraser (smfr)]

Does the page call preventDefault() on the first wheel event in a gesture?

We have logic now to stay on the threaded scrolling path as long as the page doesn't do that. See wheelEventGesturesBecomeNonBlocking() code.

[Chris Lord]

(In reply to Simon Fraser (smfr) from comment #4)
> Does the page call preventDefault() on the first wheel event in a gesture?

No, I don't think so, but I need to confirm (if it does, then indeed, scrolling shouldn't work...) Can you confirm if async scrolling here works on Mac?

> We have logic now to stay on the threaded scrolling path as long as the page
> doesn't do that. See wheelEventGesturesBecomeNonBlocking() code.

Thanks - my guess is this isn't working on Linux because the event phases are a bit funky and/or because latching isn't working correctly (or maybe some other unimplemented thing). Will look into this and fix.

[Chris Lord]

The only code I can find about it that has a side-effect appears to be here:

https://github.com/WebKit/WebKit/blob/main/Source/WebCore/page/mac/EventHandlerMac.mm#L156

> bool EventHandler::wheelEvent(NSEvent *event)
> {
>     Page* page = m_frame.page();
>     if (!page)
>         return false;
> 
>     CurrentEventScope scope(event, nil);
>     auto wheelEvent = PlatformEventFactory::createPlatformWheelEvent(event, page->chrome().platformPageClient());
>     OptionSet<WheelEventProcessingSteps> processingSteps = { WheelEventProcessingSteps::MainThreadForScrolling, WheelEventProcessingSteps::MainThreadForBlockingDOMEventDispatch };
> 
>     if (wheelEvent.phase() == PlatformWheelEventPhase::Changed || wheelEvent.momentumPhase() == PlatformWheelEventPhase::Changed) {
>         if (m_frame.settings().wheelEventGesturesBecomeNonBlocking() && m_wheelScrollGestureState.value_or(WheelScrollGestureState::Blocking) == WheelScrollGestureState::NonBlocking)
>             processingSteps = { WheelEventProcessingSteps::MainThreadForScrolling, WheelEventProcessingSteps::MainThreadForNonBlockingDOMEventDispatch };
>     }
>     return handleWheelEvent(wheelEvent, processingSteps);
> }

Am I missing something there? It sets processingSteps to { WheelEventProcessingSteps::MainThreadForScrolling, WheelEventProcessingSteps::MainThreadForBlockingDOMEventDispatch }, then if the wheel event phase is changed and settings().wheelEventGesturesBecomeNonBlocking() is true and m_wheelScrollGestureState is WheelScrollGestureState::NonBlocking, sets is to { WheelEventProcessingSteps::MainThreadForScrolling, WheelEventProcessingSteps::MainThreadForBlockingDOMEventDispatch } - which is the exact same value? Does OptionSet have some kind of assignment overload that turns assignments into toggles?

Also this code needs to be replicated in a non-Mac specific place, but I'd like to understand it first...

[Chris Lord]

(In reply to Chris Lord from comment #6)
> The only code I can find about it that has a side-effect appears to be here:
> 
> https://github.com/WebKit/WebKit/blob/main/Source/WebCore/page/mac/
> EventHandlerMac.mm#L156
> 
> > bool EventHandler::wheelEvent(NSEvent *event)
> > {
> >     Page* page = m_frame.page();
> >     if (!page)
> >         return false;
> > 
> >     CurrentEventScope scope(event, nil);
> >     auto wheelEvent = PlatformEventFactory::createPlatformWheelEvent(event, page->chrome().platformPageClient());
> >     OptionSet<WheelEventProcessingSteps> processingSteps = { WheelEventProcessingSteps::MainThreadForScrolling, WheelEventProcessingSteps::MainThreadForBlockingDOMEventDispatch };
> > 
> >     if (wheelEvent.phase() == PlatformWheelEventPhase::Changed || wheelEvent.momentumPhase() == PlatformWheelEventPhase::Changed) {
> >         if (m_frame.settings().wheelEventGesturesBecomeNonBlocking() && m_wheelScrollGestureState.value_or(WheelScrollGestureState::Blocking) == WheelScrollGestureState::NonBlocking)
> >             processingSteps = { WheelEventProcessingSteps::MainThreadForScrolling, WheelEventProcessingSteps::MainThreadForNonBlockingDOMEventDispatch };
> >     }
> >     return handleWheelEvent(wheelEvent, processingSteps);
> > }
> 
> Am I missing something there? It sets processingSteps to {
> WheelEventProcessingSteps::MainThreadForScrolling,
> WheelEventProcessingSteps::MainThreadForBlockingDOMEventDispatch }, then if
> the wheel event phase is changed and
> settings().wheelEventGesturesBecomeNonBlocking() is true and
> m_wheelScrollGestureState is WheelScrollGestureState::NonBlocking, sets is
> to { WheelEventProcessingSteps::MainThreadForScrolling,
> WheelEventProcessingSteps::MainThreadForBlockingDOMEventDispatch } - which
> is the exact same value? Does OptionSet have some kind of assignment
> overload that turns assignments into toggles?
> 
> Also this code needs to be replicated in a non-Mac specific place, but I'd
> like to understand it first...

I am missing something, mainly the "Non" in "NonBlocking"... I'll excuse myself, it was late in the day.

[Chris Lord]

Created attachment 443933 [details]
Patch

[Chris Lord]

(In reply to Chris Lord from comment #8)
> Created attachment 443933 [details]
> Patch

This patch is deceptively simple, but requires some follow-up work for at least GTK and WPE.

After this patch, with async scrolling (only option on WPE) and wheelEventGesturesBecomeNonBlocking enabled (which is default true), all wheel event gestures will become non-blocking, even when there could be a valid begin phase, like with touchpad events.

Both GTK and WPE could support the "begin" phase for touchpad events on Wayland but don't - this will be follow-up work.


After this patch, behaviour now matches what happens on Mac, but I'm a bit uncertain what ought to happen with discrete mouse-wheel events. These events have no "begin" phase, for obvious reasons, but that also means that all of them will become non-blocking. Does this problem exist on Mac, or does Mac synthesise some kind of "begin" phase?

This patch now means that if a page tries to block scrolling by preventing default on the wheel event on a platform where the begin phase isn't correctly implemented, they only have ~8ms to do so. If a platform has no intention of implementing the begin phase, they can disable the wheelEventGesturesBecomeNonBlocking setting.

That said, I think the situation where you can so easily disable async scrolling is worse than the situation where badly-behaved pages that disable scrolling in a weird way don't operate quite as intended. For what it's worth, Firefox has a similar problem I believe, but it isn't as easy to trigger.

[Chris Lord]

Created attachment 443945 [details]
Patch

[Simon Fraser (smfr)]

(In reply to Chris Lord from comment #9)
> After this patch, behaviour now matches what happens on Mac, but I'm a bit
> uncertain what ought to happen with discrete mouse-wheel events. These
> events have no "begin" phase, for obvious reasons, but that also means that
> all of them will become non-blocking. Does this problem exist on Mac, or
> does Mac synthesise some kind of "begin" phase?

No, for clicky wheel mice on macOS each scroll is a mouseWheel with no phases, so there are no NonBlocking optimizations.

[Simon Fraser (smfr)]

Comment on attachment 443945 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443945&action=review

> Source/WebCore/page/scrolling/ScrollingTree.cpp:83
> +        if (isSynchronousDispatchRegion) {

On macOS we hit this code for <select> lists and non-async overflow, for reference.

> Source/WebCore/page/scrolling/ScrollingTree.cpp:84
> +            if (wheelEvent.isGestureContinuation() && wheelEventGesturesBecomeNonBlocking())

A few lines below is this code:

        if (m_treeState.gestureState.value_or(WheelScrollGestureState::Blocking) == WheelScrollGestureState::NonBlocking)

It's not clear to me whether that same test could be used here.

> Source/WebKit/WebProcess/WebPage/EventDispatcher.cpp:131
> +            if (processingSteps.contains(WheelEventProcessingSteps::MainThreadForScrolling)
> +                && processingSteps.contains(WheelEventProcessingSteps::MainThreadForBlockingDOMEventDispatch)) {

How does the event get to the main thread in the { WheelEventProcessingSteps::ScrollingThread, WheelEventProcessingSteps::MainThreadForNonBlockingDOMEventDispatch } case?

[Chris Lord]

Clearly the patch isn't quite right yet, at least for Mac... Still investigating.

(In reply to Simon Fraser (smfr) from comment #12)
> Comment on attachment 443945 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=443945&action=review
> 
> > Source/WebCore/page/scrolling/ScrollingTree.cpp:83
> > +        if (isSynchronousDispatchRegion) {
> 
> On macOS we hit this code for <select> lists and non-async overflow, for
> reference.

Out of curiosity, when does Mac use non-async overflow?

> > Source/WebCore/page/scrolling/ScrollingTree.cpp:84
> > +            if (wheelEvent.isGestureContinuation() && wheelEventGesturesBecomeNonBlocking())
> 
> A few lines below is this code:
> 
>         if
> (m_treeState.gestureState.value_or(WheelScrollGestureState::Blocking) ==
> WheelScrollGestureState::NonBlocking)
> 
> It's not clear to me whether that same test could be used here.

Ah, it probably should - I need to look at this code closer and experiment a bit...

> > Source/WebKit/WebProcess/WebPage/EventDispatcher.cpp:131
> > +            if (processingSteps.contains(WheelEventProcessingSteps::MainThreadForScrolling)
> > +                && processingSteps.contains(WheelEventProcessingSteps::MainThreadForBlockingDOMEventDispatch)) {
> 
> How does the event get to the main thread in the {
> WheelEventProcessingSteps::ScrollingThread,
> WheelEventProcessingSteps::MainThreadForNonBlockingDOMEventDispatch } case?

After this, it still gets sent to the main thread after the scrolling thread if the result of the scrolling tree scroll says it should - but I'm not certain the scrolling tree handler takes into account that the given processingSteps may require main thread scrolling, I'll verify that (and if not, either make sure it does or check processingSteps explicitly)

[Chris Lord]

Given that Mac scroll-wheel events are the same as non-Mac in this regard, I think the isGesture* functions are not good enough to determine whether to do this short-cutting (for want of a better term) or not.

Ideally, we want to always send the first event and short-cut when the relevant ScrollingEffectsController has either an active scroll animation or is in a scrolling gesture. I think I'd like to rework this to do that somehow.

[Chris Lord]

Created attachment 444256 [details]
Patch

[Chris Lord]

Created attachment 444260 [details]
Patch

[Chris Lord]

This is a slightly different approach to the last patch in that it doesn't involve any behavioural changes in ScrollingTree and instead lets EventDispatcher check if there's user scroll in progress and make a decision based on that.

Regarding the question of main thread events being dispatched in the async case, the scrolling tree event handler mirrors the main thread flags in its output which is then given to the main thread after the scrolling tree handles the event in the case of asynchronous scrolling - this was existing behaviour.

Really hoping this doesn't break anything...

[Chris Lord]

Created attachment 444362 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 444362 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444362&action=review

> Source/WebCore/page/scrolling/ScrollingTree.cpp:77
> +        return m_treeState.nodesWithActiveUserScrolls.contains(node->scrollingNodeID());

You could early out, avoiding the hit-test, if m_treeState.nodesWithActiveUserScrolls is empty.

> Source/WebCore/page/scrolling/ScrollingTree.h:103
> +    WEBCORE_EXPORT bool isUserScrollInProgress(const PlatformWheelEvent&);

I would call this isUserScrollInProgressAtPoint() or isUserScrollInProgressAtEventLocation()

> Source/WebKit/WebProcess/WebPage/EventDispatcher.cpp:127
> +        bool useMainThreadForScrolling = processingSteps.contains(WheelEventProcessingSteps::MainThreadForScrolling) && !(platformWheelEvent.phase() == PlatformWheelEventPhase::Changed && scrollingTree->isUserScrollInProgress(platformWheelEvent));

I would prefer to avoid that new hit-test on platforms that don't need it.

[Chris Lord]

Created attachment 444498 [details]
Patch

[Chris Lord]

(In reply to Simon Fraser (smfr) from comment #19)
> Comment on attachment 444362 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444362&action=review
> 
> > Source/WebCore/page/scrolling/ScrollingTree.cpp:77
> > +        return m_treeState.nodesWithActiveUserScrolls.contains(node->scrollingNodeID());
> 
> You could early out, avoiding the hit-test, if
> m_treeState.nodesWithActiveUserScrolls is empty.
> 
> > Source/WebCore/page/scrolling/ScrollingTree.h:103
> > +    WEBCORE_EXPORT bool isUserScrollInProgress(const PlatformWheelEvent&);
> 
> I would call this isUserScrollInProgressAtPoint() or
> isUserScrollInProgressAtEventLocation()
> 
> > Source/WebKit/WebProcess/WebPage/EventDispatcher.cpp:127
> > +        bool useMainThreadForScrolling = processingSteps.contains(WheelEventProcessingSteps::MainThreadForScrolling) && !(platformWheelEvent.phase() == PlatformWheelEventPhase::Changed && scrollingTree->isUserScrollInProgress(platformWheelEvent));
> 
> I would prefer to avoid that new hit-test on platforms that don't need it.

All addressed, thanks.

[Simon Fraser (smfr)]

Comment on attachment 444498 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444498&action=review

> Source/WebKit/WebProcess/WebPage/EventDispatcher.cpp:132
> +            useMainThreadForScrolling = false;

Why not clear the bit from processingSteps?

[Chris Lord]

(In reply to Simon Fraser (smfr) from comment #22)
> Comment on attachment 444498 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444498&action=review
> 
> > Source/WebKit/WebProcess/WebPage/EventDispatcher.cpp:132
> > +            useMainThreadForScrolling = false;
> 
> Why not clear the bit from processingSteps?

We still want to deliver the event to the main thread, we just don't want to block on it (unless I've misunderstood how things work here?) If I have, we can address in follow-up.

[EWS]

Committed r285992 (244389@main): <https://commits.webkit.org/244389@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444498 [details].