Bug 136801 – REGRESSION: Text with a zero offset, zero blur shadow vanishes

• Component: Layout and Rendering
• Status: RESOLVED FIXED

To reproduce, open the URL. The text is invisible.

[Myles C. Maxfield]

Created attachment 238150 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 238150 [details]
Patch

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

> Source/WebCore/rendering/TextPainter.cpp:82
> +    // often draw the ::last:: shadow and the text itself in a single call.

I don't like ::last::

> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:606
> +            extraOffset = TextPainter::applyShadowToGraphicsContext(*context, shadow, shadowRect, onlyDrawsShadow,
> +                TextPainter::isEmptyShadow(shadow, MainTextIsOpaqueOrNot::MainTextIsNotOpaque), Horizontal);

I think the enumification is overzealous. This is the only call site with default values for the last few parameters, and that could have been avoided by giving them default values in the declaration. Your enumification creates ugliness like DrawsRealTextAlongWithShadowOrNot::DoesNotDrawRealTextAlongWithShadow in the imp, which makes me throw up a little.

[Tim Horton]

Could just be "DrawsRealTextAlongWithShadow::{Yes, No}". I think that has been suggested to me before... don't know if that would be better.

[Myles C. Maxfield]

Created attachment 238187 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 238187 [details]
Patch

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

> Source/WebCore/rendering/TextPainter.h:56
> +enum class MainTextIsOpaque {
> +    Yes,
> +    No
> +};
> +enum class ShadowIsEmpty {
> +    Yes,
> +    No
> +};
> +enum class DrawsRealTextAlongWithShadow {
> +    Yes,
> +    No
> +};

I still don't like these.

First "real" doesn't add anything to the last one. (What text is not "real" text?)

Secondly, I think they are not very readable in use:

if (opaque == MainTextIsOpaque::No)

makes me have to stop and think. This would read better as if (!opaque). I can't come up with good enum names that would make it like if (opaque == MainTextIsOpaque).

[Myles C. Maxfield]

Created attachment 238222 [details]
Patch

[Myles C. Maxfield]

Created attachment 238224 [details]
Patch

[Myles C. Maxfield]

Created attachment 238227 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 238227 [details]
Patch

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

> Source/WebCore/rendering/TextPainter.cpp:68
> +FloatSize TextPainter::applyShadowToGraphicsContext(GraphicsContext& context, const ShadowData& shadow, const FloatRect& textRect, bool onlyDrawsShadow, bool emptyShadow, FontOrientation orientation)

emptyShadow -> shadowIsEmpty

> Source/WebCore/rendering/TextPainter.cpp:103
> +    bool lastShadowIterationDrawsRealTextAsWell = !stroked && opaque;

"real text"? "as well" doesn't seem necessary.

[Myles C. Maxfield]

Created attachment 238236 [details]
Patch

[Darin Adler]

Comment on attachment 238236 [details]
Patch

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

It’s OK to rearrange like this. No need to have this live in InlineTextBox. But this is unnecessarily tricky to use, and the applyShadowToGraphicsContext function has two confusing boolean arguments!

I’m going to say review- because I think we can factor this better.

> Source/WebCore/rendering/TextPainter.cpp:103
> +    bool lastShadowIterationDrawsText = !stroked && opaque;

This should be named lastShadowIterationShouldDrawText, I think. But maybe it won’t exist at all after some more refactoring.

> Source/WebCore/rendering/TextPainter.cpp:108
> +        bool emptyShadow = TextPainter::isEmptyShadow(shadow, opaque);

It’s not great for a local variable to be named "emptyShadow" when it's not a shadow, but rather is a boolean.

> Source/WebCore/rendering/TextPainter.cpp:109
> +        bool onlyDrawsShadow = TextPainter::isLastShadowIteration(shadow) && lastShadowIterationDrawsText;

I am deeply puzzled by the logic here. It says that when we want to draw text, then we should only draw a shadow. That can’t be right so I must be reading it wrong.

> Source/WebCore/rendering/TextPainter.cpp:136
> +        if (!onlyDrawsShadow)
> +            context.restore(); // Matching call to context.save() in applyShadowToGraphicsContext
>          else
>              context.clearShadow();

Seems to me this would be clearer if these four lines of code were a function like applyShadowToGraphicsContext, maybe called removeShadowFromGraphicsContext or unapplyShadowFromGraphicsContext. Another way to do it would be to use a class for this and have the destructor do this work. That would concentrate the code to set things up and clean them up in one place.

> Source/WebCore/rendering/TextPainter.h:59
> +    static inline bool isLastShadowIteration(const ShadowData* shadow)
> +    {
> +        return shadow && !shadow->next();
> +    }
> +    static inline bool isEmptyShadow(const ShadowData* shadow, bool textIsOpaque = false)
> +    {
> +        return textIsOpaque && shadow && shadow->location() == IntPoint() && !shadow->radius();
> +    }

I understand these need for these two functions to be defined in the header file since they are inlined. But I suggest putting them outside the class definition, at the end of the header file, rather than having them inline. I think this makes the header easier to read.

I don’t understand what an “empty shadow” is, which makes the isEmptyShadow quite unpleasant. It’s something different from “no shadow at all”. I think it’s a shadow that is completely covered by opaque text perhaps, but then it’s not really a kind of shadow, so the function name is not good. It really seems bad to have to pass a “text is opaque” boolean to a function that claims to be doing something to a shadow.

> Source/WebCore/rendering/TextPainter.h:61
> +    static FloatSize applyShadowToGraphicsContext(GraphicsContext&, const ShadowData&, const FloatRect& textRect, bool onlyDrawsShadow, bool shadowIsEmpty, FontOrientation = Horizontal);

I don’t like the separation of responsibilities here. I think we should have a single helper function that does everything, or perhaps two, one to start and one to end, not three functions that have to be used carefully with boolean parameters. Maybe this set of three functions should be a class; that could help us correctly unapply the shadow from the graphics context in exactly the right cases without having to be so careful about how we write the code at the call sites.

It seems to me that applyShadowToGraphicsContext can return some indication that we should skip the shadow entirely, rather than requiring a relatively subtle and complex check at both call sites.

[Myles C. Maxfield]

<rdar://problem/18421210>

[Myles C. Maxfield]

Comment on attachment 238236 [details]
Patch

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

>> Source/WebCore/rendering/TextPainter.cpp:103
>> +    bool lastShadowIterationDrawsText = !stroked && opaque;
> 
> This should be named lastShadowIterationShouldDrawText, I think. But maybe it won’t exist at all after some more refactoring.

Done.

>> Source/WebCore/rendering/TextPainter.cpp:108
>> +        bool emptyShadow = TextPainter::isEmptyShadow(shadow, opaque);
> 
> It’s not great for a local variable to be named "emptyShadow" when it's not a shadow, but rather is a boolean.

Done.

>> Source/WebCore/rendering/TextPainter.cpp:109
>> +        bool onlyDrawsShadow = TextPainter::isLastShadowIteration(shadow) && lastShadowIterationDrawsText;
> 
> I am deeply puzzled by the logic here. It says that when we want to draw text, then we should only draw a shadow. That can’t be right so I must be reading it wrong.

This is actually the negation of what the name implies. I've switched it around and negated all the uses of it so the name is accurate. This fell out of lots of refactoring over the past few patches.

>> Source/WebCore/rendering/TextPainter.cpp:136
>>              context.clearShadow();
> 
> Seems to me this would be clearer if these four lines of code were a function like applyShadowToGraphicsContext, maybe called removeShadowFromGraphicsContext or unapplyShadowFromGraphicsContext. Another way to do it would be to use a class for this and have the destructor do this work. That would concentrate the code to set things up and clean them up in one place.

You are so right. I couldn't see the forest for the trees. I'm not sure about if I should create a new file for this new class. Given how small the class definition is, I've just put it in TextPainter.h. Please let me know if I should create a new .h and .cpp instead.

>> Source/WebCore/rendering/TextPainter.h:59
>> +    }
> 
> I understand these need for these two functions to be defined in the header file since they are inlined. But I suggest putting them outside the class definition, at the end of the header file, rather than having them inline. I think this makes the header easier to read.
> 
> I don’t understand what an “empty shadow” is, which makes the isEmptyShadow quite unpleasant. It’s something different from “no shadow at all”. I think it’s a shadow that is completely covered by opaque text perhaps, but then it’s not really a kind of shadow, so the function name is not good. It really seems bad to have to pass a “text is opaque” boolean to a function that claims to be doing something to a shadow.

Done.

I renamed it to shadowIsCompletelyCoveredByText().

>> Source/WebCore/rendering/TextPainter.h:61
>> +    static FloatSize applyShadowToGraphicsContext(GraphicsContext&, const ShadowData&, const FloatRect& textRect, bool onlyDrawsShadow, bool shadowIsEmpty, FontOrientation = Horizontal);
> 
> I don’t like the separation of responsibilities here. I think we should have a single helper function that does everything, or perhaps two, one to start and one to end, not three functions that have to be used carefully with boolean parameters. Maybe this set of three functions should be a class; that could help us correctly unapply the shadow from the graphics context in exactly the right cases without having to be so careful about how we write the code at the call sites.
> 
> It seems to me that applyShadowToGraphicsContext can return some indication that we should skip the shadow entirely, rather than requiring a relatively subtle and complex check at both call sites.

A class is so much better.

[Myles C. Maxfield]

Created attachment 238512 [details]
Patch

[Myles C. Maxfield]

Created attachment 238571 [details]
Patch

[Darin Adler]

Comment on attachment 238512 [details]
Patch

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

I think we can do considerably better.

> Source/WebCore/rendering/TextPainter.cpp:83
> +    // often draw the ::last:: shadow and the text itself in a single call.

I don’t think ::last:: here is better than just last or possibly *last*.

> Source/WebCore/rendering/TextPainter.cpp:131
> +            extraOffset = roundedIntSize(shadowApplier.extraOffset());

I don’t think this it adds clarity to have this inside an if statement any more. We should do it without the null shadow check.

> Source/WebCore/rendering/TextPainter.h:79
> +    ShadowApplier(GraphicsContext&, const ShadowData*, const FloatRect& textRect, bool onlyDrawsShadow, bool shadowIsEmpty, FontOrientation = Horizontal);

Can we replace these two booleans with an enum: DrawTextOnly, DrawShadowOnly, or DrawTextAndShadow? Or two booleans: drawText and drawShadow. Or even better, take my advice about not passing anything in that the ShadowApplier can compute, and pass in only what the caller knows that ShadowApplier doesn’t know.

> Source/WebCore/rendering/TextPainter.h:82
> +private:

Nicer with a blank line above this to separate the two.

> Source/WebCore/rendering/TextPainter.h:97
> +inline bool isLastShadowIteration(const ShadowData* shadow)
> +{
> +    return shadow && !shadow->next();
> +}
> +
> +inline bool shadowIsCompletelyCoveredByText(const ShadowData* shadow, bool textIsOpaque = false)
> +{
> +    return textIsOpaque && shadow && shadow->location() == IntPoint() && !shadow->radius();
> +}

It seems to me that we can put all this logic into ShadowApplier instead of having these two separate functions, if we let the applier also decide that there is nothing to draw. We can add a boolean member function that tells the caller there is nothing to draw, which can be called on the applier. We can also set m_shadow to nullptr in that case even if the passed-in shadow was non-null, so the destructor won’t do any work.

> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:603
> +            context->save();

Too bad that we go GraphicsContext::save/restore twice here if ShadowApplier decided to save the context; that can be an expensive operation. If we add a member function didSaveContext() to ShadowApplier, and then only save/restore here if the ShadowApplier didn’t already save/restore.

[Darin Adler]

Comment on attachment 238571 [details]
Patch

This is OK, but see my comments because I think we can do even better.

[Myles C. Maxfield]

Comment on attachment 238512 [details]
Patch

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

>> Source/WebCore/rendering/TextPainter.cpp:83
>> +    // often draw the ::last:: shadow and the text itself in a single call.
> 
> I don’t think ::last:: here is better than just last or possibly *last*.

Done.

>> Source/WebCore/rendering/TextPainter.cpp:131
>> +            extraOffset = roundedIntSize(shadowApplier.extraOffset());
> 
> I don’t think this it adds clarity to have this inside an if statement any more. We should do it without the null shadow check.

Yep - that's what that last patch is for.

>> Source/WebCore/rendering/TextPainter.h:79
>> +    ShadowApplier(GraphicsContext&, const ShadowData*, const FloatRect& textRect, bool onlyDrawsShadow, bool shadowIsEmpty, FontOrientation = Horizontal);
> 
> Can we replace these two booleans with an enum: DrawTextOnly, DrawShadowOnly, or DrawTextAndShadow? Or two booleans: drawText and drawShadow. Or even better, take my advice about not passing anything in that the ShadowApplier can compute, and pass in only what the caller knows that ShadowApplier doesn’t know.

I did the latter - only passing in lastShadowIterationShouldDrawText and opaque. onlyDrawsShadow and avoidDrawingShadow are both computed in ShadowApplier::ShadowApplier().

>> Source/WebCore/rendering/TextPainter.h:82
>> +private:
> 
> Nicer with a blank line above this to separate the two.

Done.

>> Source/WebCore/rendering/TextPainter.h:97
>> +}
> 
> It seems to me that we can put all this logic into ShadowApplier instead of having these two separate functions, if we let the applier also decide that there is nothing to draw. We can add a boolean member function that tells the caller there is nothing to draw, which can be called on the applier. We can also set m_shadow to nullptr in that case even if the passed-in shadow was non-null, so the destructor won’t do any work.

Done. These two functions are now private.

>> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:603
>> +            context->save();
> 
> Too bad that we go GraphicsContext::save/restore twice here if ShadowApplier decided to save the context; that can be an expensive operation. If we add a member function didSaveContext() to ShadowApplier, and then only save/restore here if the ShadowApplier didn’t already save/restore.

Done.

[Myles C. Maxfield]

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

[Chris Dumez]

Looks like this broke the build for quite a few bots.

[Chris Dumez]

(In reply to comment #20)
> Looks like this broke the build for quite a few bots.

Landed a build fix in http://trac.webkit.org/changeset/173942