Bug 136612 – REGRESSION (r172153): Text drawn with wrong color when second text shadow has zero offset and blur (breaks buttons at aws.amazon.com)

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

To reproduce, open the URL. The text is drawn in black, but it should be green.

This was caused by <http://trac.webkit.org/r172153>, the fix for bug 135357.

<rdar://problem/18260645>

[Myles C. Maxfield]

Created attachment 237805 [details]
Patch

[Darin Adler]

Comment on attachment 237805 [details]
Patch

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

> Source/WebCore/rendering/TextPainter.cpp:68
>  static bool isEmptyShadow(const ShadowData* shadow)
>  {
>      if (!shadow)
> -        return true;
> +        return false;
>      return shadow->location() == IntPoint() && !shadow->radius();
>  }

I think it’s confusing to change the behavior of this function without changing its name. Maybe it should take a reference and the null check could be at the call site. Or maybe we can just call this a pure bug fix and say that the old behavior was a bug.

> Source/WebCore/rendering/TextPainter.cpp:86
> +        if (isEmptyShadow(shadow)) {
> +            shadow = shadow->next();
> +            continue;
> +        }

Should put this before the definitions of extraOffset and didSaveContext.

> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:598
> +        bool didSaveContext = false;

You forgot to change the code below to use this boolean! It still says “if (shadow->next())” but should say “if (didSaveContext)”.

[Myles C. Maxfield]

Comment on attachment 237805 [details]
Patch

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

>> Source/WebCore/rendering/TextPainter.cpp:68
>>  }
> 
> I think it’s confusing to change the behavior of this function without changing its name. Maybe it should take a reference and the null check could be at the call site. Or maybe we can just call this a pure bug fix and say that the old behavior was a bug.

I think that calling this a pure bug fix is the right way to go.

>> Source/WebCore/rendering/TextPainter.cpp:86
>> +        }
> 
> Should put this before the definitions of extraOffset and didSaveContext.

Done.

>> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:598
>> +        bool didSaveContext = false;
> 
> You forgot to change the code below to use this boolean! It still says “if (shadow->next())” but should say “if (didSaveContext)”.

Done.

[Myles C. Maxfield]

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

(In reply to comment #5)
> http://trac.webkit.org/changeset/173418

This change made things worse! Now a single shadow with zero offset and blur makes text vanish entirely:
<div style="color: green; text-shadow: blue 0 0">text</div>

(In reply to comment #6)
> (In reply to comment #5)
> > http://trac.webkit.org/changeset/173418
> 
> This change made things worse! Now a single shadow with zero offset and blur makes text vanish entirely:
> <div style="color: green; text-shadow: blue 0 0">text</div>

Reported that separately as bug 136801. It’s incredible that there isn’t a single regression test for this.

[Darin Adler]

Wow, really embarrassing.

Myles, I won’t be able to think about anything other than this bug if we play Destiny now!

[Myles C. Maxfield]

On it.