Bug 14975

Summary: Computed size for padding is incorrect
Product: WebKit Reporter: Erik Arvidsson <erik.arvidsson>
Component: DOMAssignee: Dave Hyatt <hyatt>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, mitz, sam
Priority: P2 Keywords: EasyFix, HasReduction
Version: 523.x (Safari 3)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Testcase
none
testcase (not standalone, place in fast/css to run)
none
better test case (place in fast/css to run)
none
Here's the fix. Not ready for review yet since I need to make tests and stuff.
none
Patch including layout tests and changelog koivisto: review+

Erik Arvidsson
Reported 2007-08-15 11:11:08 PDT
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.
Attachments
Testcase (261 bytes, text/html)
2007-08-15 11:12 PDT, Erik Arvidsson
no flags
testcase (not standalone, place in fast/css to run) (1.42 KB, text/html)
2008-01-17 12:06 PST, Eric Seidel (no email)
no flags
better test case (place in fast/css to run) (2.37 KB, text/html)
2008-01-17 12:51 PST, Eric Seidel (no email)
no flags
Here's the fix. Not ready for review yet since I need to make tests and stuff. (18.28 KB, patch)
2008-01-17 15:46 PST, Dave Hyatt
no flags
Patch including layout tests and changelog (368.04 KB, patch)
2008-01-18 13:04 PST, Dave Hyatt
koivisto: review+
Erik Arvidsson
Comment 1 2007-08-15 11:12:26 PDT
Created attachment 15982 [details] Testcase Should say "0px"
Eric Seidel (no email)
Comment 2 2008-01-11 15:36:00 PST
This is probably just a couple line fix in CSSComputedStyleDeclaration.cpp
Eric Seidel (no email)
Comment 3 2008-01-17 03:13:33 PST
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)
Comment 4 2008-01-17 03:18:55 PST
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
Comment 5 2008-01-17 10:44:30 PST
It is that easy.
Eric Seidel (no email)
Comment 6 2008-01-17 12:06:53 PST
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
Comment 7 2008-01-17 12:21:18 PST
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)
Comment 8 2008-01-17 12:27:46 PST
An enhanced test case, and the results from various browsers: http://paste.lisp.org/display/54381
mitz
Comment 9 2008-01-17 12:42:40 PST
(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)
Comment 10 2008-01-17 12:51:17 PST
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
Comment 11 2008-01-17 15:46:09 PST
Created attachment 18515 [details] Here's the fix. Not ready for review yet since I need to make tests and stuff.
Dave Hyatt
Comment 12 2008-01-18 13:04:13 PST
Created attachment 18530 [details] Patch including layout tests and changelog
Dave Hyatt
Comment 13 2008-01-18 13:58:21 PST
Fixed in r29647. Thanks for the test cases.
Note You need to log in before you can comment on or make changes to this bug.