Bug 107193

Summary: Incorrect scrollable height during simplified layout
Product: WebKit Reporter: Tony Chang <tony>
Component: New BugsAssignee: Tony Chang <tony>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, hyatt, ojan.autocc, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Tony Chang 2013-01-17 15:55:19 PST
Incorrect scrollable height during simplified layout
Comment 1 Tony Chang 2013-01-17 16:31:51 PST
Created attachment 183312 [details]
Patch
Comment 2 Tony Chang 2013-01-17 16:34:26 PST
I think this bug has been around since http://trac.webkit.org/changeset/73385 .
Comment 3 Ojan Vafai 2013-01-17 16:44:42 PST
Comment on attachment 183312 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:2599
> +    LayoutUnit logicalScrollHeight = style()->isHorizontalWritingMode() ? scrollHeight() : scrollWidth();

Should we add a helper function for this? I bet there are a bunch of places we do this already.

> LayoutTests/fast/overflow/height-during-simplified-layout.html:17
> +shouldBe("document.getElementById('scrollable').scrollTop", "400");
> +shouldBe("document.getElementById('scrollable').scrollHeight", "600");

should we extend check-layout.js to support these?
Comment 4 WebKit Review Bot 2013-01-18 10:03:35 PST
Comment on attachment 183312 [details]
Patch

Clearing flags on attachment: 183312

Committed r140171: <http://trac.webkit.org/changeset/140171>
Comment 5 WebKit Review Bot 2013-01-18 10:03:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Tony Chang 2013-01-22 11:05:10 PST
This caused the regression in bug 107572.
Comment 7 Tony Chang 2013-01-22 15:45:50 PST
Reverted r140171 for reason:

Regressed scrollable region size in other cases.

Committed r140482: <http://trac.webkit.org/changeset/140482>
Comment 8 Tony Chang 2013-01-22 15:48:40 PST
15:31 < dhyatt> i see
15:31 < dhyatt> the bug is it's using the clamped height
15:31 < dhyatt> computing of overflow needs to use the unclamped heigth
15:31 < dhyatt> height
15:32 < tony^work> right
15:32 < tony^work> but we don't have the unclamped height anymore
15:33 < dhyatt> could store it in the overflow
15:34 < dhyatt> so you could in theory put the intrinsic height into the 
                overflow structure
15:34 < dhyatt> computeOverflow could even do it
15:34 < dhyatt> since it got passed it


New patch with this approach coming soon.
Comment 9 Tony Chang 2013-01-22 16:44:46 PST
Created attachment 184075 [details]
Patch
Comment 10 Dave Hyatt 2013-01-23 12:13:03 PST
Comment on attachment 184075 [details]
Patch

r=me, although I'd comment the code a bit to make it clear what you're doing.
Comment 11 Tony Chang 2013-01-23 12:20:55 PST
Created attachment 184280 [details]
Patch for landing
Comment 12 WebKit Review Bot 2013-01-23 12:55:38 PST
Comment on attachment 184280 [details]
Patch for landing

Clearing flags on attachment: 184280

Committed r140576: <http://trac.webkit.org/changeset/140576>
Comment 13 WebKit Review Bot 2013-01-23 12:55:42 PST
All reviewed patches have been landed.  Closing bug.