Bug 96031 - Fix RenderBox::availableHeight to subtract scrollbars in the right places
Summary: Fix RenderBox::availableHeight to subtract scrollbars in the right places
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: Ojan Vafai
URL:
Keywords:
Depends on:
Blocks: 93655
  Show dependency treegraph
 
Reported: 2012-09-06 15:09 PDT by Ojan Vafai
Modified: 2012-09-07 16:27 PDT (History)
9 users (show)

See Also:


Attachments
Patch (12.62 KB, patch)
2012-09-06 15:13 PDT, Ojan Vafai
no flags Details | Formatted Diff | Diff
Patch (16.71 KB, patch)
2012-09-07 11:26 PDT, Ojan Vafai
no flags Details | Formatted Diff | Diff
Patch (16.76 KB, patch)
2012-09-07 11:27 PDT, Ojan Vafai
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2012-09-06 15:09:32 PDT
Fix RenderBox::availableHeight to subtract scrollbars in the right places
Comment 1 Ojan Vafai 2012-09-06 15:13:47 PDT
Created attachment 162599 [details]
Patch
Comment 2 Tony Chang 2012-09-06 16:07:29 PDT
Comment on attachment 162599 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Tests: fast/block/positioning/percent-top-left-on-relative-position.html
> +               fast/css/nested-percent-height-on-replaced.html

Can you verify that Firefox and IE agree with this?  I think this change is correct, but it would be nice to corroborate.

> LayoutTests/ChangeLog:13
> +        * platform/chromium-linux/fast/css/percent-top-value-with-relative-position-expected.png:
> +        * platform/chromium-win/fast/css/percent-top-value-with-relative-position-expected.txt:

Can you explain why this changed in the ChangeLog?

Is there a Win result that will also need rebaseline?  Should we mark it as such in TestExpectations?
Comment 3 Elliott Sprehn 2012-09-06 16:17:52 PDT
Comment on attachment 162599 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:-2345
> -        // the percentage is calculated with respect to the height of the padding box of that element

Why is this explicit bug fix not needed anymore? The new code doesn't seem to check containingBlock()->availableLogicalHeight()

> Source/WebCore/rendering/RenderBox.cpp:2344
> +        return std::max(LayoutUnit(0), computeContentBoxLogicalHeight(heightIncludingScrollbar) - scrollbarLogicalHeight());

Would be nice to be consistent and use std::max<LayoutUnit>(...) or std::max(LayoutUnit(0). You've done it both ways in this patch.

> LayoutTests/platform/chromium-win/fast/css/percent-top-value-with-relative-position-expected.txt:7
> +layer at (8,300) size 784x20

That's cute, 8px is half the 16px scrollbar track size. :)
Comment 4 Ojan Vafai 2012-09-07 11:26:14 PDT
Created attachment 162828 [details]
Patch
Comment 5 Ojan Vafai 2012-09-07 11:27:53 PDT
Created attachment 162830 [details]
Patch
Comment 6 Ojan Vafai 2012-09-07 13:21:49 PDT
Comment on attachment 162599 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +               fast/css/nested-percent-height-on-replaced.html
> 
> Can you verify that Firefox and IE agree with this?  I think this change is correct, but it would be nice to corroborate.

Yup. Added to the ChangeLog.

>> Source/WebCore/rendering/RenderBox.cpp:-2345
>> -        // the percentage is calculated with respect to the height of the padding box of that element
> 
> Why is this explicit bug fix not needed anymore? The new code doesn't seem to check containingBlock()->availableLogicalHeight()

This comment is confusing. The fix for the bug was adding the containingBlockLogicalHeightForPositioned call.

>> Source/WebCore/rendering/RenderBox.cpp:2344
>> +        return std::max(LayoutUnit(0), computeContentBoxLogicalHeight(heightIncludingScrollbar) - scrollbarLogicalHeight());
> 
> Would be nice to be consistent and use std::max<LayoutUnit>(...) or std::max(LayoutUnit(0). You've done it both ways in this patch.

Lol. I copy-pasted from two different places. :) Will fix both to std::max<LayoutUnit>(...).

>> LayoutTests/ChangeLog:13
>> +        * platform/chromium-win/fast/css/percent-top-value-with-relative-position-expected.txt:
> 
> Can you explain why this changed in the ChangeLog?
> 
> Is there a Win result that will also need rebaseline?  Should we mark it as such in TestExpectations?

Good point.
Comment 7 Ojan Vafai 2012-09-07 13:54:29 PDT
Committed r127915: <http://trac.webkit.org/changeset/127915>
Comment 8 Roger Fong 2012-09-07 16:10:53 PDT
> Can you explain why this changed in the ChangeLog?
> 
> Is there a Win result that will also need rebaseline?  Should we mark it as such in TestExpectations?

Also curious as to why there is a difference between Mac and Win.
Comment 9 Ojan Vafai 2012-09-07 16:21:34 PDT
(In reply to comment #8)
> > Can you explain why this changed in the ChangeLog?
> > 
> > Is there a Win result that will also need rebaseline?  Should we mark it as such in TestExpectations?
> 
> Also curious as to why there is a difference between Mac and Win.

Platform text layout differences.
Comment 10 Ojan Vafai 2012-09-07 16:27:07 PDT
Rebaselines landed in http://trac.webkit.org/changeset/127935