Bug 90097 – Subpixel layout broken with spans with CSS position other than static

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Behdad Esfahbod]

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.

[Behdad Esfahbod]

Created attachment 149790 [details]
Patch

[WebKit Review Bot]

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.

[Emil A Eklund]

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.

[Behdad Esfahbod]

Thanks Emil.  Let me also upload the second patch, then talk about format and tests :).

[Early Warning System Bot]

Comment on attachment 149790 [details]
Patch

Attachment 149790 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13090910

[Behdad Esfahbod]

Created attachment 149798 [details]
Patch

[Behdad Esfahbod]

Ok.  Uploaded second patch.  I'll update them tomorrow based on feedback and resubmit.  Thanks!

[WebKit Review Bot]

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.

[Build Bot]

Comment on attachment 149798 [details]
Patch

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

[Robert Kroeger]

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?

[Emil A Eklund]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Behdad Esfahbod]

Created attachment 150208 [details]
LayoutTest

I wrote a layout test for this.  Figuring out how to integrate it into the patch now.

[Behdad Esfahbod]

Created attachment 150228 [details]
Patch

[Build Bot]

Comment on attachment 150228 [details]
Patch

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Behdad Esfahbod]

Created attachment 150591 [details]
Patch

[Behdad Esfahbod]

Created attachment 150592 [details]
Patch

[Behdad Esfahbod]

Fixed the win-ews build issue, and the expected-output of the newly added tests.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Behdad Esfahbod]

Created attachment 150608 [details]
Patch

[Eric Seidel (no email)]

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. :)

[Emil A Eklund]

Comment on attachment 150608 [details]
Patch

LGTM and I like the test, thank you.

[Behdad Esfahbod]

Thanks Emil!  Eric, can you please press the magic button?

[Eric Seidel (no email)]

Comment on attachment 150608 [details]
Patch

Shazam!

[WebKit Review Bot]

Comment on attachment 150608 [details]
Patch

Clearing flags on attachment: 150608

Committed r121917: <http://trac.webkit.org/changeset/121917>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.