Bug 134569 – [Mac] WebKit1 WebView iframe not responding to scroll gestures

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

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 234291 [details]
Patch

[Brent Fulgham]

<rdar://problem/17480992>

[Brent Fulgham]

<rdar://problem/17309008>

[Brent Fulgham]

(In reply to comment #2)
> <rdar://problem/17480992>

Unrelated. Ignore this one.

[Simon Fraser (smfr)]

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?

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 234301 [details]
Patch

[Tim Horton]

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.

[Tim Horton]

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

[Tim Horton]

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

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 234303 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 234359 [details]
Patch

[Simon Fraser (smfr)]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 234368 [details]
Patch

[Brent Fulgham]

Committed r170765: <http://trac.webkit.org/changeset/170765>