Bug 107193 - Incorrect scrollable height during simplified layout
Summary: Incorrect scrollable height during simplified layout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-17 15:55 PST by Tony Chang
Modified: 2013-01-23 12:55 PST (History)
5 users (show)

See Also:

Attachments
Patch (4.60 KB, patch)
2013-01-17 16:31 PST, Tony Chang 		no flags Details | Formatted Diff | Diff
Patch (6.01 KB, patch)
2013-01-22 16:44 PST, Tony Chang 		no flags Details | Formatted Diff | Diff
Patch for landing (6.13 KB, patch)
2013-01-23 12:20 PST, Tony Chang 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.