Bug 111005 – Underline should round to match other content.

• Component: Platform
• Status: RESOLVED FIXED

Underline should round to match other content.

Created attachment 190582 [details]
Patch

Created attachment 190618 [details]
Patch

With http://trac.webkit.org/changeset/117528 the old subpixel placement of underlines was replaced with a snapping version. However, that implementation takes the floor of the y value, while all other content uses the rounded y value. This means that the underlines do not move in step with other content, including text. See crbug.com/173065 for more explanation.

This change simply simply adds 0.5 to the y value in order to round the same as other content. It appears that one image test is affected. In the future this should use a valid implementation of roundToDevicePixels as the current implementation (and this newer one) cause the underline to snap to css pixels instead of physical pixels. As a result, on HiDPI devices the underline is snapped to even pixels and only moves once while the text (drawn to device pixels) moves twice as it is slowly moved vertically.

Adding those involved in the 117528 change, as they'll probably be more familiar with this.

[Stephen White]

Comment on attachment 190618 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

You should mention the affected tests here (e.g., decorations-with-text-combine).

Created attachment 190743 [details]
Patch

[Levi Weintraub]

Comment on attachment 190743 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:754
> +    // Round to nearest pixel to match text and other content.
> +    r.fTop = WebCoreFloatToSkScalar(floorf(pt.y() + 0.5f));

Do we ever have negative values here? LayoutUnit rounds differently around zero: http://trac.webkit.org/browser/trunk/Source/WebCore/platform/LayoutUnit.h#L225

(In reply to comment #7)
> (From update of attachment 190743 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190743&action=review
> 
> > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:754
> > +    // Round to nearest pixel to match text and other content.
> > +    r.fTop = WebCoreFloatToSkScalar(floorf(pt.y() + 0.5f));
> 
> Do we ever have negative values here? LayoutUnit rounds differently around zero: http://trac.webkit.org/browser/trunk/Source/WebCore/platform/LayoutUnit.h#L225

Skia currently always rounds X.5 up (towards +inf) when rounding text positions. As a result, this change should always keep the line in-step with the text. If the content rounds differently, it already rounds differently from the text. This may require some follow-up investigation, as noted in Comment #3 about roundToDevicePixels being implemented.

[Stephen White]

Comment on attachment 190743 [details]
Patch

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

>>> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:754
>>> +    r.fTop = WebCoreFloatToSkScalar(floorf(pt.y() + 0.5f));
>> 
>> Do we ever have negative values here? LayoutUnit rounds differently around zero: http://trac.webkit.org/browser/trunk/Source/WebCore/platform/LayoutUnit.h#L225
> 
> Skia currently always rounds X.5 up (towards +inf) when rounding text positions. As a result, this change should always keep the line in-step with the text. If the content rounds differently, it already rounds differently from the text. This may require some follow-up investigation, as noted in Comment #3 about roundToDevicePixels being implemented.

Nit:  floating point constants don't get a trailing 'f' in WebKit code.

[Levi Weintraub]

Comment on attachment 190743 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Test: fast/text/decorations-with-text-combine.html

So this test already covers this? Is that why you're not adding one? I'd feel more comfortable if you had test expectations for at least one platform with this patch so the difference could be seen.

(In reply to comment #9)
> (From update of attachment 190743 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190743&action=review
> 
> >>> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:754
> >>> +    r.fTop = WebCoreFloatToSkScalar(floorf(pt.y() + 0.5f));
> >> 
> >> Do we ever have negative values here? LayoutUnit rounds differently around zero: http://trac.webkit.org/browser/trunk/Source/WebCore/platform/LayoutUnit.h#L225
> > 
> > Skia currently always rounds X.5 up (towards +inf) when rounding text positions. As a result, this change should always keep the line in-step with the text. If the content rounds differently, it already rounds differently from the text. This may require some follow-up investigation, as noted in Comment #3 about roundToDevicePixels being implemented.
> 
> Nit:  floating point constants don't get a trailing 'f' in WebKit code.

Indeed, this silly thing is in the style guide. Why make calculations be done with doubles only to then convert them back to float? There is no equivalent to CGFloat in WebKit, and everything is float, so why not force 'f' instead? I have already gone through all stages of grief except acceptance. Do I not get a pass since GraphicsContext::rotate(float) in this file also uses 'f'?

(In reply to comment #10)
> (From update of attachment 190743 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190743&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Test: fast/text/decorations-with-text-combine.html
> 
> So this test already covers this? Is that why you're not adding one? I'd feel more comfortable if you had test expectations for at least one platform with this patch so the difference could be seen.

This test is the only one which changes. The effect of this change is to move one of the vertical lines in this test one pixel to the right. Every other test which uses an underline is also covering this, but I would prefer not to list all of those. None of them seem to have changed.

Created attachment 190763 [details]
Patch

[Levi Weintraub]

(In reply to comment #12)
> (In reply to comment #10)
> > (From update of attachment 190743 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=190743&action=review
> > 
> > > Source/WebCore/ChangeLog:8
> > > +        Test: fast/text/decorations-with-text-combine.html
> > 
> > So this test already covers this? Is that why you're not adding one? I'd feel more comfortable if you had test expectations for at least one platform with this patch so the difference could be seen.
> 
> This test is the only one which changes. The effect of this change is to move one of the vertical lines in this test one pixel to the right. Every other test which uses an underline is also covering this, but I would prefer not to list all of those. None of them seem to have changed.

If the rest don't change, they're not covering this :-/

Ideally, we'd have a simple test that would make clear the regression if this change were ever broken. Why aren't you bundling the updated test expectation for the platform you're using with this patch?

[Stephen White]

Comment on attachment 190763 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:754
> +    r.fTop = WebCoreFloatToSkScalar(floorf(pt.y() + static_cast<float>(0.5)));

Heh, cute...

Actually, reading this rule again, it says "*Unless required in order to force floating point math*, do not append .0, .f and .0f to floating point literals."

Since you do actually want to force floating point math here, I think f is fine.  Sorry for the runaround..

(In reply to comment #14)
> (In reply to comment #12)
> > (In reply to comment #10)
> > > (From update of attachment 190743 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=190743&action=review
> > > 
> > > > Source/WebCore/ChangeLog:8
> > > > +        Test: fast/text/decorations-with-text-combine.html
> > > 
> > > So this test already covers this? Is that why you're not adding one? I'd feel more comfortable if you had test expectations for at least one platform with this patch so the difference could be seen.
> > 
> > This test is the only one which changes. The effect of this change is to move one of the vertical lines in this test one pixel to the right. Every other test which uses an underline is also covering this, but I would prefer not to list all of those. None of them seem to have changed.
> 
> If the rest don't change, they're not covering this :-/
They are, there shouldn't be any change, but because of the silly way in which vertical text is implemented in WebKit (horizontal layout but rotated) this one changed. It wouldn't be changing if vertical text layout were sane.
> 
> Ideally, we'd have a simple test that would make clear the regression if this change were ever broken. Why aren't you bundling the updated test expectation for the platform you're using with this patch?
Because the change is very, very minor and I'm on precise, so it's very difficult to get a new baseline locally. Beside that, I would never trust any locally produced baseline, there are too many things which can go wrong with that. If you have a usable means of generating baselines ahead of time, I'm all ears. A while ago I was fighting to make the results of layout tests retrievable from try bots, but I'm not sure where that ended up.

Also, this is going to change again anyway (the text part at least), since there is a related Skia change which will go in with the next mass Skia re-baseline. I'm also against adding new pixel tests, if it can be at all avoided. Also, as mentioned in Comment #3, this is still known to be wrong, just less wrong. roundToDevicePixels needs to be implemented and then used here.

[Stephen White]

Comment on attachment 190743 [details]
Patch

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

OK.  Let's go with this version.  r=me

>>>>> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:754
>>>>> +    r.fTop = WebCoreFloatToSkScalar(floorf(pt.y() + 0.5f));
>>>> 
>>>> Do we ever have negative values here? LayoutUnit rounds differently around zero: http://trac.webkit.org/browser/trunk/Source/WebCore/platform/LayoutUnit.h#L225
>>> 
>>> Skia currently always rounds X.5 up (towards +inf) when rounding text positions. As a result, this change should always keep the line in-step with the text. If the content rounds differently, it already rounds differently from the text. This may require some follow-up investigation, as noted in Comment #3 about roundToDevicePixels being implemented.
>> 
>> Nit:  floating point constants don't get a trailing 'f' in WebKit code.
> 
> Indeed, this silly thing is in the style guide. Why make calculations be done with doubles only to then convert them back to float? There is no equivalent to CGFloat in WebKit, and everything is float, so why not force 'f' instead? I have already gone through all stages of grief except acceptance. Do I not get a pass since GraphicsContext::rotate(float) in this file also uses 'f'?

Sure.

[Levi Weintraub]

(In reply to comment #16)
> (In reply to comment #14)
> > (In reply to comment #12)
> > > (In reply to comment #10)
> > > > (From update of attachment 190743 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=190743&action=review
> > > > 
> > > > > Source/WebCore/ChangeLog:8
> > > > > +        Test: fast/text/decorations-with-text-combine.html
> > > > 
> > > > So this test already covers this? Is that why you're not adding one? I'd feel more comfortable if you had test expectations for at least one platform with this patch so the difference could be seen.
> > > 
> > > This test is the only one which changes. The effect of this change is to move one of the vertical lines in this test one pixel to the right. Every other test which uses an underline is also covering this, but I would prefer not to list all of those. None of them seem to have changed.
> > 
> > If the rest don't change, they're not covering this :-/
> They are, there shouldn't be any change, but because of the silly way in which vertical text is implemented in WebKit (horizontal layout but rotated) this one changed. It wouldn't be changing if vertical text layout were sane.

I really don't understand this argument. I do understand that this doesn't affect the way the majority of tests with underlines render, but it's still testable. Absent the updated expectations, it's impossible for me to verify this is correct, and ignoring the one test that covers it on all platforms means it lands without coverage.

Also for the record, I'm running precise and pass the pixel tests for that tests locally.

> > 
> > Ideally, we'd have a simple test that would make clear the regression if this change were ever broken. Why aren't you bundling the updated test expectation for the platform you're using with this patch?
> Because the change is very, very minor and I'm on precise, so it's very difficult to get a new baseline locally. Beside that, I would never trust any locally produced baseline, there are too many things which can go wrong with that. If you have a usable means of generating baselines ahead of time, I'm all ears. A while ago I was fighting to make the results of layout tests retrievable from try bots, but I'm not sure where that ended up.
> 
> Also, this is going to change again anyway (the text part at least), since there is a related Skia change which will go in with the next mass Skia re-baseline. I'm also against adding new pixel tests, if it can be at all avoided. Also, as mentioned in Comment #3, this is still known to be wrong, just less wrong. roundToDevicePixels needs to be implemented and then used here.

[Levi Weintraub]

To be clear, I defer to Stephen on this. You'll need to uncheck your previous patch as obsolete to land it with his R+ CQ+.

[WebKit Review Bot]

Comment on attachment 190743 [details]
Patch

Clearing flags on attachment: 190743

Committed r144375: <http://trac.webkit.org/changeset/144375>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

(In reply to comment #21)
> All reviewed patches have been landed.  Closing bug.

Re-baseline and removal of line from TestExpectations with 144380 and 144396.