RESOLVED FIXED 67543
Text rendered with a simple (i.e. 0px blur) shadow inside a transparency layer has a double shadow 
https://bugs.webkit.org/show_bug.cgi?id=67543
Summary Text rendered with a simple (i.e. 0px blur) shadow inside a transparency laye...
Tim Horton
Reported 2011-09-02 17:31:14 PDT
Splitting this off from 65643, since it's totally unrelated to that bug (and was just exposed by a bug in the patch): > If we set the shadow on a group, a transparency layer is created, and the shadow is cleared (because it's applied to the group, not the children). Then, when we go to draw the text shadow, if it has no blur (and can be rendered by simply redrawing the text), we a) store the shadow b) draw the text by hand in the shadow color c) restore the shadow. But, if we're in a transparency layer, the shadow is not clearable! > > So what happens in effect is that we draw the text shadow into the transparency layer, then when it's composited onto the screen, the group's shadow is applied, leading to the *shadow* having its own shadow! > >So, we can't use simple shadows inside a transparency layer. I assume the behavior is the same on other platforms (with the shadow clearing and all) because that makes the most sense. I think perhaps we should make the increment/decrement/inTransparencyLayer() stuff enabled on all platforms. > >I might be able to concoct a test for this that still exhibits after we fix the overdraw, but for right now it's made blindingly obvious in a variety of other tests because of that flaw in this patch.
Attachments
testcase (240 bytes, image/svg+xml)
2011-09-02 17:36 PDT, Tim Horton 		no flags
Details
initial patch (38.63 KB, patch)
2011-09-02 18:06 PDT, Tim Horton 		webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
fix windows build (41.39 KB, patch)
2011-09-06 13:43 PDT, Tim Horton 		no flags
DetailsFormatted DiffDiff
Fix crashes on Windows and Gtk (42.66 KB, patch)
2011-09-09 12:20 PDT, Tim Horton 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2011-09-02 17:31:39 PDT
<rdar://problem/10070536>
Tim Horton
Comment 2 2011-09-02 17:36:03 PDT
Created attachment 106229 [details] testcase
Tim Horton
Comment 3 2011-09-02 18:06:58 PDT
Created attachment 106233 [details] initial patch
WebKit Review Bot
Comment 4 2011-09-02 20:42:29 PDT
Comment on attachment 106233 [details] initial patch Attachment 106233 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9583831 New failing tests: svg/custom/simple-text-double-shadow.svg
Darin Adler
Comment 5 2011-09-03 08:49:04 PDT
Windows build failure: 3>..\platform\win\ScrollbarThemeWin.cpp(274) : error C2039: 'inTransparencyLayer' : is not a member of 'WebCore::GraphicsContext' 3>..\platform\win\ScrollbarThemeWin.cpp(328) : error C2039: 'inTransparencyLayer' : is not a member of 'WebCore::GraphicsContext' 3>..\platform\win\ScrollbarThemeWin.cpp(387) : error C2039: 'inTransparencyLayer' : is not a member of 'WebCore::GraphicsContext' 3>c:\cygwin\home\buildbot\webkit\source\webcore\rendering\RenderThemeWin.cpp(677) : error C2039: 'inTransparencyLayer' : is not a member of 'WebCore::GraphicsContext' 3>..\plugins\win\PluginViewWin.cpp(639) : error C2039: 'inTransparencyLayer' : is not a member of 'WebCore::GraphicsContext'
Tim Horton
Comment 6 2011-09-06 13:43:04 PDT
Created attachment 106477 [details] fix windows build
WebKit Review Bot
Comment 7 2011-09-06 20:05:20 PDT
Comment on attachment 106477 [details] fix windows build Attachment 106477 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9601403 New failing tests: svg/custom/simple-text-double-shadow.svg
Tim Horton
Comment 8 2011-09-07 14:34:34 PDT
Landed in 94714; now for grabbing new baselines and checking for build failures!
Ryosuke Niwa
Comment 9 2011-09-07 16:19:36 PDT
It seems like this patch caused a whole bunch of tests to crash on GTK: http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r94717%20(25820)/results.html
Ryosuke Niwa
Comment 10 2011-09-07 16:21:19 PDT
(In reply to comment #9) > It seems like this patch caused a whole bunch of tests to crash on GTK: > http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r94717%20(25820)/results.html On second thought, this might be an issue with the bot. Other GTK+ bots are fine even after your changeset.
Ryosuke Niwa
Comment 11 2011-09-07 16:39:35 PDT
(In reply to comment #10) > (In reply to comment #9) > > It seems like this patch caused a whole bunch of tests to crash on GTK: > > http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r94717%20(25820)/results.html > > On second thought, this might be an issue with the bot. Other GTK+ bots are fine even after your changeset. Nope. The same thing started to happen on Windows XP (Debug): http://build.webkit.org/results/Windows%20XP%20Debug%20(Tests)/r94714%20(31934)/results.html
Tim Horton
Comment 12 2011-09-07 16:51:03 PDT
Reopening due to rollout.
Tim Horton
Comment 13 2011-09-08 10:37:29 PDT
Quick debugging on gtk-linux shows that m_transparencyCount rolls over to 32768. Not sure why; it's only manipulated in one place which has an assert (which isn't being hit) to guard against this (also, stepping through and printing m_transparencyCount shows that it's not a problem there, it becomes garbage in between). I'll debug more carefully tonight, since it's less clear what's going on now... Also, I found an extra m_transparencyCount(0) on GraphicsContextPlatformPrivateCairo which would break gtk-windows build, so I'll remove that too. (that's certainly not causing this problem, though).
Tim Horton
Comment 14 2011-09-09 12:20:43 PDT
Created attachment 106901 [details] Fix crashes on Windows and Gtk Turns out WindowsCG, WindowsCairo and Cairo overload the GraphicsContext constructor, so I just needed to add my initialization there (showed up instantly in valgrind).
Darin Adler
Comment 15 2011-09-09 18:47:22 PDT
Comment on attachment 106901 [details] Fix crashes on Windows and Gtk review+ on the fix; Simon is still the reviewer on the change itself
WebKit Review Bot
Comment 16 2011-09-09 19:55:36 PDT
Comment on attachment 106901 [details] Fix crashes on Windows and Gtk Clearing flags on attachment: 106901 Committed r94897: <http://trac.webkit.org/changeset/94897>
WebKit Review Bot
Comment 17 2011-09-09 19:55:42 PDT
All reviewed patches have been landed. Closing bug.
Nikolas Zimmermann
Comment 18 2012-02-17 03:19:10 PST
This patch caused Qt bug 78332, uncovered by my repaint.js refactoring. See my comment there: "Hmpf, the problem is the m_transparencyCount generalization. Qt maintains a different count "layerCount" returned by isInTransparentLayer(), and it calls endTransparencyLayer too often, compared to all other ports - to support image clipping. Cairo has similar needs (mask image operation during restore()) but doesn't suffer from the problem - can this be reused instead? I hope Zoltan can have a look." The Qt code uses for (i=0; i < layerCount) endTransparencyLayer(). Layer count may be higher than m_transparencyCount, leading to the assertion that m_transparenyCount should always be > 0. Why is that? Qt doesn't always perform --m_layerCount, as endTransparencyLayer() does for m_transparencyCount. The whole code is only needed to properly support ImageBuffer::clip() - Zoltan Herczeg is currently working on eliminating that, replacing by composite operations. Once that's done the custom layer count stuff could be removed from Qt again, fixing the assertion. That's probably the easiest - the current code from Tim makes a lot of sense! (Note: Cairo/Gtk don't suffer from this problem, they don't maintain a separated 'layer count', but rely on m_transparencyCount)
Eric Seidel (no email)
Comment 19 2012-03-27 12:51:19 PDT
Comment on attachment 106477 [details] fix windows build Cleared Simon Fraser's review+ from obsolete attachment 106477 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Tim Horton
Comment 20 2012-11-26 22:58:54 PST
Niko, is there a reason you reopened this? The fix is still in, and bug 78332 is mysteriously fixed. I'm going to re-close it, feel free to let me know/reopen again if there was a reason.
Note You need to log in before you can comment on or make changes to this bug.