Bug 15374 - RenderBox::availableHeightUsing() contains left/right to top/bottom typo
Summary: RenderBox::availableHeightUsing() contains left/right to top/bottom typo
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 312.x
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords: NeedsReduction
Depends on:
Blocks:
 
Reported: 2007-10-04 09:27 PDT by David Kilzer (:ddkilzer)
Modified: 2023-02-12 19:26 PST (History)
4 users (show)

See Also:


Attachments
Proposed fix (no changelog or test) (806 bytes, patch)
2007-10-04 09:28 PDT, David Kilzer (:ddkilzer)
aroben: review-
Details | Formatted Diff | Diff
Potential test case (494 bytes, text/html)
2009-05-25 11:44 PDT, David Kilzer (:ddkilzer)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2007-10-04 09:27:32 PDT
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
Comment 1 David Kilzer (:ddkilzer) 2007-10-04 09:28:39 PDT
Created attachment 16535 [details]
Proposed fix (no changelog or test)

Proposed fix without a changelog entry or a layout test.
Comment 2 Darin Adler 2007-10-05 14:43:40 PDT
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 3 Adam Roben (:aroben) 2007-10-06 23:07:27 PDT
Comment on attachment 16535 [details]
Proposed fix (no changelog or test)

r- until we have a test and ChangeLog.
Comment 4 David Kilzer (:ddkilzer) 2007-10-07 19:03:49 PDT
(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?

Comment 5 David Kilzer (:ddkilzer) 2007-10-07 21:45:55 PDT
(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?

Comment 6 David Kilzer (:ddkilzer) 2009-05-24 07:18:20 PDT
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.
Comment 7 David Kilzer (:ddkilzer) 2009-05-25 11:44:17 PDT
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].
Comment 8 David Kilzer (:ddkilzer) 2009-05-26 22:25:47 PDT
LayoutTests/tables/mozilla/bugs/bug131020.html is also affected (very slightly) by this change.
Comment 9 Ahmad Saleem 2023-02-02 07:25:18 PST
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!
Comment 10 Karl Dubost 2023-02-12 19:26:53 PST
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.