| Summary: | REGRESSION (r172153): Text drawn with wrong color when second text shadow has zero offset and blur (breaks buttons at aws.amazon.com) | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | mitz | ||||
| Component: | Layout and Rendering | Assignee: | Myles C. Maxfield <mmaxfield> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | commit-queue, darin, d-r, esprehn+autocc, fmalita, glenn, gyuyoung.kim, kondapallykalyan, mmaxfield, pdr, schenney, sergio | ||||
| Priority: | P1 | Keywords: | InRadar, Regression | ||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| URL: | data:text/html,<div%20style="color:%20green;%20text-shadow:%20yellow%2010px%2010px,%20red%200%200;">This%20should%20be%20green</div> | ||||||
| Attachments: |
|
||||||
|
Description
mitz
2014-09-07 12:46:56 PDT
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. (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. |