Bug 67543 - Text rendered with a simple (i.e. 0px blur) shadow inside a transparency layer has a double shadow
Summary: Text rendered with a simple (i.e. 0px blur) shadow inside a transparency laye...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on: 67746
Blocks: 78332
  Show dependency treegraph
 
Reported: 2011-09-02 17:31 PDT by Tim Horton
Modified: 2012-11-26 22:58 PST (History)
7 users (show)

See Also:


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-
Details | Formatted Diff | Diff
fix windows build (41.39 KB, patch)
2011-09-06 13:43 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Fix crashes on Windows and Gtk (42.66 KB, patch)
2011-09-09 12:20 PDT, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 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.
Comment 1 Radar WebKit Bug Importer 2011-09-02 17:31:39 PDT
<rdar://problem/10070536>
Comment 2 Tim Horton 2011-09-02 17:36:03 PDT
Created attachment 106229 [details]
testcase
Comment 3 Tim Horton 2011-09-02 18:06:58 PDT
Created attachment 106233 [details]
initial patch
Comment 4 WebKit Review Bot 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
Comment 5 Darin Adler 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'
Comment 6 Tim Horton 2011-09-06 13:43:04 PDT
Created attachment 106477 [details]
fix windows build
Comment 7 WebKit Review Bot 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
Comment 8 Tim Horton 2011-09-07 14:34:34 PDT
Landed in 94714; now for grabbing new baselines and checking for build failures!
Comment 9 Ryosuke Niwa 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
Comment 10 Ryosuke Niwa 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.
Comment 11 Ryosuke Niwa 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
Comment 12 Tim Horton 2011-09-07 16:51:03 PDT
Reopening due to rollout.
Comment 13 Tim Horton 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).
Comment 14 Tim Horton 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).
Comment 15 Darin Adler 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
Comment 16 WebKit Review Bot 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>
Comment 17 WebKit Review Bot 2011-09-09 19:55:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Nikolas Zimmermann 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)
Comment 19 Eric Seidel (no email) 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.
Comment 20 Tim Horton 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.