Bug 120542 – [wk2] REGRESSION (r154673): PDF scrolling moves very slowly

• Component: PDF
• Status: RESOLVED FIXED

[Matt Arsenault]

Created attachment 210142 [details]
Testcase PDF

This PDF scrolls very slowly. Not graphically slowly, but slow as in it takes a lot of scrolling effort to move forward in the PDF. Scrolling the full height of my trackpad only advances about one line forward of text instead of most of a page like expected. The PDF scrolls normally in Preview.


Link to PDF: http://cs.brown.edu/courses/cs033/docs/x86_cheatsheet.pdf
I will also attach it in case it changes or goes away.

[Tim Horton]

Super bizarre; I can reproduce. Dragging the scroll thumb or autoscrolling (via dragging) work fine.

[Radar WebKit Bug Importer]

<rdar://problem/14881237>

[Tim Horton]

Broke in http://trac.webkit.org/changeset/154673; Christophe, might you take a peek?

[zalan]

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

[Chris Dumez]

I am investigating, sorry about the breakage. Unfortunately, it seems only Mac WK2 supports this PDF plugin so I cannot test / reproduce the issue.

[Chris Dumez]

Created attachment 210183 [details]
Experiment #1

Looking at my patch, I could find one small behavior change. WheelEvent::deltaX() / WheelEvent::deltaY() used to return a rounded integer and now they return a double value.
I figured this should not be too much of a problem because EventHandler's scrollNode() takes a floating point value in argument anyway.

This experimental patch attached rounds the value returned by WheelEvent::deltaX() / WheelEvent::deltaY() in EventHandler so that it should behave exactly like before. It would be great if someone could let me know if it has any impact on the PDF scrolling issue (since I don't run the mac port unfortunately).

I am not sure why this would impact PDF scrolling in WK2 at this point but I wasn't able to find any other behavior change with my patch yet.

[Alexey Proskuryakov]

Chris, I suspect that this may be more like bug 87557, which had very similar symptoms.

[Tim Horton]

(In reply to comment #6)
> Created an attachment (id=210183) [details]
> Experiment #1
> 
> Looking at my patch, I could find one small behavior change. WheelEvent::deltaX() / WheelEvent::deltaY() used to return a rounded integer and now they return a double value.
> I figured this should not be too much of a problem because EventHandler's scrollNode() takes a floating point value in argument anyway.
> 
> This experimental patch attached rounds the value returned by WheelEvent::deltaX() / WheelEvent::deltaY() in EventHandler so that it should behave exactly like before. It would be great if someone could let me know if it has any impact on the PDF scrolling issue (since I don't run the mac port unfortunately).

This patch does not fix the problem. I have a feeling Alexey's note is probably more accurate.

[Tim Horton]

FWIW I'm not seeing PDFPlugin::handleWheelEvent get hit *at all* now.

[Tim Horton]

(In reply to comment #9)
> FWIW I'm not seeing PDFPlugin::handleWheelEvent get hit *at all* now.

The event that's making it to PluginView is a "wheel" event, not a "mousewheel" event, so it doesn't get forwarded on to the Plugin.

[Tim Horton]

(In reply to comment #10)
> (In reply to comment #9)
> > FWIW I'm not seeing PDFPlugin::handleWheelEvent get hit *at all* now.
> 
> The event that's making it to PluginView is a "wheel" event, not a "mousewheel" event, so it doesn't get forwarded on to the Plugin.

... which is exactly what your change is about. Should we just be using "wheel" in WebKit2? Should that have been part of your patch?

[Tim Horton]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > FWIW I'm not seeing PDFPlugin::handleWheelEvent get hit *at all* now.
> > 
> > The event that's making it to PluginView is a "wheel" event, not a "mousewheel" event, so it doesn't get forwarded on to the Plugin.
> 
> ... which is exactly what your change is about. Should we just be using "wheel" in WebKit2? Should that have been part of your patch?

MediaControlsApple also uses mousewheelEvent. Should that be changed too? Did you by any chance grep the codebase for the "legacy" event you were changing?

[Chris Dumez]

Thanks for investigating this.

Note that my patch adds support for the 'wheel' event but does NOT remove support for the legacy 'mousewheel' event. Therefore, code that is still using the legacy event should in theory be fine. There are a lot of layout tests using it and still passing.

There is one case where the 'mousewheel' event no longer gets fired. This is when there is a listener of the 'wheel' event already. The 'mousewheel' is only fired as fallback (similarly to webkit- prefixed events). The assumption is that the web page either uses the new standard event, or the legacy one, but not both.

[Tim Horton]

(In reply to comment #13)
> Thanks for investigating this.
> 
> Note that my patch adds support for the 'wheel' event but does NOT remove support for the legacy 'mousewheel' event. Therefore, code that is still using the legacy event should in theory be fine. There are a lot of layout tests using it and still passing.
> 
> There is one case where the 'mousewheel' event no longer gets fired. This is when there is a listener of the 'wheel' event already. The 'mousewheel' is only fired as fallback (similarly to webkit- prefixed events). The assumption is that the web page either uses the new standard event, or the legacy one, but not both.

To a page, maybe, but what events will make it to e.g. PluginView::handleEvent? Where does the wheel/mouseWheel split happen? If a page uses wheel events, will a <object> inside it get mouseWheel events, or not?

None of the aforementioned layout tests test plugins, obviously, because plugins very clearly do not get mouseWheel events anymore.

[Chris Dumez]

(In reply to comment #14)
> (In reply to comment #13)
> > Thanks for investigating this.
> > 
> > Note that my patch adds support for the 'wheel' event but does NOT remove support for the legacy 'mousewheel' event. Therefore, code that is still using the legacy event should in theory be fine. There are a lot of layout tests using it and still passing.
> > 
> > There is one case where the 'mousewheel' event no longer gets fired. This is when there is a listener of the 'wheel' event already. The 'mousewheel' is only fired as fallback (similarly to webkit- prefixed events). The assumption is that the web page either uses the new standard event, or the legacy one, but not both.
> 
> To a page, maybe, but what events will make it to e.g. PluginView::handleEvent? Where does the wheel/mouseWheel split happen? If a page uses wheel events, will a <object> inside it get mouseWheel events, or not?
> 
> None of the aforementioned layout tests test plugins, obviously, because plugins very clearly do not get mouseWheel events anymore.

The split occurs in EventTarget.cpp. I don't know much about plugins but I will take a look as well.

[Chris Dumez]

Created attachment 210966 [details]
Patch

[Chris Dumez]

You were right, I missed several instances where the code is only checking for 'mousewheel' event name but not 'wheel'. I fixed that in this patch. I am confident this fixes the scrolling issue even though I haven't tested.

I initially grep'ed on Blink side to make sure I updated everything but failed to do the same on WebKit side when I ported the patch. Sorry about that.

[Tim Horton]

Comment on attachment 210966 [details]
Patch

That works for me, and seems reasonable.

[Tim Horton]

(In reply to comment #17)
> You were right, I missed several instances where the code is only checking for 'mousewheel' event name but not 'wheel'. I fixed that in this patch. I am confident this fixes the scrolling issue even though I haven't tested.
> 
> I initially grep'ed on Blink side to make sure I updated everything but failed to do the same on WebKit side when I ported the patch. Sorry about that.

That would do it :D Thanks for fixing!

[WebKit Commit Bot]

Comment on attachment 210966 [details]
Patch

Clearing flags on attachment: 210966

Committed r155321: <http://trac.webkit.org/changeset/155321>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.