Bug 67543

Summary:

Text rendered with a simple (i.e. 0px blur) shadow inside a transparency layer has a double shadow

Product:

WebKit

Reporter:

Tim Horton <thorton>

Component:

Layout and Rendering

Assignee:

Tim Horton <thorton>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, dglazkov, mrobinson, rniwa, webkit-bug-importer, webkit.review.bot, zimmermann

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

Unspecified

Bug Depends on:

67746

Bug Blocks:

78332

Attachments:

Description	Flags
testcase	none
initial patch	webkit.review.bot: commit-queue-
fix windows build	none
Fix crashes on Windows and Gtk	none

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-	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
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.