Bug 219547 – REGRESSION(r270425) [GTK] wheel scrolling stopped working

• Status: RESOLVED
• Resolution: FIXED
• Component: Scrolling

[Lauro Moura]

# Timeouts

fast/events/remove-child-onscroll.html
fast/events/wheel/wheel-event-destroys-frame.html
fast/events/wheel/wheel-event-destroys-overflow.html
fast/events/wheel/wheel-event-outside-body.html
fast/events/wheel/wheelevent-basic.html
fast/events/wheel/wheelevent-in-text-node.html
fast/events/wheel/wheelevent-mousewheel-interaction.html
fast/scrolling/iframe-scrollable-after-back.html
fast/scrolling/overflow-scrollable-after-back.html
pointer-lock/mouse-event-delivery.html
scrollbars/scroll-rtl-or-bt-layer.html

# Failures

fast/events/wheel/continuous-platform-wheelevent-in-scrolling-div.html
fast/frames/flattening/scrolling-in-object.html
fast/scrolling/rtl-scrollbars-listbox-scroll.html

Using the MiniBrowser, I can't scroll anything when async scrolling is enabled. Could not test disabling it yet, though.

[Lauro Moura]

Gardened in r270443.

[Lauro Moura]

*** Bug 219646 has been marked as a duplicate of this bug. ***

[Lauro Moura]

Created attachment 415835 [details]
Tentative patch

[Lauro Moura]

Comment on attachment 415835 [details]
Tentative patch

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

> LayoutTests/platform/gtk/fast/scrolling/overflow-scrollable-after-back-expected.txt:4
> +PASS: mouseWheel caused scrolling

Not sure if this extra scroll is right, though. The test sends 4 scroll phases (begin, changed, changed, ended), and the 3 first ones happen to trigger the callbacks.

[Simon Fraser (smfr)]

Comment on attachment 415835 [details]
Tentative patch

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

> Source/WebKit/WebProcess/WebPage/EventDispatcher.cpp:168
> +    auto scrollingTree = m_scrollingTrees.get(pageID);
> +    if (!scrollingTree)
> +        dispatchWheelEventViaMainThread(pageID, wheelEvent, processingSteps);

How does GTK work with ENABLE(SCROLLING_THREAD) but no scrolling tree? That combination makes no sense.

[Carlos Garcia Campos]

Async scrolling depends on accelerated compositing mode being enabled. In the GTK port, we have an accelerate compositing policy setting, that is ondemand by default, which means we enable AC mode when the page requires to enter in AC mode. Async scrolling requires AC mode to be always enabled, so we only enable it when the AC policy is set to always.

[Philippe Normand]

Ping reviewers

[Michael Catanzaro]

Hm, I think I'm not the right person to review this. But I understand that all scrolling is totally broken for a month now, so would be good to land.

I guess we have not been testing trunk very much in December. :P

[Alice Mikhaylenko]

All non-async scrolling*

Which means that at the very least it never happens with GTK4, so scrolling worked there the whole time.

So if you switch hardware acceleration policy to always and then open a new tab in minibrowser, it will work.

[EWS]

Committed r271235: <https://trac.webkit.org/changeset/271235>

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

[Radar WebKit Bug Importer]

<rdar://problem/72889749>

[Fujii Hironori]

This fix isn't perfect.
Filed: Bug 221067 – [GTK] Page scrolling by wheel events doesn't work in non-AC mode pages if Hardware Acceleration Policy is changed dynamically from "always" to "never"

[Fujii Hironori]

Comment on attachment 415835 [details]
Tentative patch

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

> Source/WebKit/WebProcess/WebPage/EventDispatcher.cpp:166
> +    auto scrollingTree = m_scrollingTrees.get(pageID);

You are accessing m_scrollingTrees without locking m_scrollingTreesMutex. Is this OK?

[Fujii Hironori]

(In reply to Fujii Hironori from comment #13)
> Comment on attachment 415835 [details]
> Tentative patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415835&action=review
> 
> > Source/WebKit/WebProcess/WebPage/EventDispatcher.cpp:166
> > +    auto scrollingTree = m_scrollingTrees.get(pageID);
> 
> You are accessing m_scrollingTrees without locking m_scrollingTreesMutex. Is
> this OK?

r272867 removed the suspicious code.

[Philippe Normand]

Thanks! I'll test this, I noticed wheel scrolling wasn't working anymore in the WPE MiniBrowser (and Cog). I'll open a new bug if the issue is still present.

[Fujii Hironori]

(In reply to Philippe Normand from comment #15)
> Thanks! I'll test this, I noticed wheel scrolling wasn't working anymore in
> the WPE MiniBrowser (and Cog). I'll open a new bug if the issue is still
> present.

r271235 and r272867 are fixes for non-AC mode (the case of no scrolling tree). WPE seems to have a different issue.