Summary: | getComputedStyle gives incorrect information for 'height' property | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Allan Jardine <allan.jardine> | ||||||||||||||||
Component: | Layout and Rendering | Assignee: | Alexis Menard (darktears) <menard> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | bdakin, dbarton, dbates, dglazkov, eric, hyatt, jchaffraix, koivisto, macpherson, menard, mitz, webkit.review.bot | ||||||||||||||||
Priority: | P2 | Keywords: | HasReduction | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Attachments: |
|
Description
Allan Jardine
2010-01-13 05:22:46 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). 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. |