Autosize should use documentRect height instead of scrollHeight

[Tim Horton]

Created attachment 193966 [details]
testcase

We've found a few cases where autosizing will get the wrong size with overflow. I'll attach one of them here. The solution *seems* to be to use RenderView::documentRect's height, which gets the right height, instead of scrollHeight, which gets the wrong (very short) clipped height.

<rdar://problem/13202277>

[Tim Horton]

Created attachment 193967 [details]
patch

[Tim Horton]

Autosizing on Mac doesn't actually resize the view, it waits for the view to be laid-out by autolayout, and this only works for WebKit2 anyway. Making this testable on Mac would be a good chunk of work.

I might consider adding a test that works on other platforms but is skipped on Mac, just so others don't regress this.

[David Levin]

You could consider adding drt support like what was done in http://trac.webkit.org/changeset/104398

I only did it for chromium but it was the only port using the feature at the time.

[David Levin]

I was trying to remember why I used that (and I finally tracked it back to the chromium code that had inspired it:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/render_view_impl.cc&q=scrollHeight%20file:%5C.cc&sq=package:chromium&type=cs&l=3961), so I'll admit to not having a large bias for scrollHeight over documentRect.



In fact after some more code investigation, it looks like the content size is set based on documentRect (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/WebCore/page/FrameView.cpp&sq=package:chromium&type=cs&rcl=1363736896&l=593) and that is how scrollbars are determined so this seems very consistent with what this function is trying to do.

Thanks!


You may want to consider adding some simple drt support (like what I mentioned below), but I don't think that should block your fix. (Just to help you sleep better at night knowing that folks are much less likely to break you at random, you may want to consider it in the future.)

[Tim Horton]

(In reply to comment #4)
> I was trying to remember why I used that (and I finally tracked it back to the chromium code that had inspired it:
> https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/render_view_impl.cc&q=scrollHeight%20file:%5C.cc&sq=package:chromium&type=cs&l=3961), so I'll admit to not having a large bias for scrollHeight over documentRect.
> 
> 
> 
> In fact after some more code investigation, it looks like the content size is set based on documentRect 

^^ this is exactly what made me pretty sure that we want this change. Our use of autosizing involves the WKView being sized to the FrameView contentsSize, and that was getting the right number, but we'd only paint a few pixels right at the top :)

(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/WebCore/page/FrameView.cpp&sq=package:chromium&type=cs&rcl=1363736896&l=593) and that is how scrollbars are determined so this seems very consistent with what this function is trying to do.
> 
> Thanks!
> 
> 
> You may want to consider adding some simple drt support (like what I mentioned below), but I don't think that should block your fix. (Just to help you sleep better at night knowing that folks are much less likely to break you at random, you may want to consider it in the future.)

Agreed, I definitely want to work out how to make testing go.

Thanks for the review!

[Tim Horton]

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

[Tim Horton]

This broke autosizing entirely, it will now never shrink to be smaller than it was before. David, do you know if it's unreasonable to remove the condition from this call?

    // If this is the first time we run autosize, start from small height and
    // allow it to grow.
    if (!m_didRunAutosize)
         resize(frameRect().width(), m_minAutoSize.height());

If we shrink the FrameView each time, the documentRect is right. If we don't, the documentRect never gets smaller than it has previously been.

[Tim Horton]

(In reply to comment #7)
> This broke autosizing entirely, it will now never shrink to be smaller than it was before. David, do you know if it's unreasonable to remove the condition from this call?
> 
>     // If this is the first time we run autosize, start from small height and
>     // allow it to grow.
>     if (!m_didRunAutosize)
>          resize(frameRect().width(), m_minAutoSize.height());
> 
> If we shrink the FrameView each time, the documentRect is right. If we don't, the documentRect never gets smaller than it has previously been.

But there are presumably performance implications.

[Tim Horton]

Created attachment 196045 [details]
better patch!

[Tim Horton]

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

[Michael Thiessen]

FYI, this has been reverted in Blink for triggering ASSERTs

(https://codereview.chromium.org/14139021/)