Bug 134569 - [Mac] WebKit1 WebView iframe not responding to scroll gestures
Summary: [Mac] WebKit1 WebView iframe not responding to scroll gestures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on: 127386 128133 128225
Blocks: 136029
  Show dependency treegraph
 
Reported: 2014-07-02 15:53 PDT by Brent Fulgham
Modified: 2014-08-17 14:02 PDT (History)
9 users (show)

See Also:


Attachments
Patch (9.64 KB, patch)
2014-07-02 16:08 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (17.54 KB, patch)
2014-07-02 18:03 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (18.25 KB, patch)
2014-07-02 18:24 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (569.85 KB, application/zip)
2014-07-02 18:59 PDT, Build Bot
no flags Details
Patch (18.09 KB, patch)
2014-07-03 11:34 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (17.44 KB, patch)
2014-07-03 13:05 PDT, Brent Fulgham
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2014-07-02 15:53:01 PDT
My recent latching improvements broke iframe scroll handling for WK1 clients. This patch corrects this problem as follows:

1. WK1 scrollable areas are implemented on top of PlatformWidgets (i.e., NSViews).
2. NSView widgets are only given scroll commands when the EventHandler::handleWheelEvent returns 'false'.
3. For WK2 scrolling, we want EventHandler::handleWheelEvent to return 'true' when a scroll event has been completed.

The impedance mismatch here means that WK1 PlatformWidgets are not being issued the 'scroll' command, so things don't work properly.

The fix is as follows:

1. When a 'scroll' event has been handled by an inner platform widget, we want to make sure we block scrolling if the widget did not start the gesture at the scroll limit for the predominant scroll direction. This allows us to latch to an inner iframe so that scrolling is not propagated up to the main frame.
2. If we are latched to a platform widget during scroll, we always return 'false' so that the NSView receives a scroll event (WK1 case). In the WK2 case the scrolling element is not a platform widget, and we return true.
3. If we are scrolling an overflow div in WK1, we want to return false if we started at a scroll limit so that the scroll will propagate to the parent. For WK2 we always return true.
Comment 1 Brent Fulgham 2014-07-02 16:08:33 PDT
Created attachment 234291 [details]
Patch
Comment 2 Brent Fulgham 2014-07-02 16:11:02 PDT
<rdar://problem/17480992>
Comment 3 Brent Fulgham 2014-07-02 16:11:33 PDT
<rdar://problem/17309008>
Comment 4 Brent Fulgham 2014-07-02 16:14:35 PDT
(In reply to comment #2)
> <rdar://problem/17480992>

Unrelated. Ignore this one.
Comment 5 Simon Fraser (smfr) 2014-07-02 16:15:56 PDT
Comment on attachment 234291 [details]
Patch

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

r- for the  NSScrollView stuff and a better name for platformCompleteWidgetWheelEvent

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=134569

Is there a radar too?

> Source/WebCore/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).
> +

Please describe the problem and how you fixed it briefly here.

> Source/WebCore/page/EventHandler.cpp:2534
> +bool EventHandler::platformCompleteWidgetWheelEvent(const PlatformWheelEvent&, ContainerNode*, bool)

Not sure what a WidgetWheelEvent is.

> Source/WebCore/page/EventHandler.cpp:2593
> +            bool widgetIsPlatformWidget = !!widget->platformWidget();

You don't need the !! in C++

you could move this bool to just before the call to platformCompleteWidgetWheelEvent()

> Source/WebCore/page/mac/EventHandlerMac.mm:813
> +    if (!deltaIsPredominantlyVertical(deltaX, deltaY) && deltaX) {
> +        SEL horizontalScrollerSelector = NSSelectorFromString(@"horizontalScroller");
> +        if (![widget respondsToSelector:horizontalScrollerSelector])
> +            return true;
> +
> +        CGFloat horizontalScroll = [[widget performSelector:horizontalScrollerSelector] floatValue];
> +        if (deltaX < 0)
> +            return horizontalScroll == 1.0;
> +        
> +        return !horizontalScroll;
> +    }

This should do something with NSScrollView, not this selector stuff. if ([widget isKindOfClass:[NSScrollView class]])...

> Source/WebCore/page/mac/EventHandlerMac.mm:932
> +    if (!widgetIsPlatformWidget)
> +        return true;

Why bother passing widgetIsPlatformWidget if you just early return on it here?
Comment 6 Brent Fulgham 2014-07-02 18:03:22 PDT
Comment on attachment 234291 [details]
Patch

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

>> Source/WebCore/ChangeLog:4
>> +        https://bugs.webkit.org/show_bug.cgi?id=134569
> 
> Is there a radar too?

Done.

>> Source/WebCore/page/EventHandler.cpp:2534
>> +bool EventHandler::platformCompleteWidgetWheelEvent(const PlatformWheelEvent&, ContainerNode*, bool)
> 
> Not sure what a WidgetWheelEvent is.

I'll rename it to "platformCompletePlatformWidgetWheelEvent".  It's the special case of completing wheel event processing for PlatformWidgets.

>> Source/WebCore/page/EventHandler.cpp:2593
>> +            bool widgetIsPlatformWidget = !!widget->platformWidget();
> 
> You don't need the !! in C++
> 
> you could move this bool to just before the call to platformCompleteWidgetWheelEvent()

Done.

>> Source/WebCore/page/mac/EventHandlerMac.mm:813
>> +    }
> 
> This should do something with NSScrollView, not this selector stuff. if ([widget isKindOfClass:[NSScrollView class]])...

Done.

>> Source/WebCore/page/mac/EventHandlerMac.mm:932
>> +        return true;
> 
> Why bother passing widgetIsPlatformWidget if you just early return on it here?

Done. I'll remove the argument, and only call this method when we are working with a PlatformWidget.
Comment 7 Brent Fulgham 2014-07-02 18:03:50 PDT
Created attachment 234301 [details]
Patch
Comment 8 Tim Horton 2014-07-02 18:14:34 PDT
Comment on attachment 234301 [details]
Patch

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

> Source/WebCore/page/mac/EventHandlerMac.mm:799
> +    NSView* widget = platformWidgetForEventTarget(eventTarget);

star's on the wrong side.

> Source/WebCore/page/mac/EventHandlerMac.mm:806
> +    NSScrollView* scrollView = reinterpret_cast<NSScrollView*>(widget);

star's on the wrong side.
Comment 9 Tim Horton 2014-07-02 18:15:03 PDT
Comment on attachment 234301 [details]
Patch

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

> LayoutTests/platform/mac/fast/scrolling/scroll-iframe-fragment.html:22
> +    //debug("Page before: " + pageScrollPositionBefore + ", IFrame before: " + iFrameScrollPositionBefore);
> +    //debug("Page after:  " + pageScrollPositionAfter + ", IFrame after: " + iFrameScrollPositionAfter);

no thank you
Comment 10 Tim Horton 2014-07-02 18:15:58 PDT
Comment on attachment 234301 [details]
Patch

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

> LayoutTests/platform/mac/fast/scrolling/scroll-iframe-fragment.html:59
> +    setTimeout(checkForScroll, 100);

:(

> LayoutTests/platform/mac/fast/scrolling/scroll-iframe-fragment.html:63
> +

extra space

> LayoutTests/platform/mac/fast/scrolling/scroll-iframe-fragment.html:83
> + <div id='notToBeScrolled' style='height: 1000px; width: 1000px;'>

indentation of all of this stuff is weird
Comment 11 Brent Fulgham 2014-07-02 18:22:32 PDT
Comment on attachment 234301 [details]
Patch

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

>> Source/WebCore/page/mac/EventHandlerMac.mm:799
>> +    NSView* widget = platformWidgetForEventTarget(eventTarget);
> 
> star's on the wrong side.

Done.

>> Source/WebCore/page/mac/EventHandlerMac.mm:806
>> +    NSScrollView* scrollView = reinterpret_cast<NSScrollView*>(widget);
> 
> star's on the wrong side.

Done.

>> LayoutTests/platform/mac/fast/scrolling/scroll-iframe-fragment.html:22
>> +    //debug("Page after:  " + pageScrollPositionAfter + ", IFrame after: " + iFrameScrollPositionAfter);
> 
> no thank you

Then you shan't have any.

>> LayoutTests/platform/mac/fast/scrolling/scroll-iframe-fragment.html:63
>> +
> 
> extra space

Gone.

>> LayoutTests/platform/mac/fast/scrolling/scroll-iframe-fragment.html:83
>> + <div id='notToBeScrolled' style='height: 1000px; width: 1000px;'>
> 
> indentation of all of this stuff is weird

Fixed.
Comment 12 Brent Fulgham 2014-07-02 18:24:24 PDT
Created attachment 234303 [details]
Patch
Comment 13 Build Bot 2014-07-02 18:59:04 PDT
Comment on attachment 234303 [details]
Patch

Attachment 234303 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5196791385423872

New failing tests:
media/W3C/video/networkState/networkState_during_loadstart.html
Comment 14 Build Bot 2014-07-02 18:59:09 PDT
Created attachment 234307 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 15 Darin Adler 2014-07-03 10:12:06 PDT
Comment on attachment 234303 [details]
Patch

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

review- because of the strange always-null Element* argument (see comments below)

> Source/WebCore/page/EventHandler.cpp:2619
> +    return platformCompleteWheelEvent(e, element, scrollableContainer, scrollableArea);

This always passes null, because we have an early return above if element is non-null. I think it would be clearer to say nullptr here instead of “element”. But also, I just don’t see the point of adding this argument!

> Source/WebCore/page/mac/EventHandlerMac.mm:785
> +static NSView* platformWidgetForEventTarget(ContainerNode* eventTarget)

This should take en Element* rather than a ContainerNode*.

The return value should have the space on the other side because it’s an Objective-C type (although I want us to change that style rule soon).

> Source/WebCore/page/mac/EventHandlerMac.mm:788
> +        return nullptr;

Should be return nil, since this is an Objective-C object pointer.

> Source/WebCore/page/mac/EventHandlerMac.mm:792
> +        return nullptr;

Should be return nil, since this is an Objective-C object pointer.

> Source/WebCore/page/mac/EventHandlerMac.mm:794
> +    return toRenderWidget(target)->widget()->platformWidget();

What guarantees that widget() is non-null?

> Source/WebCore/page/mac/EventHandlerMac.mm:803
> +    if (![widget isKindOfClass: [NSScrollView class]])

Should not have a space after the colon here.

> Source/WebCore/page/mac/EventHandlerMac.mm:806
> +    NSScrollView *scrollView = reinterpret_cast<NSScrollView*>(widget);

It’s a little non-idiomatic to use reinterpret_cast here. Doesn’t static_cast work? Often we just use C-style casts for Objective-C object pointers. Also missing a space after NSScrollView before the *.

> Source/WebCore/page/mac/EventHandlerMac.mm:820
> +    if (!deltaIsPredominantlyVertical(deltaX, deltaY) && deltaX) {
> +        CGFloat horizontalScroll = scrollView.horizontalScroller.floatValue;
> +        if (deltaX < 0)
> +            return horizontalScroll == 1.0;
> +        
> +        return !horizontalScroll;
> +    }
> +
> +    CGFloat verticalScroll = scrollView.verticalScroller.floatValue;
> +    if (deltaY < 0)
> +        return verticalScroll == 1.0;
> +    
> +    return !verticalScroll;

I would write it like this instead:

    if (!deltaIsPredominantlyVertical(deltaX, deltaY) && deltaX)
        return scrollView.horizontalScroller.doubleValue == (deltaX >= 0 ? 0 : 1);
    return scrollView.verticalScroller.doubleValue == (deltaY >= 0 ? 0 : 1);

I think the tighter version is easier to read and floatValue is just a slower version of doubleValue that calls doubleValue and then converts to a float.
Comment 16 Brent Fulgham 2014-07-03 11:25:17 PDT
Comment on attachment 234303 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:2619
>> +    return platformCompleteWheelEvent(e, element, scrollableContainer, scrollableArea);
> 
> This always passes null, because we have an early return above if element is non-null. I think it would be clearer to say nullptr here instead of “element”. But also, I just don’t see the point of adding this argument!

It's because 'element' is not always null. We break out of the earlier code if the element renderer is not a widget, AND the element does not handle the dispatchWheelEvent internally. So we still need this argument.

>> Source/WebCore/page/mac/EventHandlerMac.mm:785
>> +static NSView* platformWidgetForEventTarget(ContainerNode* eventTarget)
> 
> This should take en Element* rather than a ContainerNode*.
> 
> The return value should have the space on the other side because it’s an Objective-C type (although I want us to change that style rule soon).

I support that change! I've always hated that style.

But I've changed the code to comply.

>> Source/WebCore/page/mac/EventHandlerMac.mm:788
>> +        return nullptr;
> 
> Should be return nil, since this is an Objective-C object pointer.

Done.

>> Source/WebCore/page/mac/EventHandlerMac.mm:792
>> +        return nullptr;
> 
> Should be return nil, since this is an Objective-C object pointer.

Done.

>> Source/WebCore/page/mac/EventHandlerMac.mm:794
>> +    return toRenderWidget(target)->widget()->platformWidget();
> 
> What guarantees that widget() is non-null?

I'll check and handle this possibility.

>> Source/WebCore/page/mac/EventHandlerMac.mm:803
>> +    if (![widget isKindOfClass: [NSScrollView class]])
> 
> Should not have a space after the colon here.

Done.

>> Source/WebCore/page/mac/EventHandlerMac.mm:806
>> +    NSScrollView *scrollView = reinterpret_cast<NSScrollView*>(widget);
> 
> It’s a little non-idiomatic to use reinterpret_cast here. Doesn’t static_cast work? Often we just use C-style casts for Objective-C object pointers. Also missing a space after NSScrollView before the *.

Done.

>> Source/WebCore/page/mac/EventHandlerMac.mm:820
>> +    return !verticalScroll;
> 
> I would write it like this instead:
> 
>     if (!deltaIsPredominantlyVertical(deltaX, deltaY) && deltaX)
>         return scrollView.horizontalScroller.doubleValue == (deltaX >= 0 ? 0 : 1);
>     return scrollView.verticalScroller.doubleValue == (deltaY >= 0 ? 0 : 1);
> 
> I think the tighter version is easier to read and floatValue is just a slower version of doubleValue that calls doubleValue and then converts to a float.

Done.
Comment 17 Brent Fulgham 2014-07-03 11:34:32 PDT
Created attachment 234359 [details]
Patch
Comment 18 Simon Fraser (smfr) 2014-07-03 11:58:41 PDT
Comment on attachment 234359 [details]
Patch

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

> Source/WebCore/page/mac/EventHandlerMac.mm:815
> +static bool scrolledToEdgeInDominantDirection(Element* eventTarget, float deltaX, float deltaY)
> +{
> +    NSView *widget = platformWidgetForEventTarget(eventTarget);
> +    if (!widget)
> +        return true;
> +
> +    if (![widget isKindOfClass:[NSScrollView class]])
> +        return true;
> +
> +    NSScrollView *scrollView = (NSScrollView *)widget;
> +
> +    if (!deltaIsPredominantlyVertical(deltaX, deltaY) && deltaX)
> +        return scrollView.horizontalScroller.doubleValue == (deltaX >= 0 ? 0 : 1);
> +    return scrollView.verticalScroller.doubleValue == (deltaY >= 0 ? 0 : 1);
> +}

This NSScrollView belongs to a ScrollView which is a ScrollableArea, so I'm confused about why you need this code at all. You should just be able to query a ScrollableArea in the caller.
Comment 19 Brent Fulgham 2014-07-03 12:31:32 PDT
When processing a wheel event for an iframe in WK1, the logic in 'findEnclosingScrollableContainer' skips the passed iframe, because it does not have the "hasOverflowClip" flag set.

However, instead of interacting with the PlatformWidget, I can talk to the Widget, which should be a ScrollView and ask it for its state.
Comment 20 Brent Fulgham 2014-07-03 13:05:51 PDT
Created attachment 234368 [details]
Patch
Comment 21 Brent Fulgham 2014-07-03 13:11:07 PDT
Committed r170765: <http://trac.webkit.org/changeset/170765>