Bug 96031

Created attachment 162599 [details] Patch

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

Created attachment 162828 [details] Patch

Created attachment 162830 [details] Patch

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.

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

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

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

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