[sub-pixel] Rounding error in table cell height calculation causes unnecessary scrollbar

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Emil A Eklund]

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.

[Emil A Eklund]

Downstream chromium bug: https://code.google.com/p/chromium/issues/detail?id=180747

[Emil A Eklund]

Created attachment 192105 [details]
Patch

[Levi Weintraub]

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.

[Emil A Eklund]

Created attachment 192107 [details]
Patch

[Levi Weintraub]

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?

[Levi Weintraub]

(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

[Emil A Eklund]

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

[Emil A Eklund]

Created attachment 192112 [details]
Patch

[Emil A Eklund]

Comment on attachment 192112 [details]
Patch

Thanks for the quick review!

[Emil A Eklund]

Committed r145242: <http://trac.webkit.org/changeset/145242>

[James Robinson]

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?

[Emil A Eklund]

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