Bug 15374 – RenderBox::availableHeightUsing() contains left/right to top/bottom typo

• Component: Layout and Rendering
• Status: RESOLVED CONFIGURATION CHANGED

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

Created attachment 16535 [details]
Proposed fix (no changelog or test)

Proposed fix without a changelog entry or a layout test.

[Darin Adler]

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.

[Adam Roben (:aroben)]

Comment on attachment 16535 [details]
Proposed fix (no changelog or test)

r- until we have a test and ChangeLog.

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

LayoutTests/tables/mozilla/bugs/bug131020.html is also affected (very slightly) by this change.

[Ahmad Saleem]

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!

[Karl Dubost]

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.