Bug 14975 – Computed size for padding is incorrect

• Component: DOM
• Status: RESOLVED FIXED

[Erik Arvidsson]

When an element is not displayed (display:none) and no padding has been set the computed padding is returned as 'auto', which is not a valid value for padding.

[Erik Arvidsson]

Created attachment 15982 [details]
Testcase

Should say "0px"

[Eric Seidel (no email)]

This is probably just a couple line fix in CSSComputedStyleDeclaration.cpp

[Eric Seidel (no email)]

This will be simple to fix.  More interesting is what the correct behavior (as defined by what FF does) in the cases where it is display: none and padding is specified (either as px or as a %).  Also, what happens when a table cell is display: none yet the enclosing table has cellpadding specified?  We might have to break RenderObject::padding() out into a free function which an be passed a renderer and style (if available).

Also interesting to look at is other CSSComputedStyleDelcaration examples where if (renderer) is checked, do those cases return invalid css values when !renderer?  Again, the fix will be simple, we just need some better test cases.

[Eric Seidel (no email)]

If firefox just returns 0px for all test cases where the element is display: none than our job is really easy here. :)  we just change the !renderer case to return new CSSPrimitiveValue(0, CSSPrimitiveValue::CSS_PX);  I don't suspect it will be quite that easy.

[Dave Hyatt]

It is that easy.

[Eric Seidel (no email)]

Created attachment 18506 [details]
testcase (not standalone, place in fast/css to run)

My fears weren't entirely unfounded!

Here's FF's results from this test:

PASS window.getComputedStyle(test0, null).paddingTop is "0px"
FAIL window.getComputedStyle(test1, null).paddingTop should be 0px. Was 10px.
FAIL window.getComputedStyle(test2, null).paddingTop should be 0px. Was 10px.
FAIL window.getComputedStyle(test3, null).paddingTop should be 0px. Was 1px.

[Dave Hyatt]

I still think 0 is a perfectly acceptable result for now.  Auto isn't even a legal value for padding, so I think we make good progress if we just return 0.

Actually returning the real padding would involve resolving style, and we can't really do that until we have some kind of undisplayed style cache.

[Eric Seidel (no email)]

An enhanced test case, and the results from various browsers:
http://paste.lisp.org/display/54381

(In reply to comment #7)
> Actually returning the real padding would involve resolving style, and we can't
> really do that until we have some kind of undisplayed style cache.

We have that.

[Eric Seidel (no email)]

Created attachment 18507 [details]
better test case (place in fast/css to run)

Results from this test case:

Opera 9:
PASS window.getComputedStyle(test0, null).paddingTop is "0px"
PASS window.getComputedStyle(test1, null).paddingTop is "10px"
PASS window.getComputedStyle(test2, null).paddingTop is "10px"
PASS window.getComputedStyle(test3, null).paddingTop is "10px"
PASS window.getComputedStyle(test4, null).paddingTop is "10px"
PASS window.getComputedStyle(test5, null).paddingTop is "10px"
PASS window.getComputedStyle(test6, null).paddingTop is "10px"
PASS window.getComputedStyle(test7, null).paddingTop is "10px"
PASS successfullyParsed is true

TEST COMPLETE

FF3b2:
PASS window.getComputedStyle(test0, null).paddingTop is "0px"
PASS window.getComputedStyle(test1, null).paddingTop is "10px"
PASS window.getComputedStyle(test2, null).paddingTop is "10px"
PASS window.getComputedStyle(test3, null).paddingTop is "10px"
PASS window.getComputedStyle(test4, null).paddingTop is "10px"
PASS window.getComputedStyle(test5, null).paddingTop is "10px"
FAIL window.getComputedStyle(test6, null).paddingTop should be 10px. Was 0px.
FAIL window.getComputedStyle(test7, null).paddingTop should be 10px. Was 0px.
PASS successfullyParsed is true

TEST COMPLETE

Safari3:
FAIL window.getComputedStyle(test0, null).paddingTop should be 0px. Was auto.
PASS window.getComputedStyle(test1, null).paddingTop is "10px"
PASS window.getComputedStyle(test2, null).paddingTop is "10px"
FAIL window.getComputedStyle(test3, null).paddingTop should be 10px. Was auto.
PASS window.getComputedStyle(test4, null).paddingTop is "10px"
PASS window.getComputedStyle(test5, null).paddingTop is "10px"
FAIL window.getComputedStyle(test6, null).paddingTop should be 10px. Was 10%.
FAIL window.getComputedStyle(test7, null).paddingTop should be 10px. Was 10%.
PASS successfullyParsed is true

TEST COMPLETE

[Dave Hyatt]

Created attachment 18515 [details]
Here's the fix.  Not ready for review yet since I need to make tests and stuff.

[Dave Hyatt]

Created attachment 18530 [details]
Patch including layout tests and changelog

[Dave Hyatt]

Fixed in r29647.  Thanks for the test cases.