Bug 48074 – [Cairo] Text underline is not shadowed when text-shadow is enabled

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

[Martin Robinson]

When text-shadow is enabled, text underline are not shadowed. This can be observed in the GTK+ pixel result for fast/text/stroking-decorations.html.

[Martin Robinson]

Created attachment 71448 [details]
Patch for this issue

[Dirk Schulze]

Comment on attachment 71448 [details]
Patch for this issue

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

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:281
> +    int numSegments = (distance - remainder) / patternWidth;

You really want to use a integer division?

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:294
> +            return (patternWidth - remainder) + (remainder / 2.0);
> +        return patternWidth / 2.0;

2.f

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:299
> +        return (patternWidth - remainder) / 2.0;

2.f

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:341
> +        double patternWidthAsDouble = patternWidth;

You already defined patternWidth as double above :-P

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:359
> +    cairo_save(cairoContext);
> +    drawLineOnCairoContext(this, cairoContext, point1, point2);
> +    cairo_restore(cairoContext);

Why do you use cairo_save? What gets restored after drawLineOnCairoContext()? Maybe it's faster to do it manually?

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:421
> +        patternWidth = strokeThickness() / 2.0;

patternWidth is an integer, please use floorf or ceilf

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:424
> +        patternWidth = 3.0 * strokeThickness() / 2.0;

Ditto.

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:435
>              distance = static_cast<int>((M_PI * hRadius) / 2.0);

Can you use floor(f) or ceil(f) here please? And 2.f should be ok. Don't use M_PI, take piDouble or piFloat from <wtf/MathExtras.h>

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:437
>              distance = static_cast<int>((M_PI * sqrtf((hRadius * hRadius + vRadius * vRadius) / 2.0)) / 2.0);

Ditto.

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:440
> +        float patternOffset = calculateStrokePatternOffset(distance, patternWidth);
> +        double patternWidthAsDouble = patternWidth;

why don't you use double directly and avoid copying the data?

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:441
> +        cairo_set_dash(cr, &patternWidthAsDouble, 1, patternOffset);

hm, does cairo_set_dash really use a double and a float? I thought cairo always uses double.

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:681
> +    cairo_restore(cairoContext);

Again, is save and restore necessary here?

[Martin Robinson]

(In reply to comment #2)

Thanks for the review!

> (From update of attachment 71448 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71448&action=review
> 
> > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:281
> > +    int numSegments = (distance - remainder) / patternWidth;
> You really want to use a integer division?

This is from the original code, as far as I can tell (patWidth) is an int. I hoped to not change the behavior of this portion at all, but I can address that in a followup patch, if necesarry.

-        int coverage = distance-remainder;
-        int numSegments = coverage/patWidth;
-

> 2.f
> 2.f

The style guide is actually inconclusive on this and in the one example that is shows, it shows the style I used in the patch: "Floating point literals" at http://webkit.org/coding/coding-style.html. Maybe this should be decided on the list, if there is a preference one way or another?

> > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:341
> > +        double patternWidthAsDouble = patternWidth;
> You already defined patternWidth as double above :-P

Good point! I've changed it to an int.

> > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:359
> > +    cairo_save(cairoContext);
> > +    drawLineOnCairoContext(this, cairoContext, point1, point2);
> > +    cairo_restore(cairoContext);
> 
> Why do you use cairo_save? What gets restored after drawLineOnCairoContext()? Maybe it's faster to do it manually?

Unfortunately, drawLineOnCairoContext must call cairo_set_dash and there doesn't appear to be a way to fetch from the context and restore manually. I'm in agreement though that, in general, we shouldn't rely on the cairo context to store the state. We can avoid most needless save/restore pairs that way. So if GraphicsContext::setPlatformStrokeStyle didn't call cairo_set_dash, but just marked it as dirty, we could update the context lazily.

> > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:421
> > +        patternWidth = strokeThickness() / 2.0;
> patternWidth is an integer, please use floorf or ceilf

The original code used patWidth = static_cast<int>(width / 2);, so I guess floorf is the right call here?

> > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:435
> >              distance = static_cast<int>((M_PI * hRadius) / 2.0);
> 
> Can you use floor(f) or ceil(f) here please? And 2.f should be ok. Don't use M_PI, take piDouble or piFloat from <wtf/MathExtras.h>

I will change all references to M_PI to piFloat.

> > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:440
> > +        float patternOffset = calculateStrokePatternOffset(distance, patternWidth);
> > +        double patternWidthAsDouble = patternWidth;
> why don't you use double directly and avoid copying the data?

The original code used an int here and I was worrying about causing a regresson by making too many changes. I can modify this to use double everywhere though. Doing this in general, might remove the need to call floorf/ceilf so much. I'm just nervous about adjusting the code too much, as it appears sensitive.

> > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:441
> > +        cairo_set_dash(cr, &patternWidthAsDouble, 1, patternOffset);
> hm, does cairo_set_dash really use a double and a float? I thought cairo always uses double.

Fixed! Thank you.

> > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:681
> > +    cairo_restore(cairoContext);
> Again, is save and restore necessary here?

Answered above.

[Martin Robinson]

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

[Martin Robinson]

Created attachment 84137 [details]
Patch with Krit's suggestions and a good deal of cleanup

[Dirk Schulze]

Comment on attachment 84137 [details]
Patch with Krit's suggestions and a good deal of cleanup

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

I'd like to see it a second time.

> Source/WebCore/platform/graphics/GraphicsContext.h:509
> +

Remove this line

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:326
> +    bool isVerticalLine = (point1.x() == point2.x());

avoid the braces

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:431
> +        patternWidth = floor(strokeThickness() / 2.f);

floorf, but depends what strokeThickness returns. please check this.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:434
> +        patternWidth = 3 * floor(strokeThickness() / 2.f);

ditto

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:445
> +            distance = floor((piFloat * hRadius) / 2.0);

floorf

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:447
> +            distance = floor((piFloat * sqrtf((hRadius * hRadius + vRadius * vRadius) / 2.0)) / 2.0);

ditto

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:448
> +        double patternOffset = calculateStrokePatternOffset(distance, patternWidth);

if patternOffset is double, you should may use double in general and ignore the last two comments.

[Martin Robinson]

(In reply to comment #6)
> (From update of attachment 84137 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84137&action=review


> I'd like to see it a second time.
Thank you kindly for the review.

> > Source/WebCore/platform/graphics/GraphicsContext.h:509
> > +
> Remove this line

Done.

> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:326
> > +    bool isVerticalLine = (point1.x() == point2.x());
> avoid the braces

Done.

> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:431
> > +        patternWidth = floor(strokeThickness() / 2.f);
> floorf, but depends what strokeThickness returns. please check this.
> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:434
> > +        patternWidth = 3 * floor(strokeThickness() / 2.f);
> ditto
> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:445
> > +            distance = floor((piFloat * hRadius) / 2.0);
> floorf
> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:447
> > +            distance = floor((piFloat * sqrtf((hRadius * hRadius + vRadius * vRadius) / 2.0)) / 2.0);
> ditto

Done!

> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:448
> > +        double patternOffset = calculateStrokePatternOffset(distance, patternWidth);
> if patternOffset is double, you should may use double in general and ignore the last two comments.

I've changed distance to a florat moved the floorf here.

[Martin Robinson]

Created attachment 91327 [details]
Patch

[Dirk Schulze]

Comment on attachment 91327 [details]
Patch

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

> LayoutTests/ChangeLog:12
> +        * platform/gtk/fast/text/stroking-decorations-expected.checksum:
> +        * platform/gtk/fast/text/stroking-decorations-expected.png:

At least the checksum is missing. And I can't see the image with the review tool :-(. But that is not your problem.

> Source/WebCore/ChangeLog:8
> +        Use ShadowContext to enable shadows for text underlines. Also remove quite a bit

You mean ContextShadow

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:443
> +            distance = (piFloat * hRadius) / 2.0;

2 or 2.f if you use float. But 2 shouldn't be a problem here.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:445
> +            distance = (piFloat * sqrtf((hRadius * hRadius + vRadius * vRadius) / 2.0)) / 2.0;

Ditto.

[Martin Robinson]

Committed r85279: <http://trac.webkit.org/changeset/85279>

[Martin Robinson]

Comment on attachment 91327 [details]
Patch

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

Thanks for the review!

>> LayoutTests/ChangeLog:12
>> +        * platform/gtk/fast/text/stroking-decorations-expected.png:
> 
> At least the checksum is missing. And I can't see the image with the review tool :-(. But that is not your problem.

There are no longer checksum files, but this line was left by accident in the ChangeLog. I've removed it.

>> Source/WebCore/ChangeLog:8
>> +        Use ShadowContext to enable shadows for text underlines. Also remove quite a bit
> 
> You mean ContextShadow

Fixed.

>> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:443
>> +            distance = (piFloat * hRadius) / 2.0;
> 
> 2 or 2.f if you use float. But 2 shouldn't be a problem here.

Changed everything to x.f.