Bug 102970 – [WK1] REGRESSION (r129545): Main frame doesn't rubberband unless WebFrameLoadDelegate implements -webView:didFirstLayoutInFrame:

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

[Adam Roben (:aroben)]

Starting in r129545, the main frame no longer rubberbands in WebKit1 apps. It's easy to see this with any long content.

[Adam Roben (:aroben)]

I think I see the bug. In -[WebView _cacheFrameLoadDelegateImplementations] we have this code:

http://trac.webkit.org/browser/trunk/Source/WebKit/mac/WebView/WebView.mm?rev=129545#L1653

    // It would be nice to get rid of this code and transition all clients to using didLayout instead of
    // didFirstLayoutInFrame and didFirstVisuallyNonEmptyLayoutInFrame. In the meantime, this is required
    // for backwards compatibility.
    Page* page = core(self);
    if (page) {
        unsigned milestones = 0;
        if (cache->didFirstLayoutInFrameFunc)
            milestones |= DidFirstLayout;
        if (cache->didFirstVisuallyNonEmptyLayoutInFrameFunc)
            milestones |= DidFirstVisuallyNonEmptyLayout;
        page->addLayoutMilestones(static_cast<LayoutMilestones>(milestones));
    }

This means that we'll only listen for the DidFirstLayout milestone if the WebFrameLoadDelegate responds to -webView:didFirstLayoutInFrame:.

In WebFrameLoaderClient::dispatchDidLayout, we have this code:

http://trac.webkit.org/browser/trunk/Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm?rev=129545#L673

    if (milestones & DidFirstLayout) {
        // FIXME: We should consider removing the old didFirstLayout API since this is doing double duty with the
        // new didLayout API.
        if (implementations->didFirstLayoutInFrameFunc)
            CallFrameLoadDelegate(implementations->didFirstLayoutInFrameFunc, webView, @selector(webView:didFirstLayoutInFrame:), m_webFrame.get());

        // See WebFrameLoaderClient::provisionalLoadStarted.
        WebDynamicScrollBarsView *scrollView = [m_webFrame->_private->webFrameView _scrollView];
        if ([getWebView(m_webFrame.get()) drawsBackground])
            [scrollView setDrawsBackground:YES];
#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1070
        [scrollView setVerticalScrollElasticity:NSScrollElasticityAutomatic];
        [scrollView setHorizontalScrollElasticity:NSScrollElasticityAutomatic];
#endif
    }

So we'll only set the scroll view's elasticity if we get a DidFirstLayout milestone, but we'll only get that if the WebFrameLoadDelegate responds to -webView:didFirstLayoutInFrame:. (Note that not even implementing the new -webView:didLayout: method will get elasticity to work.)

[Adam Roben (:aroben)]

A simple fix would be to always ask for DidFirstLayout in WK1.

[Beth Dakin]

(In reply to comment #2)
> A simple fix would be to always ask for DidFirstLayout in WK1.

I think this fix makes sense. I did something similar for other ports that were doing necessary work when those events fire. I will post a patch in a minute!

[Beth Dakin]

Created attachment 176127 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 176127 [details]
Patch

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

> Source/WebKit/mac/ChangeLog:9
> +        We should always register for DidFirstLayout in WK1 since we do stork 

stork!

[Beth Dakin]

Created attachment 176337 [details]
Stork-free patch

[Adam Roben (:aroben)]

Comment on attachment 176337 [details]
Stork-free patch

This looks great to me. Probably worth having someone else take a look since I suggested the approach though. Thanks for jumping on it!

[Sam Weinig]

Comment on attachment 176337 [details]
Stork-free patch

Looks good to me too. r=me.

[Beth Dakin]

Excellent! Thanks, guys.

http://trac.webkit.org/changeset/136033

[Adam Roben (:aroben)]

Yay!

[Adam Roben (:aroben)]

I can confirm this is fixed in the r136085 nightly build.