Summary: | Scrollable plugins not registered properly in ScrollingCoordinator | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nat Duca <nduca> | ||||||||||
Component: | WebCore Misc. | Assignee: | James Robinson <jamesr> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | andersca, enne, fishd, gustavo, jamesr, nduca, pnormand, tonikitoo, webkit.review.bot, xan.lopez | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Nat Duca
2012-03-25 19:36:29 PDT
Plugins are PluginViewBases, which are Widgets and thus scroll, but they aren't WebCore::ScrollableAreas so the computeNonFastScrollableRegion() test in ScrollingCoordinator ignores them. The fix is to check for scrollable plugins when iterating over Widgets. Created attachment 133844 [details]
Patch
This appears to work - any ideas on how to test? I was thinking of adding a DRT hook to dump the nonFastScrollableRegion as text on somewhere. Created attachment 133854 [details]
Patch
Comment on attachment 133854 [details] Patch Attachment 133854 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12134951 Comment on attachment 133854 [details] Patch Attachment 133854 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12128978 Comment on attachment 133844 [details]
Patch
I don't really like the idea of making all plug-ins be non-scrollable. Can we add a member function to PluginViewBase to let subclasses control this instead? (I'm really tempted to not send wheel events to some plug-ins that we know never handle them correctly)
(In reply to comment #7) > (From update of attachment 133844 [details]) > I don't really like the idea of making all plug-ins be non-scrollable. Can we add a member function to PluginViewBase to let subclasses control this instead? (I'm really tempted to not send wheel events to some plug-ins that we know never handle them correctly) I can do that, but then it means I can't make a layout test (since I don't know how to instantiate a plugin that will hook up to the correct subclass). Comment on attachment 133854 [details] Patch Attachment 133854 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12140014 Created attachment 133866 [details]
Patch
How about this? Other PluginViewBase subclasses would have to register themselves in the proper way, but I'm not familiar with how they all work. Comment on attachment 133866 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133866&action=review > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:142 > +Region ScrollingCoordinator::nonFastScrollableRegion() > +{ > + return computeNonFastScrollableRegion(m_page->mainFrame()->view()); > +} This change seems unrelated. > Source/WebCore/page/scrolling/ScrollingCoordinator.h:118 > + Region nonFastScrollableRegion(); Ditto. > Source/WebCore/plugins/PluginViewBase.h:56 > + virtual bool scrollable() { return false; } I think this should be called wantsWheelEvents instead, since a plug-in may want wheel events without being scrollable. Committed r112142: <http://trac.webkit.org/changeset/112142> Reopening since this doesn't properly handle a plugin becoming scrollable and could be done in a more straightforward way. Created attachment 133880 [details]
Patch
*** Bug 82164 has been marked as a duplicate of this bug. *** Comment on attachment 133880 [details] Patch Clearing flags on attachment: 133880 Committed r112325: <http://trac.webkit.org/changeset/112325> All reviewed patches have been landed. Closing bug. |