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>
Created attachment 237805 [details] Patch
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)”.
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.
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.
Wow, really embarrassing. Myles, I won’t be able to think about anything other than this bug if we play Destiny now!
On it.