Bug 111794 - [sub-pixel] Rounding error in table cell height calculation causes unnecessary scrollbar
Summary: [sub-pixel] Rounding error in table cell height calculation causes unnecessar...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Emil A Eklund
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-07 16:43 PST by Emil A Eklund
Modified: 2013-03-08 11:13 PST (History)
6 users (show)

See Also:


Attachments
Patch (5.18 KB, patch)
2013-03-07 16:46 PST, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (5.86 KB, patch)
2013-03-07 16:55 PST, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (5.86 KB, patch)
2013-03-07 17:02 PST, Emil A Eklund
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emil A Eklund 2013-03-07 16:43:47 PST
In RenderTableCell::logicalHeightForRowSizing the adjustedLogicalHeight is calculated from the logicalHeight and intrinsic padding and is then returned and floored. This can cause cause the cell to be slightly smaller (0.5px) than the the element it contains.
Comment 1 Emil A Eklund 2013-03-07 16:46:03 PST
Downstream chromium bug: https://code.google.com/p/chromium/issues/detail?id=180747
Comment 2 Emil A Eklund 2013-03-07 16:46:35 PST
Created attachment 192105 [details]
Patch
Comment 3 Levi Weintraub 2013-03-07 16:52:30 PST
Comment on attachment 192105 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192105&action=review

> Source/WebCore/rendering/RenderTableCell.h:93
>      LayoutUnit logicalHeightForRowSizing() const

Looks like this should be an integer, and that would make using pixelSnappedLogicalHeight make more sense.
Comment 4 Emil A Eklund 2013-03-07 16:55:03 PST
Created attachment 192107 [details]
Patch
Comment 5 Levi Weintraub 2013-03-07 16:56:18 PST
Comment on attachment 192107 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192107&action=review

> Source/WebCore/rendering/RenderTableCell.h:101
> -            styleLogicalHeight += computedCSSPaddingBefore() + computedCSSPaddingAfter() + borderBefore() + borderAfter();
> +            styleLogicalHeight += (computedCSSPaddingBefore() + computedCSSPaddingAfter() + borderBefore() + borderAfter()).floor();

wouldn't (computedCSSPaddingBefore() + computedCSSPaddingAfter().floor() + borderBefore() + borderAfter() be more efficient?
Comment 6 Levi Weintraub 2013-03-07 16:56:43 PST
(In reply to comment #5)
> (From update of attachment 192107 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192107&action=review
> 
> > Source/WebCore/rendering/RenderTableCell.h:101
> > -            styleLogicalHeight += computedCSSPaddingBefore() + computedCSSPaddingAfter() + borderBefore() + borderAfter();
> > +            styleLogicalHeight += (computedCSSPaddingBefore() + computedCSSPaddingAfter() + borderBefore() + borderAfter()).floor();
> 
> wouldn't (computedCSSPaddingBefore() + computedCSSPaddingAfter().floor() + borderBefore() + borderAfter() be more efficient?

(computedCSSPaddingBefore() + computedCSSPaddingAfter()).floor() rather
Comment 7 Emil A Eklund 2013-03-07 16:57:03 PST
(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 192107 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=192107&action=review
> > 
> > > Source/WebCore/rendering/RenderTableCell.h:101
> > > -            styleLogicalHeight += computedCSSPaddingBefore() + computedCSSPaddingAfter() + borderBefore() + borderAfter();
> > > +            styleLogicalHeight += (computedCSSPaddingBefore() + computedCSSPaddingAfter() + borderBefore() + borderAfter()).floor();
> > 
> > wouldn't (computedCSSPaddingBefore() + computedCSSPaddingAfter().floor() + borderBefore() + borderAfter() be more efficient?
> 
> (computedCSSPaddingBefore() + computedCSSPaddingAfter()).floor() rather

Good idea.
Comment 8 Emil A Eklund 2013-03-07 17:02:36 PST
Created attachment 192112 [details]
Patch
Comment 9 Emil A Eklund 2013-03-07 17:04:14 PST
Comment on attachment 192112 [details]
Patch

Thanks for the quick review!
Comment 10 Emil A Eklund 2013-03-08 10:32:50 PST
Committed r145242: <http://trac.webkit.org/changeset/145242>
Comment 11 James Robinson 2013-03-08 11:12:29 PST
This caused subtle changes in lots of tests of the form:

--- /Volumes/data/b/build/slave/WebKit_Mac10_8/build/layout-test-results/css1/basic/inheritance-expected.txt
+++ /Volumes/data/b/build/slave/WebKit_Mac10_8/build/layout-test-results/css1/basic/inheritance-actual.txt
@@ -1,8 +1,8 @@
-layer at (0,0) size 785x730
+layer at (0,0) size 785x731
   RenderView at (0,0) size 785x600
-layer at (0,0) size 785x730
-  RenderBlock {HTML} at (0,0) size 785x730
-    RenderBody {BODY} at (8,8) size 769x714 [color=#008000] [bgcolor=#CCCCCC]
+layer at (0,0) size 785x731
+  RenderBlock {HTML} at (0,0) size 785x731
+    RenderBody {BODY} at (8,8) size 769x715 [color=#008000] [bgcolor=#CCCCCC]

or very small changes in the scrollbar's height.  It looks like these are expected for this sort of change.  Shall I just rebaseline?
Comment 12 Emil A Eklund 2013-03-08 11:13:37 PST
(In reply to comment #11)
> or very small changes in the scrollbar's height.  It looks like these are expected for this sort of change.  Shall I just rebaseline?

All expected. I included new linux baselines in the patch and will rebaseline for other ports once the bots cycle.