[Chromium] Flash cannot receive scroll events when threaded compositing is in use

[Yusuke Sato]

https://code.google.com/p/chromium/issues/detail?id=158506

Chrome Version       : 23.0.1271.55
OS Version: 2913.140.0

URLs (if applicable) : http://harboursixty.com/
Other browsers tested:
  Add OK or FAIL after other browsers where you have tested this issue:
   IE 10: OK with trackpad (touch scrolling scrolls page)
   Chrome 23 on Linux: OK
   Chrome 23 on Windows: OK

What steps will reproduce the problem?
1. Load a flash app that has a scrollable element - eg. the menu at http://harboursixty.com/
2. Try to scroll the menu using the trackpad

What is the expected result?
Expect two-finger trackpad scrolling to scroll the element.

What happens instead of that?
Instead it scrolls the page.  The only way to scroll the element is to click and drag on the scrollbar.

Please provide any additional information below. Attach a screenshot if
possible.

UserAgentString: Mozilla/5.0 (X11; CrOS x86_64 2913.140.0) AppleWebKit/537.11 (KHTML, like Gecko) Chrome/23.0.1271.55 Safari/537.11

[Yusuke Sato]

Created attachment 172721 [details]
Patch

[Tony Chang]

I don't know anything about the compositor.  Adding people who do.

[James Robinson]

Comment on attachment 172721 [details]
Patch

Hmm - I would expect plugins to get added to the nonFastScrollableRegion for the given layer.  See ScrollingCoordinator::computeNonFastScrollableRegion()

Need tests as well.

[Yusuke Sato]

Created attachment 173753 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Yusuke Sato]

Thanks James. I took a look at the function as well as your recent changes against WebPluginContainerImpl.

I think the problem is that a plugin is added to nonFastScrollableRegion only when it uses a scroll bar, while some plugins including Flapper that don't use a scroll bar need to handle wheel events.
The second patch I just uploaded puts a plugin into the nonFastScrollableRegion (even if the plugin does not use a scroll bar) when the plugin explicitly requests wheel events. Chrome counterpart of the fix is http://codereview.chromium.org/11312195/ .

Re testing, I found that wheel events were already tested in LayoutTests/platform/chromium/plugins/. I'm new to WebKit but I guess the reason why chromium bots are currently green is that the bots are not using ACCELERATED_COMPOTITING yet. With my patch against Tools/DumpRenderTree/chromium/TestWebPlugin.cpp, I think the bots can be kept green even if ACCELERATED_COMPOTITING is turned on at some point in the future. (Please correct me if I'm wrong..)

Please take another look at patch #2.

[James Robinson]

Chromium always sets USE(ACCELERATED_COMPOSITING) to true.  The chromium port most likely wouldn't even compile if that was not set.  I suspect that there is something else going on.

[Yusuke Sato]

Created attachment 173789 [details]
Patch

[Yusuke Sato]

Thanks again, James. I found that I confused ACCELERATED_COMPOSITING with Chrome's threaded compositing. So what really happening here is as follows:

* On Chrome OS (M23-25, at least), the issue reproduces since Chrome's threaded compositing feature is enabled by default.
* On Chrome OS (M23-25, at least), the issue does not reproduce if threaded compositing is disabled via chrome://flags.

* On Mac (Chrome M24), the issue does not reproduce by default since threaded compositing is disabled by default.
* On Mac (Chrome M24), the issue reproduces if threaded compositing is enabled via chrome://flags.

When it reproduces, according to chrome://tracing results, Chrome's LayerImpl::tryScroll() takes this path:

    if (!scrollable()) {
        TRACE_EVENT0("cc", "LayerImpl::tryScroll: Ignored not scrollable");
        return InputHandlerClient::ScrollIgnored;
    }

and the event is discarded. My patch asks the compositor not to do so and to take the following path instead.

    if (type == InputHandlerClient::Wheel && haveWheelEventHandlers()) {
        TRACE_EVENT0("cc", "LayerImpl::tryScroll: Failed wheelEventHandlers");
        return InputHandlerClient::ScrollOnMainThread;
    }

I've updated the ChangeLog comments and removed #if USE(ACCELERATED_COMPOSITING) in WebPluginContainerImpl::setWantsWheelEvents().

I'm hoping the patch makes sense this time...

[James Robinson]

Comment on attachment 173789 [details]
Patch

Thanks for the thorough investigation.  Your explanation makes sense - if we the plugin doesn't have scrollbars, without this patch we assume it doesn't want wheel events at all which is definitely wrong.

R=me - set cq? if you would like a committer to land this for you.

[Yusuke Sato]

Thanks, since I'm not a WebKit committer, I've set cq?.

[WebKit Review Bot]

Comment on attachment 173789 [details]
Patch

Clearing flags on attachment: 173789

Committed r135178: <http://trac.webkit.org/changeset/135178>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.