Bug 33593 – getComputedStyle gives incorrect information for 'height' property

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Allan Jardine]

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.

[Julien Chaffraix]

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

[Alexis Menard (darktears)]

Created attachment 129520 [details]
Possible patch.

[Alexis Menard (darktears)]

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

[WebKit Review Bot]

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

[Alexis Menard (darktears)]

Created attachment 129695 [details]
Patch

[Julien Chaffraix]

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.

[Alexis Menard (darktears)]

Created attachment 129719 [details]
Patch

[Alexis Menard (darktears)]

Comment on attachment 129719 [details]
Patch

Adding review flag for some people to look at it...maybe...

[Eric Seidel (no email)]

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 }

[Alexis Menard (darktears)]

Created attachment 130149 [details]
Patch

[WebKit Review Bot]

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.

[Dave Hyatt]

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.

[Dave Hyatt]

Comment on attachment 130149 [details]
Patch

Actually meh I changed my mind. This is probably fine.

[Dave Hyatt]

Please fix the style failures though before landing.

[Alexis Menard (darktears)]

Created attachment 130193 [details]
Patch landed 2012-03-05

[WebKit Review Bot]

Comment on attachment 130193 [details]
Patch landed 2012-03-05

Clearing flags on attachment: 130193

Committed r109818: <http://trac.webkit.org/changeset/109818>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dave Barton]

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.

[Dave Barton]

Reopening to attach new patch.

[Dave Barton]

Created attachment 134936 [details]
Patch

[Dave Barton]

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!

[Daniel Bates]

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

[Dave Barton]

I've opened bug 83092 for the further intrinsic padding & content box issues.

Thanks to everyone for their help.