Bug 136029 – [Mac] Gesture scrolls don't work in the WebKit1 clients after scrolling a non-scrollable iFrame

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Brent Fulgham]

Testing with various WebKit1 clients after my change for Iframe scroll gestures has revealed an edge case:

Under the following conditions:
1. A Cocoa application embeds a WebView using WK1.
2. The webview consists of an Iframe.
3. The Iframe size is the same as the WebView, so that there is no scrollable region.

If the user performs a scroll wheel gesture on the web view, other scrollable portions of the user interface become "locked up".

[Brent Fulgham]

<rdar://problem/17767169>

[Brent Fulgham]

Analysis indicates that WebKit is issuing two scrollWheel events during the handling of this gesture. The two events are intercepted at the OS level and result in some undesirable side effects that end up locking the view.

It looks like the nuance I missed in my earlier work was that although we bail out early when interacting with IFrames in the general case (under WK1), when we pass through the EventHandler::passWheelEventToWidget for an IFrame we actually DO dispatch the 'scrollWheel' event to the NSScrollView backing the IFrame.

This seems to set up the multiple-scrollWheel event condition found in Dictionary.app.

I think the correct fix is to recognize a WK1 case where the widget to which we just passed the wheel event is an frame view (IFrame), to not dispatch a second scrollWheel event by returning 'true' in this case.

[Brent Fulgham]

Created attachment 236736 [details]
Patch

[Sam Weinig]

Since this depends on host application logic, it might be a good candidate for a TestWebKitAPI test.

[Maciej Stachowiak]

Comment on attachment 236736 [details]
Patch

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

> Source/WebCore/page/mac/EventHandlerMac.mm:923
> +    // If the platform widget is a frame view (IFrame), and we are in WebKit1, 'passWheelEventToWidget' has already
> +    // called 'scrollWheel' on the underlying platform widget. We need to return true in this case to avoid getting
> +    // multiple copies of the scrollWheel event sent to the platform widget.

IS such a long comment really necessary? I'd suggest no comment, or a one-liner stating the reason.

[Brent Fulgham]

Committed r172703: <http://trac.webkit.org/changeset/172703>

[Brent Fulgham]

(In reply to comment #5)
> (From update of attachment 236736 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236736&action=review
> 
> > Source/WebCore/page/mac/EventHandlerMac.mm:923
> > +    // If the platform widget is a frame view (IFrame), and we are in WebKit1, 'passWheelEventToWidget' has already
> > +    // called 'scrollWheel' on the underlying platform widget. We need to return true in this case to avoid getting
> > +    // multiple copies of the scrollWheel event sent to the platform widget.
> 
> IS such a long comment really necessary? I'd suggest no comment, or a one-liner stating the reason.

OK! Fixed while landing.

[Brent Fulgham]

See Bug 136040 for work on creating a TestWebKitAPI test for this condition.

[Darin Adler]

Comment on attachment 236736 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2658
> +bool EventHandler::platformCompletePlatformWidgetWheelEvent(const PlatformWheelEvent&, const Widget*, ContainerNode*)

Since this can never be null, it should be a Widget& rather than a Widget*.

[Brent Fulgham]

Comment on attachment 236736 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:2658
>> +bool EventHandler::platformCompletePlatformWidgetWheelEvent(const PlatformWheelEvent&, const Widget*, ContainerNode*)
> 
> Since this can never be null, it should be a Widget& rather than a Widget*.

Good point! I'll land this as a follow-up patch.

[Brent Fulgham]

Added Darin's suggested clean-up:

Committed r172705: <http://trac.webkit.org/changeset/172705>

[Brent Fulgham]

Correct a bad merge from my last change:

Committed in r172720. <http://trac.webkit.org/changeset/172720>