Whilst investigating a fix for Bug 15359, I noticed that RenderBox::availableHeightUsing() seems to be using the wrong margin/padding attributes for calculating height: return overrideSize() - (borderLeft() + borderRight() + paddingLeft() + paddingRight()); I believe this line of code should be: return overrideSize() - (borderTop() + borderBottom() + paddingTop() + paddingBottom()); After making this change, though, no layout tests failed (as of r25955), so this code is not being adequately tested. If this is the correct fix, a layout test should be written to tickle this code. Note that this code was originally committed in r3351 (search for "artificially"): http://trac.webkit.org/projects/webkit/changeset/3351#file17
Created attachment 16535 [details] Proposed fix (no changelog or test) Proposed fix without a changelog entry or a layout test.
Comment on attachment 16535 [details] Proposed fix (no changelog or test) Lets construct a test -- we can start by giving a giant borderLeft to a table cell to trigger this code, and then figure out something affected by the result of availableHeightUsing.
Comment on attachment 16535 [details] Proposed fix (no changelog or test) r- until we have a test and ChangeLog.
(In reply to comment #2) > (From update of attachment 16535 [details] [edit]) > Lets construct a test -- we can start by giving a giant borderLeft to a table > cell to trigger this code, and then figure out something affected by the result > of availableHeightUsing. The problem is that I need to construct an HTML table cell with an internal overrideSize set to return a large positive integer so that this calculation in RenderBox::availableHeightUsing(const Length&) doesn't return a large negative integer: if (isTableCell() && (h.isAuto() || h.isPercent())) return overrideSize() - (borderLeft() + borderRight() + paddingLeft() + paddingRight()); Otherwise, setting a border or a padding to a large positive integer (like 1000px) results in a large negative integer being returned from RenderBox::availableHeightUsing(), which is returned from RenderBox::availableHeight(), which is returned from RenderBox::calcReplacedHeightUsing(), which then is "lost" in the return statement for RenderBox::calcReplacedHeight() since it's so small (much less than zero). Any idea how to construct a table cell with a large internal overrideSize?
(In reply to comment #4) > Any idea how to construct a table cell with a large internal overrideSize? A <select> list sometimes sets an internal overrideSize, but I can't figure out how to make it both a replaced element and not-inline. Are there other elements that set overrideSize internally?
Interestingly, this line of code is (a) still not fixed and (b) is apparently exercised by the layout tests per Line 1640 in this code coverage analysis: <http://nerget.com/Coverage/Coverage-23-May-2009/__WebCore__rendering__RenderBox.cpp.html> Perhaps the incorrect calculation normally doesn't matter since the top/bottom border and padding match the left/right values by default.
Created attachment 30656 [details] Potential test case This is a potential test case, but I think something else is going on besides the single line patch in Attachment #16535 [details].
LayoutTests/tables/mozilla/bugs/bug131020.html is also affected (very slightly) by this change.
All three browsers (Safari 16.3, Chrome Canary 112 and Firefox Nightly 111) differ in the attached test case. Just wanted to share updated testing result and also change status from “Assigned” to “New”. Thanks!
While all the browsers behave differently with regards to this test. The piece of code has been changed since the patch. // We need to stop here, since we don't want to increase the height of the table // artificially. We're going to rely on this cell getting expanded to some new // height, and then when we lay out again we'll use the calculation below. if (isTableCell() && (h.isAuto() || h.isPercentOrCalculated())) { if (hasOverridingLogicalHeight()) return overridingLogicalHeight() - computedCSSPaddingBefore() - computedCSSPaddingAfter() - borderBefore() - borderAfter(); return logicalHeight() - borderAndPaddingLogicalHeight(); } It doesn't refer anymore to Left and Right, but to Before and After which is I think correct.