Created attachment 46450 [details] Page showing the issue When using getComputedStyle on a table cell to obtain the 'height' property, Webkit appears to be giving the incorrect value (possibly returning the line-height instead?). The attached example (and live example here: http://sprymedia.co.uk/media/misc/webkit/table_height.html ) shows that the computed height is returns as 18px, even although I've specifically given the cell a height of 200px in the CSS. I would have expected that the 200px would be returned. Behaviour of other browsers: 1. Firefox 3.5: gives a height of 140px. This was why this test was but together in the first place, it appears to be using a "border-box" box model - for some reason. 2. Opera 10.10: gives a height of 140px as well Tested on shipping Safari 4.0.2, Webkit r53178 and Chrome (Mac) 4.0.249.49 (35163) beta.
As discussed with Alexis, it looks like this is a table cell issue: CSSComputedStyle calls internally RenderBox::contentBoxRect() that doesn't include the cell's intrinsic padding as it should. Allan, a work-around is to set box-sizing: border-box; (or anything but 'content-box') on your cell. That should give you the right value (or at least something closer to what you want).
Created attachment 129520 [details] Possible patch.
(In reply to comment #2) > Created an attachment (id=129520) [details] > Possible patch. The proposed patch triggers regression in some layout tests for example : vertical-align-baseline.html where one cell is not correctly rendered. <td style="padding-top: 15px; border-top: 10px solid blue; border-bottom: 5px solid blue; vertical-align: bottom;"><div style="background-color: lightpink;">Lorem</div></td> I'm wondering if my patch is the correct approach or I should do something special in CSSComputedStyleDeclaration to handle the special case of cells. A bit like padding top is doing (by passing false to not take into account the intrinsic padding). Any pointer would be appreciated here.
Comment on attachment 129520 [details] Possible patch. Attachment 129520 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11766155 New failing tests: fast/table/vertical-align-baseline.html editing/deleting/5433862-1.html fast/table/table-insert-before-non-anonymous-block.html fast/table/border-collapsing/003.html tables/mozilla/bugs/bug3037-1.html editing/deleting/5206311-2.html editing/deleting/5206311-1.html fast/table/table-in-table-percent-width.html editing/deleting/5433862-2.html fast/table/table-in-table-percent-width-quirks-mode.html
Created attachment 129695 [details] Patch
Comment on attachment 129695 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129695&action=review > Source/WebCore/rendering/RenderBox.h:134 > + LayoutRect contentBoxRect(bool includeIntrinsicPadding = false) const { return LayoutRect(borderLeft() + paddingLeft(includeIntrinsicPadding), borderTop() + paddingTop(includeIntrinsicPadding), contentWidth(includeIntrinsicPadding), contentHeight(includeIntrinsicPadding)); } I really don't think the booleans are the way to go. Especially since here actually they mean to *exclude* the intrinsic padding from the contentBoxRect... We would need a new function if we really go down that path to avoid such errors. > Source/WebCore/rendering/RenderBox.h:182 > + LayoutUnit contentWidth(bool includeIntrinsicPadding = false) const { return clientWidth() - paddingLeft(includeIntrinsicPadding) - paddingRight(includeIntrinsicPadding); } > + LayoutUnit contentHeight(bool includeIntrinsicPadding = false) const { return clientHeight() - paddingTop(includeIntrinsicPadding) - paddingBottom(includeIntrinsicPadding); } IMHO the real bug is those 2 functions: we shouldn't remove the intrinsic padding AFAICT but dhyatt should comment on that further as he wrote the original intrinsic padding logic.
Created attachment 129719 [details] Patch
Comment on attachment 129719 [details] Patch Adding review flag for some people to look at it...maybe...
Comment on attachment 129719 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129719&action=review > Source/WebCore/editing/DeleteSelectionCommand.cpp:366 > + if (r && r->isTableCell() && toRenderTableCell(r)->contentHeight(true) <= 0) { Enums are alwasy more clear than booleans. > Source/WebCore/rendering/RenderBox.h:181 > + LayoutUnit contentWidth(bool includeIntrinsicPadding = false) const { return clientWidth() - paddingLeft(includeIntrinsicPadding) - paddingRight(includeIntrinsicPadding); } Please make this an Enum if you're going to go this route. enum PaddingOptions { IncludeIntrinsicPadding, ExcludeIntrinsicPadding }
Created attachment 130149 [details] Patch
Attachment 130149 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/rendering/RenderBoxModelObject.h:83: The parameter name "paddingOption" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderBoxModelObject.h:84: The parameter name "paddingOption" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderBoxModelObject.h:85: The parameter name "paddingOption" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderBoxModelObject.h:86: The parameter name "paddingOption" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderBoxModelObject.h:87: The parameter name "paddingOption" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderBoxModelObject.h:88: The parameter name "paddingOption" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderBoxModelObject.h:89: The parameter name "paddingOption" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderBoxModelObject.h:90: The parameter name "paddingOption" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderTableCell.h:123: The parameter name "paddingOption" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderTableCell.h:124: The parameter name "paddingOption" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderTableCell.h:125: The parameter name "paddingOption" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderTableCell.h:126: The parameter name "paddingOption" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderTableCell.h:131: The parameter name "paddingOption" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderTableCell.h:132: The parameter name "paddingOption" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 14 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 130149 [details] Patch This is way too intrusive. Just patch CSSComputedStyleDeclaration to do the right thing if renderer()->isTableCell() and don't complicate the interfaces to all of these functions that are widely used by all the other renderers.
Comment on attachment 130149 [details] Patch Actually meh I changed my mind. This is probably fine.
Please fix the style failures though before landing.
Created attachment 130193 [details] Patch landed 2012-03-05
Comment on attachment 130193 [details] Patch landed 2012-03-05 Clearing flags on attachment: 130193 Committed r109818: <http://trac.webkit.org/changeset/109818>
All reviewed patches have been landed. Closing bug.
I am exploring using intrinsic padding for MathML, and this patch is causing me problems. I think the 5 uses of ExcludeIntrinsicPadding in RenderBox.h are wrong. For example: LayoutUnit contentWidth(PaddingOptions paddingOption = ExcludeIntrinsicPadding) const { return clientWidth() - paddingLeft(paddingOption) - paddingRight(paddingOption); } Note that ExcludeIntrinsicPadding here causes the intrinsic padding to *not* be subtracted out (as Julien pointed out in comment #6), so it *is* counted as part of the content width, thus changing the previous behavior of contentWidth() (i.e., when called with no arguments). When I add intrinsic left padding to a MathML element, this confuses { text-align: center }, for instance, breaking my MathML layout. Perhaps the enum values should be changed to { PaddingIncludesIntrinsic, PaddingExcludesIntrinsic }. Then the 5 occurrences of ExcludeIntrinsicPadding as a default argument in RenderBox.h could be changed to PaddingIncludesIntrinsic. I could do this here or in a new bug, but I'd rather that someone involved in table rendering do it, so they can consider & revise the rest of this patch as well.
Reopening to attach new patch.
Created attachment 134936 [details] Patch
I went ahead and created a new patch, since I need this for MathML. I am fairly new, so please let me know if this corrected patch would be better in a new bug report, or if I should mark the previous patch as still not obsolete, since it was landed. Thanks!
(In reply to comment #21) > I went ahead and created a new patch, since I need this for MathML. I am fairly new, so please let me know if this corrected patch would be better in a new bug report, or if I should mark the previous patch as still not obsolete, since it was landed. Thanks! Please file a new bug report with the patch. You may find it beneficial to read the email <https://lists.webkit.org/pipermail/webkit-dev/2011-June/017169.html>, which discusses the strategy and motivation for filing a new bug report instead of reopening- or posting patches on- the original bug report.
I've opened bug 83092 for the further intrinsic padding & content box issues. Thanks to everyone for their help.