98062 2012-10-01 13:43:44 -0700 [chromium] Subpixel text positioning causes incorrect inline-block layout. 2012-11-08 10:01:46 -0800 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 101497 1 flackr tdanderson eae leviw rbyers tdanderson vollick oldest_to_newest 731938 0 flackr 2012-10-01 13:43:44 -0700 When the attached page is viewed with subpixel text positioning enabled it causes the page to have a horizontal scrollbar and a scrollWidth 1 greater than the offsetWidth even though all the content fits on the page. Interestingly the span's scrollWidth is also 1 greater than its offsetWidth. 731940 1 leviw 2012-10-01 13:45:00 -0700 Super interesting. Can you give attaching the test page another try? 731943 2 166534 flackr 2012-10-01 13:50:22 -0700 Created attachment 166534 Test case showing the subpixel text positioning size problem 731954 3 leviw 2012-10-01 13:56:43 -0700 What version of Chrome are you repro-ing with? Emil and I tried to repro with 22, 23, and 24 on Windows, Mac, and Linux to no avail. 731955 4 leviw 2012-10-01 13:57:18 -0700 Sorry, I missed the "subpixel text positioning" part. Sorry! 733721 5 vollick 2012-10-03 08:22:10 -0700 This bug is possibly related to http://codereview.chromium.org/10996037/. We were doing some dodgy RectF to Rect conversions in chrome -- we were just flooring the position and size when we should have been taking either the enclosing or enclosed rect -- so I got rid of these conversions. There are a couple of spots where I'd chosen ToEnclosingRect rather than the old conversion, and this might have been the wrong choice and may be causing some rects to be one pixel larger than they ought to be. 752452 6 171021 tdanderson 2012-10-26 14:56:28 -0700 Created attachment 171021 Patch 752455 7 tdanderson 2012-10-26 14:57:13 -0700 (In reply to comment #6) > Created an attachment (id=171021) [details] > Patch This issue only reproduces in some cases. In the attached test case, if you replace the span with <span>x</span> the problem does not reproduce, but with <span>xx</span> it does. The horizontal scrollbar appears because in ScrollView::updateScrollbars(), the contents width is reported as being 1 larger than the visible width. I looked at the value of layoutOverflowRect().width() in RenderView::unscaledDocumentRect() after changing the text of the test case. Here is some data: (text on test page, layoutOverflowRect().width(), horizontal scrollbar present?) 'x', 575.296875, no 'xx', 575.609375, yes 'xxx', 575.906250, yes 'xxxx', 575.218750, no 'Hello, world!', 575.765625, yes This seemed to indicate that rounding (rather than always taking the floor) may be responsible for the one pixel difference. This patch indeed fixes the bug but is likely not a landable solution. An alternate solution could be to ensure that overflow is not falsely reported (perhaps using knownToHaveNoOverflow() ?). If |m_overflow| in RenderBox is never set, then layoutOverflowRect().width() will always be 575.000000. 753005 8 tdanderson 2012-10-28 16:35:08 -0700 Levi or Emil, are you able to provide any guidance about the best way to proceed here? 754189 9 leviw 2012-10-30 04:03:49 -0700 (In reply to comment #7) > I looked at the value of layoutOverflowRect().width() in RenderView::unscaledDocumentRect() after changing the text of the test case. Here is some data: > > (text on test page, layoutOverflowRect().width(), horizontal scrollbar present?) > 'x', 575.296875, no > 'xx', 575.609375, yes > 'xxx', 575.906250, yes > 'xxxx', 575.218750, no > 'Hello, world!', 575.765625, yes > > This seemed to indicate that rounding (rather than always taking the floor) may be responsible for the one pixel difference. This patch indeed fixes the bug but is likely not a landable solution. > > An alternate solution could be to ensure that overflow is not falsely reported (perhaps using knownToHaveNoOverflow() ?). If |m_overflow| in RenderBox is never set, then layoutOverflowRect().width() will always be 575.000000. Rounding is, of course, necessary for sub-pixel layout to work properly. Are you saying that we're only setting m_overflow in the fail case? We should be treating all values as LayoutUnits when determining if we overflow. What's the delta between the client rect and the value we're using to determine that we have overflow when it occurs? If you turn off sub-pixel layout (in Platform.h, not FractionalLayoutUnit.h), does the problem go away? Do we not set m_overflow in that case? This is a step towards the problem, but it's not quite enough to understand the root cause yet. 756690 10 tdanderson 2012-11-01 15:41:47 -0700 (In reply to comment #9) > (In reply to comment #7) > > I looked at the value of layoutOverflowRect().width() in RenderView::unscaledDocumentRect() after changing the text of the test case. Here is some data: > > > > (text on test page, layoutOverflowRect().width(), horizontal scrollbar present?) > > 'x', 575.296875, no > > 'xx', 575.609375, yes > > 'xxx', 575.906250, yes > > 'xxxx', 575.218750, no > > 'Hello, world!', 575.765625, yes > > > > This seemed to indicate that rounding (rather than always taking the floor) may be responsible for the one pixel difference. This patch indeed fixes the bug but is likely not a landable solution. > > > > An alternate solution could be to ensure that overflow is not falsely reported (perhaps using knownToHaveNoOverflow() ?). If |m_overflow| in RenderBox is never set, then layoutOverflowRect().width() will always be 575.000000. > > Rounding is, of course, necessary for sub-pixel layout to work properly. Are you saying that we're only setting m_overflow in the fail case? We should be treating all values as LayoutUnits when determining if we overflow. What's the delta between the client rect and the value we're using to determine that we have overflow when it occurs? If you turn off sub-pixel layout (in Platform.h, not FractionalLayoutUnit.h), does the problem go away? Do we not set m_overflow in that case? > > This is a step towards the problem, but it's not quite enough to understand the root cause yet. On further investigation, |m_overflow| (in RenderBox and InlineFlowBox) is getting set in both the pass case ('x') and the fail case ('xx'). When determining if we have overflow in the pass case, the overflow rect width is 0.296875 larger than the client rect. When determining if we have overflow in the fail case, the overflow rect width is 0.609375 larger than the client rect. Despite having |m_overflow| set, the pass case does not invoke a horizontal scrollbar because the 0.296875 disappears as a result of rounding when setting the integer contents size. If I disable sub-pixel layout as you suggested, the problem does indeed go away and |m_overflow| is never set in either RenderBox or InlineFlowBox. 757167 11 leviw 2012-11-02 07:10:49 -0700 (In reply to comment #10) > If I disable sub-pixel layout as you suggested, the problem does indeed go away and |m_overflow| is never set in either RenderBox or InlineFlowBox. Aha! That's most definitely a bug we can look into :) 761898 12 tdanderson 2012-11-08 09:59:50 -0800 Fixed with http://trac.webkit.org/changeset/133903 761900 13 eae 2012-11-08 10:01:46 -0800 Awesome, thanks for verifying Terry! 166534 2012-10-01 13:50:22 -0700 2012-10-01 13:50:22 -0700 Test case showing the subpixel text positioning size problem subpixel.html text/html 230 flackr PCFET0NUWVBFIGh0bWw+CjxodG1sPgo8aGVhZD4KPHN0eWxlPgpib2R5IHsKICBmb250LXNpemU6 IHNtYWxsOwogIG1hcmdpbjogMDsKICB0ZXh0LWFsaWduOiByaWdodDsKfQoKc3BhbiB7CiAgZGlz cGxheTogaW5saW5lLWJsb2NrOwp9Cjwvc3R5bGU+CjwvaGVhZD4KPGJvZHk+CiAgPHNwYW4+VGhp cyBwYWdlIHNob3VsZCBub3QgaGF2ZSBhIHNjcm9sbGJhci48L3NwYW4+CjwvYm9keT4KPC9odG1s Pgo= 171021 2012-10-26 14:56:28 -0700 2012-10-26 14:57:54 -0700 Patch bug-98062-20121026175500.patch text/plain 1523 tdanderson U3VidmVyc2lvbiBSZXZpc2lvbjogMTMyNjk2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNGVlMGIzMjA0YzBlMzg4 ODVhOTUzZGJlODk1YjdhNTExYWU1M2E4OS4uNzNlZTYxODU1NDkxMTE2ZDgzOTkxODJkNDNhNjBi ZGIwZGQyY2E0NiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDEyLTEwLTI2ICBUZXJy eSBBbmRlcnNvbiAgPHRkYW5kZXJzb25AY2hyb21pdW0ub3JnPgorCisgICAgICAgIFtjaHJvbWl1 bV0gU3VicGl4ZWwgdGV4dCBwb3NpdGlvbmluZyBjYXVzZXMgaW5jb3JyZWN0IGlubGluZS1ibG9j ayBsYXlvdXQuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD05ODA2MgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IE5vdCBmb3IgcmV2aWV3LiBQbGVhc2Ugc2VlIG15IGV4cGxhbmF0aW9uIG9mIHRoaXMgcGF0Y2gg aW4gdGhlIGJ1ZyBkZXNjcmlwdGlvbi4KKworICAgICAgICBObyBuZXcgdGVzdHMgKE9PUFMhKS4K KworICAgICAgICAqIHBsYXRmb3JtL0ZyYWN0aW9uYWxMYXlvdXRVbml0Lmg6CisgICAgICAgIChX ZWJDb3JlOjpzbmFwU2l6ZVRvUGl4ZWwpOgorCiAyMDEyLTEwLTI2ICBWaW5jZW50IFNjaGVpYiAg PHNjaGVpYkBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgR2VuZXJhdGVkIHNob3VsZCBub3QgYmUg c3VwcG9ydGVkIGZvciB0aGluZ3Mgd2l0aCBhIHNoYWRvdwpkaWZmIC0tZ2l0IGEvU291cmNlL1dl YkNvcmUvcGxhdGZvcm0vRnJhY3Rpb25hbExheW91dFVuaXQuaCBiL1NvdXJjZS9XZWJDb3JlL3Bs YXRmb3JtL0ZyYWN0aW9uYWxMYXlvdXRVbml0LmgKaW5kZXggMDY5OWIwNjQ5NjM5MjAwNzU5MGY5 YmU0YTJkMTBkMGY0ODYzNGI5NC4uNjg0OTYzMmE0YjkzZmYzNDZiMjI5OTM3ODdhZmZjNzc0Mzlj NmU4ZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vRnJhY3Rpb25hbExheW91 dFVuaXQuaAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9GcmFjdGlvbmFsTGF5b3V0VW5p dC5oCkBAIC04MTcsNyArODE3LDcgQEAgaW5saW5lIGZsb2F0JiBvcGVyYXRvci89KGZsb2F0JiBh LCBjb25zdCBGcmFjdGlvbmFsTGF5b3V0VW5pdCYgYikKIGlubGluZSBpbnQgc25hcFNpemVUb1Bp eGVsKEZyYWN0aW9uYWxMYXlvdXRVbml0IHNpemUsIEZyYWN0aW9uYWxMYXlvdXRVbml0IGxvY2F0 aW9uKSAKIHsKICAgICBGcmFjdGlvbmFsTGF5b3V0VW5pdCBmcmFjdGlvbiA9IGxvY2F0aW9uLmZy YWN0aW9uKCk7Ci0gICAgcmV0dXJuIChmcmFjdGlvbiArIHNpemUpLnJvdW5kKCkgLSBmcmFjdGlv bi5yb3VuZCgpOworICAgIHJldHVybiAoZnJhY3Rpb24gKyBzaXplKS5mbG9vcigpIC0gZnJhY3Rp b24uZmxvb3IoKTsKIH0KIAogfSAvLyBuYW1lc3BhY2UgV2ViQ29yZQo=