Bug 81779 – [GTK] Scrolling doesn't work in WebKit2 since r110185

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

Bug #16123 broke WebKit scrolling. For some reason the None scroll animator doesn't work for us in WebKit2. The only way to scroll is by using the scrollbars, but mouse wheel or keys don't work at all

[Zan Dobersek]

Works well with r109584, performing a bisection.

[Carlos Garcia Campos]

It works for me if I set m_scrollAnimatorEnabled to false in Settings, so I assumed r109584 introduced the regression.

[Zan Dobersek]

(In reply to comment #2)
> It works for me if I set m_scrollAnimatorEnabled to false in Settings, so I assumed r109584 introduced the regression.

Does that bring back only the simple scrolling capability or the entire smoothing effect?

[Carlos Garcia Campos]

(In reply to comment #3)
> (In reply to comment #2)
> > It works for me if I set m_scrollAnimatorEnabled to false in Settings, so I assumed r109584 introduced the regression.
> 
> Does that bring back only the simple scrolling capability or the entire smoothing effect?

I think it uses the old simple scrolling, by using ScrollAnimator::scroll() instead of ScrollAnimatorNone::scroll()

[Zan Dobersek]

FWIW, the same problem occurs in WebKit1 as well when enable-smooth-scrolling setting is set to true.

[Carlos Garcia Campos]

(In reply to comment #5)
> FWIW, the same problem occurs in WebKit1 as well when enable-smooth-scrolling setting is set to true.

That makes more sense. We are very late in the release cycle, I've added this bug to the list of things to fix before 1.8

https://trac.webkit.org/wiki/WebKitGTK/1.8.x

[Carlos Garcia Campos]

(In reply to comment #6)
> (In reply to comment #5)
> > FWIW, the same problem occurs in WebKit1 as well when enable-smooth-scrolling setting is set to true.
> 
> That makes more sense. We are very late in the release cycle, I've added this bug to the list of things to fix before 1.8
> 
> https://trac.webkit.org/wiki/WebKitGTK/1.8.x

Sorry, Zan told me that smooth scrolling didn't land in the branch, so I've just removed the bug from the wiki page.

[Zan Dobersek]

The offending commit is r110185
http://trac.webkit.org/changeset/110185

Changes made cause the animation scheduling to be done through ChromeClient only if the port is using display refresh monitor. Gtk is currently using display refresh timer (sort of a fallback), which was not accounted for in that revision. That's causing the scrolling animations not to be scheduled and done.

There's two ways of fixing this - use another timer in Chrome class in case the port is not using the display refresh monitor, or try to implement a display refresh monitor for the Gtk port.

I've looked up in Chromium how the display refresh monitor is done there. It seems that if the --disable-gpu-vsync switch is present, the animation interval is 0, meaning animation updates are made as fast as possible. Otherwise they're aiming for the standard 60FPS with animation intervals of 16 milliseconds.

If we'd be following that approach, implementing the display refresh monitor would be pretty straightforward, at least in WebKit1. I'd like some opinion before approaching this, though.

[Martin Robinson]

(In reply to comment #8)

> I've looked up in Chromium how the display refresh monitor is done there. It seems that if the --disable-gpu-vsync switch is present, the animation interval is 0, meaning animation updates are made as fast as possible. Otherwise they're aiming for the standard 60FPS with animation intervals of 16 milliseconds.

My understanding is that GPU Vsync doesn't really make as much sense in composited window managers. I I could be mistaken, but it makes sense to have an appropriate fallback for when the hardware support isn't present.

[Zan Dobersek]

(In reply to comment #8)
> Changes made cause the animation scheduling to be done through ChromeClient only if the port is using display refresh monitor. Gtk is currently using display refresh timer (sort of a fallback), which was not accounted for in that revision. That's causing the scrolling animations not to be scheduled and done.
> 

I actually got this part wrong. Chromium doesn't use neither display refresh monitor nor the refresh timer, but pipes the request through to ChromeClient (the WebViewImpl[1]). The request is then handled in Chromium and WebViewImpl::updateAnimations[2] is called, relaying the animation serving request back to FrameView.

I wonder if it would make sense to go along a similar way, exposing some API through which user would be able to throttle animation updates to desired refresh rate, enable vsync (if applicable) etc.

[1] http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/src/WebViewImpl.cpp#L3207
[2] http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/src/WebViewImpl.cpp#L1368

[Carlos Garcia Campos]

I would focus on making scrolling work again and then look for a solution to use the scroll animator, it's been broken for two weeks now.

[Zan Dobersek]

CC-ing author and reviewer from bug #78938 from which r110185 landed.

[Zan Dobersek]

Created attachment 135361 [details]
Patch

[Scott Byer]

What are the downstream products of the GTK port that need this in the short term, and can't deal with temporarily disabling the scroll animator?

The self-timer was always meant to be a short-term stopover, getting the animation timer going for GTK is definitely the better way forward.

If it's critical, this looks OK (an earlier patch in bug #78938 had the code like this).

[James Robinson]

Comment on attachment 135361 [details]
Patch

Looks OK.  A Timer is gonna give you very janky animations, though, you really want to be tied in to the rendering loop of the consumer (window manager or higher level application) to get smoothness.

[WebKit Review Bot]

Comment on attachment 135361 [details]
Patch

Clearing flags on attachment: 135361

Committed r113161: <http://trac.webkit.org/changeset/113161>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.