Bug 98062 – [chromium] Subpixel text positioning causes incorrect inline-block layout.

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

[Robert Flack]

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.

[Levi Weintraub]

Super interesting. Can you give attaching the test page another try?

[Robert Flack]

Created attachment 166534 [details]
Test case showing the subpixel text positioning size problem

[Levi Weintraub]

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.

[Levi Weintraub]

Sorry, I missed the "subpixel text positioning" part. Sorry!

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.

[Terry Anderson]

Created attachment 171021 [details]
Patch

[Terry Anderson]

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

[Terry Anderson]

Levi or Emil, are you able to provide any guidance about the best way to proceed here?

[Levi Weintraub]

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

[Terry Anderson]

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

[Levi Weintraub]

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

[Terry Anderson]

Fixed with http://trac.webkit.org/changeset/133903

[Emil A Eklund]

Awesome, thanks for verifying Terry!