WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
33593
getComputedStyle gives incorrect information for 'height' property
https://bugs.webkit.org/show_bug.cgi?id=33593
Summary
getComputedStyle gives incorrect information for 'height' property
Allan Jardine
Reported
2010-01-13 05:22:46 PST
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.
Attachments
Page showing the issue
(1.86 KB, text/html)
2010-01-13 05:22 PST
,
Allan Jardine
no flags
Details
Possible patch.
(1.59 KB, patch)
2012-02-29 14:34 PST
,
Alexis Menard (darktears)
no flags
Details
Formatted Diff
Diff
Patch
(6.25 KB, patch)
2012-03-01 06:17 PST
,
Alexis Menard (darktears)
no flags
Details
Formatted Diff
Diff
Patch
(9.08 KB, patch)
2012-03-01 09:57 PST
,
Alexis Menard (darktears)
no flags
Details
Formatted Diff
Diff
Patch
(22.73 KB, patch)
2012-03-05 09:07 PST
,
Alexis Menard (darktears)
no flags
Details
Formatted Diff
Diff
Patch landed 2012-03-05
(22.37 KB, patch)
2012-03-05 13:26 PST
,
Alexis Menard (darktears)
no flags
Details
Formatted Diff
Diff
Patch
(16.48 KB, patch)
2012-03-30 20:38 PDT
,
Dave Barton
dbates
: review-
dbates
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Julien Chaffraix
Comment 1
2012-02-28 16:42:25 PST
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)
Comment 2
2012-02-29 14:34:32 PST
Created
attachment 129520
[details]
Possible patch.
Alexis Menard (darktears)
Comment 3
2012-02-29 14:38:55 PST
(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 4
2012-02-29 19:52:03 PST
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)
Comment 5
2012-03-01 06:17:54 PST
Created
attachment 129695
[details]
Patch
Julien Chaffraix
Comment 6
2012-03-01 09:49:54 PST
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)
Comment 7
2012-03-01 09:57:50 PST
Created
attachment 129719
[details]
Patch
Alexis Menard (darktears)
Comment 8
2012-03-01 14:50:18 PST
Comment on
attachment 129719
[details]
Patch Adding review flag for some people to look at it...maybe...
Eric Seidel (no email)
Comment 9
2012-03-01 14:58:59 PST
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)
Comment 10
2012-03-05 09:07:30 PST
Created
attachment 130149
[details]
Patch
WebKit Review Bot
Comment 11
2012-03-05 09:10:55 PST
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 12
2012-03-05 11:40:20 PST
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 13
2012-03-05 11:43:51 PST
Comment on
attachment 130149
[details]
Patch Actually meh I changed my mind. This is probably fine.
Dave Hyatt
Comment 14
2012-03-05 11:44:11 PST
Please fix the style failures though before landing.
Alexis Menard (darktears)
Comment 15
2012-03-05 13:26:17 PST
Created
attachment 130193
[details]
Patch landed 2012-03-05
WebKit Review Bot
Comment 16
2012-03-05 16:51:53 PST
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
Comment 17
2012-03-05 16:52:02 PST
All reviewed patches have been landed. Closing bug.
Dave Barton
Comment 18
2012-03-30 14:01:43 PDT
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
Comment 19
2012-03-30 20:38:35 PDT
Reopening to attach new patch.
Dave Barton
Comment 20
2012-03-30 20:38:38 PDT
Created
attachment 134936
[details]
Patch
Dave Barton
Comment 21
2012-03-30 20:43:11 PDT
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
Comment 22
2012-04-01 15:32:48 PDT
(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
Comment 23
2012-04-03 16:30:24 PDT
I've opened
bug 83092
for the further intrinsic padding & content box issues. Thanks to everyone for their help.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug