Bug 120542 - [wk2] REGRESSION (r154673): PDF scrolling moves very slowly
Summary: [wk2] REGRESSION (r154673): PDF scrolling moves very slowly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: PDF (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.8
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
: 120417 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-08-30 11:43 PDT by Matt Arsenault
Modified: 2013-09-08 22:18 PDT (History)
10 users (show)

See Also:


Attachments
Testcase PDF (57 bytes, text/plain)
2013-08-30 11:43 PDT, Matt Arsenault
no flags Details
Experiment #1 (1.25 KB, patch)
2013-08-31 02:19 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (5.73 KB, patch)
2013-09-08 00:37 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Arsenault 2013-08-30 11:43:20 PDT
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.
Comment 1 Tim Horton 2013-08-30 11:48:09 PDT
Super bizarre; I can reproduce. Dragging the scroll thumb or autoscrolling (via dragging) work fine.
Comment 2 Radar WebKit Bug Importer 2013-08-30 11:48:25 PDT
<rdar://problem/14881237>
Comment 3 Tim Horton 2013-08-30 12:36:01 PDT
Broke in http://trac.webkit.org/changeset/154673; Christophe, might you take a peek?
Comment 4 zalan 2013-08-31 01:57:47 PDT
*** Bug 120417 has been marked as a duplicate of this bug. ***
Comment 5 Chris Dumez 2013-08-31 02:01:35 PDT
I am investigating, sorry about the breakage. Unfortunately, it seems only Mac WK2 supports this PDF plugin so I cannot test / reproduce the issue.
Comment 6 Chris Dumez 2013-08-31 02:19:45 PDT
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.
Comment 7 Alexey Proskuryakov 2013-08-31 09:08:37 PDT
Chris, I suspect that this may be more like bug 87557, which had very similar symptoms.
Comment 8 Tim Horton 2013-09-07 16:10:49 PDT
(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.
Comment 9 Tim Horton 2013-09-07 16:27:47 PDT
FWIW I'm not seeing PDFPlugin::handleWheelEvent get hit *at all* now.
Comment 10 Tim Horton 2013-09-07 16:36:53 PDT
(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.
Comment 11 Tim Horton 2013-09-07 16:39:17 PDT
(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?
Comment 12 Tim Horton 2013-09-07 16:43:08 PDT
(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?
Comment 13 Chris Dumez 2013-09-07 23:16:07 PDT
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.
Comment 14 Tim Horton 2013-09-07 23:22:21 PDT
(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.
Comment 15 Chris Dumez 2013-09-07 23:35:10 PDT
(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.
Comment 16 Chris Dumez 2013-09-08 00:37:22 PDT
Created attachment 210966 [details]
Patch
Comment 17 Chris Dumez 2013-09-08 00:40:04 PDT
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.
Comment 18 Tim Horton 2013-09-08 15:42:39 PDT
Comment on attachment 210966 [details]
Patch

That works for me, and seems reasonable.
Comment 19 Tim Horton 2013-09-08 15:43:12 PDT
(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!
Comment 20 WebKit Commit Bot 2013-09-08 22:17:57 PDT
Comment on attachment 210966 [details]
Patch

Clearing flags on attachment: 210966

Committed r155321: <http://trac.webkit.org/changeset/155321>
Comment 21 WebKit Commit Bot 2013-09-08 22:18:01 PDT
All reviewed patches have been landed.  Closing bug.