Bug 96031 – Fix RenderBox::availableHeight to subtract scrollbars in the right places

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

Fix RenderBox::availableHeight to subtract scrollbars in the right places

[Ojan Vafai]

Created attachment 162599 [details]
Patch

[Tony Chang]

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?

[Elliott Sprehn]

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. :)

[Ojan Vafai]

Created attachment 162828 [details]
Patch

[Ojan Vafai]

Created attachment 162830 [details]
Patch

[Ojan Vafai]

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.

[Ojan Vafai]

Committed r127915: <http://trac.webkit.org/changeset/127915>

[Roger Fong]

> 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.

[Ojan Vafai]

(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.

[Ojan Vafai]

Rebaselines landed in http://trac.webkit.org/changeset/127935