Bug 132770 – [Mac] [iOS] Underlines are too low

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[Mac] [iOS] Underlines are too low

[Myles C. Maxfield]

Created attachment 231215 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/16364632>

[Darin Adler]

Comment on attachment 231215 [details]
Patch

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

> Source/WebCore/rendering/InlineTextBox.cpp:1015
> +    float textDecorationThickness = roundf(fontSizeScaling);

Seems really strange to “round a scaling”; why do strokes need to be integers? Wouldn’t the pixel ratio have something to do with the rounding we want to do, rather than just integer boundaries. Rationale might be helpful.

Is it really OK to round the thickness down to 0 if the font size is <1/2 the size of the text decoration base font size?

I think getting rid of the fontSizeScaling local would be helpful, since it would be incorrect to use that value in the code below.

[zalan]

(In reply to comment #3)
> (From update of attachment 231215 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231215&action=review
> 
> > Source/WebCore/rendering/InlineTextBox.cpp:1015
> > +    float textDecorationThickness = roundf(fontSizeScaling);
> 
> Seems really strange to “round a scaling”; why do strokes need to be integers? Wouldn’t the pixel ratio have something to do with the rounding we want to do, rather than just integer boundaries. Rationale might be helpful.
Agree, integral rounding at painting time needs strong justification.

[Myles C. Maxfield]

*** Bug 132722 has been marked as a duplicate of this bug. ***

[Myles C. Maxfield]

Comment on attachment 231215 [details]
Patch

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

>>> Source/WebCore/rendering/InlineTextBox.cpp:1015
>>> +    float textDecorationThickness = roundf(fontSizeScaling);
>> 
>> Seems really strange to “round a scaling”; why do strokes need to be integers? Wouldn’t the pixel ratio have something to do with the rounding we want to do, rather than just integer boundaries. Rationale might be helpful.
>> 
>> Is it really OK to round the thickness down to 0 if the font size is <1/2 the size of the text decoration base font size?
>> 
>> I think getting rid of the fontSizeScaling local would be helpful, since it would be incorrect to use that value in the code below.
> 
> Agree, integral rounding at painting time needs strong justification.

Done. I was keeping the roundf() from above, but I think the two of you are right.

[Myles C. Maxfield]

https://trac.webkit.org/changeset/168599

[Darin Adler]

While the roundf is probably not right, removing it could have big effects; we might start drawing anti-aliased grey underlines that don’t look good where before we would be rounding up or down and drawing much-different-looking black underlines. We need to follow up with plenty of testing.

[zalan]

(In reply to comment #7)
> https://trac.webkit.org/changeset/168599
+float textDecorationThickness = renderer().style().fontSize() / textDecorationBaseFontSize; 
This should be pixel snapped to avoid lines like 1.6px -to not to produce anti-aliased lines.

+FloatPoint deviceOrigin = FloatPoint(roundf(devicePoint.x()), ceilf(devicePoint.y()));
This is integral rounding, so it will most likely end up snapping to wrong coordinates in certain edge cases. I think we should not round in this function at all and the caller (or where we have the context of the rounding strategy) should do the snapping instead.

[Myles C. Maxfield]

(In reply to comment #9)
> (In reply to comment #7)
> > https://trac.webkit.org/changeset/168599
> +float textDecorationThickness = renderer().style().fontSize() / textDecorationBaseFontSize; 
> This should be pixel snapped to avoid lines like 1.6px -to not to produce anti-aliased lines.
> 
> +FloatPoint deviceOrigin = FloatPoint(roundf(devicePoint.x()), ceilf(devicePoint.y()));
> This is integral rounding, so it will most likely end up snapping to wrong coordinates in certain edge cases. I think we should not round in this function at all and the caller (or where we have the context of the rounding strategy) should do the snapping instead.

<rdar://problem/16903502>

[Myles C. Maxfield]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #7)
> > > https://trac.webkit.org/changeset/168599
> > +float textDecorationThickness = renderer().style().fontSize() / textDecorationBaseFontSize; 
> > This should be pixel snapped to avoid lines like 1.6px -to not to produce anti-aliased lines.
> > 
> > +FloatPoint deviceOrigin = FloatPoint(roundf(devicePoint.x()), ceilf(devicePoint.y()));
> > This is integral rounding, so it will most likely end up snapping to wrong coordinates in certain edge cases. I think we should not round in this function at all and the caller (or where we have the context of the rounding strategy) should do the snapping instead.
> 
> <rdar://problem/16903502>

I confused the Radar Bug Importer with my last comment.

The new radar I filed for moving the pixel-snapping code is at <rdar://problem/16903502>

<rdar://problem/16364632>

[Myles C. Maxfield]

http://trac.webkit.org/changeset/168599