Bug 88070 – [Gtk] Process Gtk 3.4 smooth scroll events properly

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[José Dapena Paz]

Gtk 3.4 (3.3.18) has introduced a new direction for GtkEventScroll, GTK_SCROLL_SMOOTH. This direction allows to get more precise indication of the wheel scroll requested from input, using gdk_event_get_scroll_deltas.

We are getting the event both in WebKit1 and WebKit2.

On WebKit2, WebEventFactory::createWebWheelEvent will crash as we assert if the direction is not up/down/left/right. On WebKit1 we silently ignore the event as we won't get a delta for an unknown direction.

The solution is not simply adding the case for smooth scroll. For many case we get both smooth and standard scroll direction events, so we have to avoid processing one scroll event two times.

[José Dapena Paz]

(In reply to comment #0)
> The solution is not simply adding the case for smooth scroll. For many case we get both smooth and standard scroll direction events, so we have to avoid processing one scroll event two times.

Confirmed the expected outcome in Gtk is not duplicating event. We get the directional events or smooth (it does best effort). Anything else should be considered a bug in Gtk.

So the scope of this bug should be simply providing support for smooth scroll. This is basically adding GDK_SMOOTH_SCROLL_MASK to the events mask, and processing the GDK_SCROLL_SMOOTH direction properly.

[José Dapena Paz]

The bug fix can be tested with proper tests support in DumpRenderTree (bug 36003) and WebKitTestRunner (bug 69417). So setting dependencies.

[José Dapena Paz]

Created attachment 146041 [details]
Patch

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Martin Robinson]

Comment on attachment 146041 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146041&action=review

This change looks good, but I'm slightly confused how you are able to unskip tests because of this change, as you didn't modify the DRT. I think it makes sense to now implement continuousMouseScrollBy if possible.

> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:83
> +        case GDK_SCROLL_SMOOTH:
> +            {
> +                gdouble deltaX, deltaY;
> +                gdk_event_get_scroll_deltas((GdkEvent *) event,
> +                                            &deltaX,
> +                                            &deltaY);
> +                m_deltaX = -deltaX;
> +                m_deltaY = -deltaY;
> +            }
> +            break;

I think it makes more sense for the curly brace to be after GDK_SCROLL_SMOOTH, if possible, like this:

case GDK_SCROLL_SMOOTH: {
    gdouble deltaX, deltaY;
    ...
}

It matches more closely to WebKit style. You should use a static_cast for event instead of a C-style cast here. Also lines in WebKit typically go out to about 120 characters.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:158
> +    case GDK_SCROLL_SMOOTH:
> +        {

Same comment here as above.

[José Dapena Paz]

(In reply to comment #5)
> (From update of attachment 146041 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146041&action=review
> 
> This change looks good, but I'm slightly confused how you are able to unskip tests because of this change, as you didn't modify the DRT. I think it makes sense to now implement continuousMouseScrollBy if possible.

You need fix for 36003 (DumpRenderTree) and 69417 (WebKitTestRunner). The tests for this fix depend on them.

[Martin Robinson]

I think it would be nice to roll all these patches up into one patch. That way you can land the code change and the tests at once.

[José Dapena Paz]

Created attachment 146054 [details]
Patch

[Martin Robinson]

Comment on attachment 146054 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146054&action=review

Apologies as I didn't see that you added support for WebKit2 Mac. I think that part belongs in another patch. Thanks for extending this fix to WebKit2 as well. Can we unskip some tests for WebKit2? 

By the way, you might wish to take a look at the WebKit style guidelines (http://www.webkit.org/coding/coding-style.html) if you haven't already.

> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:83
> +        case GDK_SCROLL_SMOOTH:
> +            {
> +                gdouble deltaX, deltaY;
> +                gdk_event_get_scroll_deltas((GdkEvent *) event,
> +                                            &deltaX,
> +                                            &deltaY);
> +                m_deltaX = -deltaX;
> +                m_deltaY = -deltaY;
> +            }
> +            break;

Looks like you forgot this bit. :)

> Tools/ChangeLog:8
> +        WTR needs an implementation for eventSender.continuousMouseScrollBy
> +        https://bugs.webkit.org/show_bug.cgi?id=69417
> +
> +        Added continousMouseScrollBy support in WebKitTestRunner, and added
> +        implementation for gtk and mac, and stub for Qt.
> +

You'll need to combine these two changelog entries for this patch to land properly. It's probably easiest just to run generate-ChangeLogs again with just this bug number.

> Tools/DumpRenderTree/gtk/EventSender.cpp:84
> +// WebCore and layout tests assume this value
> +static const float pixelsPerScrollTick = 40.0f;

WebKit style guidelines suggest that you can just write:

// WebCore and layout tests assume this value.
 84static const float pixelsPerScrollTick = 40;

Note that comments that are complete sentences should have punctuation.

> Tools/DumpRenderTree/gtk/EventSender.cpp:441
> +

This is a bit of a nit, but I think this newline could be removed.

> Tools/DumpRenderTree/gtk/EventSender.cpp:443
> +    if ((horizontal && vertical) || horizontal > 1 || horizontal < -1 || vertical > 1 || vertical < -1) {

This code now handles horizontal and vertical scrolls that are great than 1 in either direction. Perhaps the comment above should mention that as well.

> Tools/DumpRenderTree/gtk/EventSender.cpp:452
> +#endif
> +    g_return_val_if_fail((!vertical || !horizontal), JSValueMakeUndefined(context));

Perhaps this should go into an #else block?

> Tools/DumpRenderTree/gtk/EventSender.cpp:471
> +    // GTK support continuous scroll events from 3.3.18

I think you can just remove this comment for now.

> Tools/DumpRenderTree/gtk/EventSender.cpp:480
> +    int horizontal = (int)JSValueToNumber(context, arguments[0], exception);

If this cast is really necessary it should be a static_cast. I'm not sure that it is necessary though.

> Tools/DumpRenderTree/gtk/EventSender.cpp:482
> +    int vertical = (int)JSValueToNumber(context, arguments[1], exception);

Ditto.

> Tools/DumpRenderTree/gtk/EventSender.cpp:485
> +    gboolean paged = false;

JSValueToBoolean returns a bool, so it's appropriate to use that type here.

> Tools/DumpRenderTree/gtk/EventSender.cpp:486
> +    if (argumentCount == 3) {

Perhaps this should be argumentCount >= 3 for future-proofing.

> Tools/WebKitTestRunner/InjectedBundle/Bindings/EventSendingController.idl:33
> +        void continuousMouseScrollBy (in long x, in long y, in [Optional] boolean paged);

You have an extra space after continuousMouseScrollBy here.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:48
> +// WebCore and layout tests assume this value
> +static const float pixelsPerScrollTick = 40.0f;

Same comment as above.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:414
> +    // We need smooth scroll events to implement this in Gtk+. In any case
> +    // we don't support paged scroll events.

Probably can just say "GTK+ does not support paged scroll events."

> Tools/WebKitTestRunner/mac/EventSenderProxy.mm:415
> +{
> +    RetainPtr<CGEventRef> cgScrollEvent(AdoptCF, CGEventCreateScrollWheelEvent(0, kCGScrollEventUnitPixel, 2, y, x));
> +
> +    // CGEvent locations are in global display coordinates.
> +    CGPoint lastGlobalMousePosition = {
> +        m_position.x,
> +        [[NSScreen mainScreen] frame].size.height - m_position.y
> +    };
> +    CGEventSetLocation(cgScrollEvent.get(), lastGlobalMousePosition);
> +
> +    NSEvent *event = [NSEvent eventWithCGEvent:cgScrollEvent.get()];
> +    NSView *targetView = [m_testController->mainWebView()->platformView() hitTest:[event locationInWindow]];
> +    if (targetView)
> +        [targetView scrollWheel:event];
> +}

This can probably be saved for another patch.

> LayoutTests/platform/gtk/TestExpectations:734
>  // mouseScrollBy() and continuousMouseScrollBy() are not yet implemented in the GTK EventSender API.
> -BUGWK36003 : fast/events/remove-child-onscroll.html = FAIL
> -BUGWK36003 : fast/events/platform-wheelevent-in-scrolling-div.html = FAIL
> -BUGWK36003 : fast/events/continuous-platform-wheelevent-in-scrolling-div.html = FAIL
> -BUGWK36003 : fast/events/wheelevent-direction-inverted-from-device.html = FAIL
>  BUGWK36003 : fast/events/wheelevent-in-horizontal-scrollbar-in-rtl.html = FAIL
>  BUGWK36003 : fast/events/wheelevent-in-vertical-scrollbar-in-rtl.html = FAIL
> -BUGWK36003 : fast/events/scroll-in-scaled-page-with-overflow-hidden.html = FAIL
>  BUGWK36003 : fast/events/platform-wheelevent-paging-x-in-non-scrolling-div.html = FAIL
>  BUGWK36003 : fast/events/platform-wheelevent-paging-x-in-non-scrolling-page.html = FAIL
>  BUGWK36003 : fast/events/platform-wheelevent-paging-x-in-scrolling-div.html = FAIL

Why are the rest of the tests failing? It would be good to update the comment and potentially open bugs for the remaining failures. Particularly continuousMouseScrollBy is implemented now, so the comment is inaccurate.

[José Dapena Paz]

Created attachment 146315 [details]
Patch

Style fixes, removed MAC implementation and updated test expectations

[Martin Robinson]

Comment on attachment 146315 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146315&action=review

This seems ready to go to me, though I have a couple lingering questions about some of the changes you made to the Skipped files.

> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:76
> +                gdk_event_get_scroll_deltas(reinterpret_cast<GdkEvent *>(event), &deltaX, &deltaY);

This is a bit of a nit, but you can just write: reinterpret_cast<GdkEvent*>(event) (without the space).

> LayoutTests/platform/gtk-wk2/Skipped:416
> +fast/events/scroll-in-scaled-page-with-overflow-hidden.html

I'm not sure I understand this change. The patch causes this test to start failing?

> LayoutTests/platform/wk2/Skipped:-1031
> -# WTR needs an implementation for eventSender.continuousMouseScrollBy
> -# https://bugs.webkit.org/show_bug.cgi?id=69417
> -fast/events/wheelevent-direction-inverted-from-device.html
> -

I'm not sure it's correct to unskip this test unless it's also skipped in the platform-specific files.  I think perhaps you may need to add this to LayoutTests/platform/qt-5.0-wk2/Skipped if Qt also doesn't have support for continuous scrolls.

[Martin Robinson]

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

[José Dapena Paz]

Created attachment 151697 [details]
Patch

[WebKit Review Bot]

Comment on attachment 151697 [details]
Patch

Attachment 151697 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13200327

New failing tests:
http/tests/security/mixedContent/filesystem-url-in-iframe.html

[WebKit Review Bot]

Created attachment 151704 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Build Bot]

Comment on attachment 151697 [details]
Patch

Attachment 151697 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13209367

[Martin Robinson]

Comment on attachment 151697 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151697&action=review

This patch looks fine for the most part to me, but it looks like it breaks the mac build.

> Tools/DumpRenderTree/gtk/EventSender.cpp:493
> +    bool paged = false;
> +    if (argumentCount >= 3) {
> +        paged = JSValueToBoolean(context, arguments[2]);

You don't usage paged outside of this scope so you should declare it in the inner scope.

> Tools/DumpRenderTree/gtk/EventSender.cpp:496
> +
> +        // Gtk+ does not support paged scroll events
> +        g_return_val_if_fail(!paged, JSValueMakeUndefined(context));

This whole thing can just be:

// We do not support paged scrolls yet.
g_return_val_if_fail(argumentCount < 3 || !JSValueToBoolean(context, arguments[2]), JSValueMakeUndefined(context));

> Tools/WebKitTestRunner/mac/EventSenderProxy.mm:402
> +    // not implemented

Should probably read 

// FIXME: Implement this.

as with other files.

[José Dapena Paz]

Created attachment 161165 [details]
Patch

Fixed mac build and updated coding style

[Gyuyoung Kim]

Comment on attachment 161165 [details]
Patch

Attachment 161165 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13680150

[José Dapena Paz]

Created attachment 161172 [details]
Patch

Fixed mac build and updated coding style

[WebKit Review Bot]

Comment on attachment 161172 [details]
Patch

Clearing flags on attachment: 161172

Committed r127070: <http://trac.webkit.org/changeset/127070>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.