Bug 90097

Summary: Subpixel layout broken with spans with CSS position other than static
Product: WebKit Reporter: Behdad Esfahbod <behdad>
Component: Layout and RenderingAssignee: Behdad Esfahbod <behdad>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eae, eric.carlson, eric, feature-media-reviews, leviw, rbyers, rjkroege, webkit.review.bot
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 60318, 90281    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-04
none
LayoutTest
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-01
none
Patch
none
Patch
none
Archive of layout-test-results from gce-cr-linux-08
none
Patch none

Description Behdad Esfahbod 2012-06-27 13:26:50 PDT
We are facing an issue with Chrome Linux with --enable-text-subpixel-rendering where getBoundingClientRect() is returning rounded-to-integers bounds for spans with position:static but not any other position setting.  See:

  http://code.google.com/p/chromium/issues/detail?id=132571

Here is a minimal test case:

  http://jsfiddle.net/ALd5F/2/

Currently, this is sample output from Chrome Linux with --enable-text-subpixel-positioning:

T TTT
span[0]: rect.width=9.7734375
span[1]: rect.width=10
span[2]: rect.width=10
span[3]: rect.width=9

Firefox on the Mac does subpixel text positioning, and here's its output:

T T T T

span[0]: rect.width=9.76666259765625
span[1]: rect.width=9.766677856445312
span[2]: rect.width=9.766677856445312
span[3]: rect.width=9.766677856445312

The position:relative case goes through RenderInline to InlineBox, and looks like a thinko, and should be harmless to fix:

diff --git a/Source/WebCore/rendering/InlineBox.h b/Source/WebCore/rendering/InlineBox.h
index 2ed9cd4..84108db 100644
--- a/Source/WebCore/rendering/InlineBox.h
+++ b/Source/WebCore/rendering/InlineBox.h
@@ -200,7 +200,7 @@ public:
 
     float width() const { return isHorizontal() ? logicalWidth() : logicalHeight(); }
     float height() const { return isHorizontal() ? logicalHeight() : logicalWidth(); }
-    FloatSize size() const { return IntSize(width(), height()); }
+    FloatSize size() const { return FloatSize(width(), height()); }
     float right() const { return left() + width(); }
     float bottom() const { return top() + height(); }
 

The two remaining ones go through RenderBlock, eventually getting ceil'ed in RenderBlock::updatePreferredWidth():

static inline void updatePreferredWidth(LayoutUnit& preferredWidth, float& result)
{
    LayoutUnit snappedResult = ceilf(result);
    preferredWidth = max(snappedResult, preferredWidth);
}

This one is clearly more delicate.  We don't want rounding to full pixels.  But we want to make sure that the allocated with is at least as large as requested width.  So, when converting from float to LayoutUnits, we need to round up.

Attaching separate patches for the two issues.
Comment 1 Behdad Esfahbod 2012-06-27 13:34:08 PDT
Created attachment 149790 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-27 13:39:08 PDT
Attachment 149790 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:10:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Emil A Eklund 2012-06-27 13:50:43 PDT
The change itself looks good, apart form the style violations.
Could you add a test for it though please? One based on the minimal test case will do. I'll gladly lend a hand to convert it to a test if you'd like.
Comment 4 Behdad Esfahbod 2012-06-27 13:54:02 PDT
Thanks Emil.  Let me also upload the second patch, then talk about format and tests :).
Comment 5 Early Warning System Bot 2012-06-27 13:59:52 PDT
Comment on attachment 149790 [details]
Patch

Attachment 149790 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13090910
Comment 6 Behdad Esfahbod 2012-06-27 14:38:53 PDT
Created attachment 149798 [details]
Patch
Comment 7 Behdad Esfahbod 2012-06-27 14:39:42 PDT
Ok.  Uploaded second patch.  I'll update them tomorrow based on feedback and resubmit.  Thanks!
Comment 8 WebKit Review Bot 2012-06-27 14:42:27 PDT
Attachment 149798 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:11:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Build Bot 2012-06-27 15:05:10 PDT
Comment on attachment 149798 [details]
Patch

Attachment 149798 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13090923
Comment 10 Robert Kroeger 2012-06-27 15:11:01 PDT
Comment on attachment 149798 [details]
Patch

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

> Source/WebCore/platform/FractionalLayoutUnit.h:89
> +    static FractionalLayoutUnit fromFloatCeil(float value)

are there unit tests for FractionalLayoutUnit? If so, it would be nice to add one?
Comment 11 Emil A Eklund 2012-06-27 15:14:29 PDT
(In reply to comment #10)
> (From update of attachment 149798 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149798&action=review
> 
> > Source/WebCore/platform/FractionalLayoutUnit.h:89
> > +    static FractionalLayoutUnit fromFloatCeil(float value)
> 
> are there unit tests for FractionalLayoutUnit? If so, it would be nice to add one?

No, we don't really have unit tests in webkit. It's all LayoutTests.
Comment 12 WebKit Review Bot 2012-06-27 16:48:57 PDT
Comment on attachment 149798 [details]
Patch

Attachment 149798 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13090950

New failing tests:
fast/html/details-marker-style.html
svg/zoom/page/zoom-replaced-intrinsic-ratio-001.htm
media/video-zoom-controls.html
svg/zoom/page/zoom-img-preserveAspectRatio-support-1.html
svg/zoom/page/zoom-svg-through-object-with-absolute-size.xhtml
svg/zoom/page/zoom-svg-through-object-with-percentage-size.xhtml
Comment 13 WebKit Review Bot 2012-06-27 16:49:13 PDT
Created attachment 149821 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 14 Behdad Esfahbod 2012-06-29 10:13:52 PDT
Created attachment 150208 [details]
LayoutTest

I wrote a layout test for this.  Figuring out how to integrate it into the patch now.
Comment 15 Behdad Esfahbod 2012-06-29 11:39:48 PDT
Created attachment 150228 [details]
Patch
Comment 16 Build Bot 2012-06-29 12:27:23 PDT
Comment on attachment 150228 [details]
Patch

Attachment 150228 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13121371
Comment 17 WebKit Review Bot 2012-06-29 22:55:29 PDT
Comment on attachment 150228 [details]
Patch

Attachment 150228 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13118527

New failing tests:
fast/html/details-marker-style.html
svg/zoom/page/zoom-replaced-intrinsic-ratio-001.htm
media/video-zoom-controls.html
svg/zoom/page/zoom-svg-through-object-with-absolute-size.xhtml
svg/zoom/page/zoom-svg-through-object-with-percentage-size.xhtml
fast/sub-pixel/size-of-span-with-different-positions.html
Comment 18 WebKit Review Bot 2012-06-29 22:55:33 PDT
Created attachment 150307 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 19 Behdad Esfahbod 2012-07-03 05:38:52 PDT
Created attachment 150591 [details]
Patch
Comment 20 Behdad Esfahbod 2012-07-03 05:46:11 PDT
Created attachment 150592 [details]
Patch
Comment 21 Behdad Esfahbod 2012-07-03 05:55:54 PDT
Fixed the win-ews build issue, and the expected-output of the newly added tests.
Comment 22 WebKit Review Bot 2012-07-03 06:58:42 PDT
Comment on attachment 150592 [details]
Patch

Attachment 150592 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13129490

New failing tests:
svg/zoom/page/zoom-svg-through-object-with-percentage-size.xhtml
svg/zoom/page/zoom-replaced-intrinsic-ratio-001.htm
media/video-zoom-controls.html
fast/html/details-marker-style.html
svg/zoom/page/zoom-svg-through-object-with-absolute-size.xhtml
Comment 23 WebKit Review Bot 2012-07-03 06:58:46 PDT
Created attachment 150601 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 24 Behdad Esfahbod 2012-07-03 07:58:24 PDT
Created attachment 150608 [details]
Patch
Comment 25 Eric Seidel (no email) 2012-07-03 11:44:05 PDT
This looks right to me, but Emil is the best reviewer for this.  When he says LGTM, I'm happy to do the official flag marking. :)
Comment 26 Emil A Eklund 2012-07-04 12:11:06 PDT
Comment on attachment 150608 [details]
Patch

LGTM and I like the test, thank you.
Comment 27 Behdad Esfahbod 2012-07-05 07:51:52 PDT
Thanks Emil!  Eric, can you please press the magic button?
Comment 28 Eric Seidel (no email) 2012-07-05 10:28:43 PDT
Comment on attachment 150608 [details]
Patch

Shazam!
Comment 29 WebKit Review Bot 2012-07-05 11:21:01 PDT
Comment on attachment 150608 [details]
Patch

Clearing flags on attachment: 150608

Committed r121917: <http://trac.webkit.org/changeset/121917>
Comment 30 WebKit Review Bot 2012-07-05 11:21:07 PDT
All reviewed patches have been landed.  Closing bug.