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.
Created attachment 149790 [details] Patch
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.
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.
Thanks Emil. Let me also upload the second patch, then talk about format and tests :).
Comment on attachment 149790 [details] Patch Attachment 149790 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13090910
Created attachment 149798 [details] Patch
Ok. Uploaded second patch. I'll update them tomorrow based on feedback and resubmit. Thanks!
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 on attachment 149798 [details] Patch Attachment 149798 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13090923
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?
(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 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
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
Created attachment 150208 [details] LayoutTest I wrote a layout test for this. Figuring out how to integrate it into the patch now.
Created attachment 150228 [details] Patch
Comment on attachment 150228 [details] Patch Attachment 150228 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13121371
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
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
Created attachment 150591 [details] Patch
Created attachment 150592 [details] Patch
Fixed the win-ews build issue, and the expected-output of the newly added tests.
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
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
Created attachment 150608 [details] Patch
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 on attachment 150608 [details] Patch LGTM and I like the test, thank you.
Thanks Emil! Eric, can you please press the magic button?
Comment on attachment 150608 [details] Patch Shazam!
Comment on attachment 150608 [details] Patch Clearing flags on attachment: 150608 Committed r121917: <http://trac.webkit.org/changeset/121917>
All reviewed patches have been landed. Closing bug.