Bug 24584 – Translucent text rendering on Chromium is broken

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Stephen White]

Translucent text rendering in Chromium renders text as opaque in some cases.

See:

LayoutTests/svg/text/text-text-08-b.svg
LayoutTests/fast/css/hsla-color.html

[Stephen White]

Created attachment 28591 [details]
Fix for TransparencyWin

FontChromiumWin was calling beginTransparencyLayer()/endTransparencyLayer(), with a TransparencyWin inside to do GDI ClearType rendering over an opaque background.  TransparencyWin does its special sauce in the destructor, but it was being called too late to be used correctly in the layer.  Put into uninit() instead, and call that explicitly.

[Eric Seidel (no email)]

Comment on attachment 28591 [details]
Fix for TransparencyWin

Why not just call it what it does?  "compositeToParentLayer()" or "restoreContextAndCompositeToParentLayer()"

uninit seems like a really bad name for the function.

[Stephen White]

(In reply to comment #2)
> (From update of attachment 28591 [details] [review])
> Why not just call it what it does?  "compositeToParentLayer()" or
> "restoreContextAndCompositeToParentLayer()"
> 
> uninit seems like a really bad name for the function.

I wanted to stress that it should pair up with init() (and that calling it is mandatory if you call init()).

It's not compositing "to" anything.  It's compositing over the background using ClearType, but even that is misleading, since it will get composited again over the background using text alpha in the endTransparencyLayer() call.

Mind if we just call it "composite()"?

[Eric Seidel (no email)]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 28591 [details] [review] [review])
> > Why not just call it what it does?  "compositeToParentLayer()" or
> > "restoreContextAndCompositeToParentLayer()"
> > 
> > uninit seems like a really bad name for the function.
> 
> I wanted to stress that it should pair up with init() (and that calling it is
> mandatory if you call init()).

I regretted being so disparaging in my original comment almost as soon as I hit "commit".  :(

> It's not compositing "to" anything.  It's compositing over the background using
> ClearType, but even that is misleading, since it will get composited again over
> the background using text alpha in the endTransparencyLayer() call.
> 
> Mind if we just call it "composite()"?

composite() is OK.  It's not particularly specific (you'd still have to read the composite() function to guess at what it did).  It seems it would also make sense to ASSERT if possible that we'd already composited.  We should probably be clearing m_savedOnDrawContext after restore now that this is a free function.  composite() (or whatever you call it) should also be a private function, since no one should call it except for the destructor.

[Stephen White]

(In reply to comment #4)
> composite() is OK.  It's not particularly specific (you'd still have to read
> the composite() function to guess at what it did).  It seems it would also make
> sense to ASSERT if possible that we'd already composited.  We should probably
> be clearing m_savedOnDrawContext after restore now that this is a free
> function.  composite() (or whatever you call it) should also be a private
> function, since no one should call it except for the destructor.

Actually, no.  This work isn't done in the destructor any more, since that was what was causing the problem (it's too late).  It's called explicitly by FontChromiumWin, so it needs to be public.

[Stephen White]

Created attachment 28594 [details]
Fix for TransparencyWin (2)

Changed uninit() to composite().  Reset m_savedOnDrawContext after restoring context.  Asserted that it was reset in destructor.

[Eric Seidel (no email)]

Comment on attachment 28594 [details]
Fix for TransparencyWin (2)

Looks fine.

You can remove WARNING: NO TEST CASES ADDED OR CHANGED :)

Also, you should normally have the bug URL in teh ChangeLog.

[Stephen White]

Created attachment 28603 [details]
Fix for TransparencyWin (3)

Turns out I broke another layout test with this fix (LayoutTests/fast/forms/indeterminate.html), now fixed.

Modified ChangeLog as requested.

[Eric Seidel (no email)]

Comment on attachment 28603 [details]
Fix for TransparencyWin (3)

Seems that we could catch this kind of error with an !NDEBUG bool.

But this is OK.

[Brett Wilson (Google)]

Comment on attachment 28603 [details]
Fix for TransparencyWin (3)

> Index: WebCore/platform/graphics/chromium/TransparencyWin.h
> ===================================================================
> --- WebCore/platform/graphics/chromium/TransparencyWin.h	(revision 41685)
> +++ WebCore/platform/graphics/chromium/TransparencyWin.h	(working copy)
> @@ -134,6 +134,9 @@ public:
>                TransformMode transformMode,
>                const IntRect& region);
>  
> +    // Combines the source and destination bitmaps using the given mode.
> +    void composite();

You should comment on the conditions where this is function is required. Which layer modes is it required for? Also, the comments for those layer modes should say that calling composite before destruction is required.

Second, now that this is required, it needs to be called in the other places that TransparencyWin is used. This is RenderThemeChromiumWin as well as webkit/tools/webcore_unit_tests/TransparencyWin_unittest.cpp in Chrome. I think that it will assert in both places otherwise.

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/chromium/FontChromiumWin.cpp
	M	WebCore/platform/graphics/chromium/TransparencyWin.cpp
	M	WebCore/platform/graphics/chromium/TransparencyWin.h
	M	WebCore/rendering/RenderThemeChromiumWin.cpp
Committed r41692

[Eric Seidel (no email)]

(In reply to comment #10)
> (From update of attachment 28603 [details] [review]) 
> You should comment on the conditions where this is function is required. Which
> layer modes is it required for? Also, the comments for those layer modes should
> say that calling composite before destruction is required.
> 
> Second, now that this is required, it needs to be called in the other places
> that TransparencyWin is used. This is RenderThemeChromiumWin as well as
> webkit/tools/webcore_unit_tests/TransparencyWin_unittest.cpp in Chrome. I think
> that it will assert in both places otherwise.

Sigh.  I'll re-open and you can fix this in a second patch which you can have Brett review.

[Stephen White]

(In reply to comment #12)
> (In reply to comment #10)
> > (From update of attachment 28603 [details] [review] [review]) 
> > You should comment on the conditions where this is function is required. Which
> > layer modes is it required for? Also, the comments for those layer modes should
> > say that calling composite before destruction is required.
> > 
> > Second, now that this is required, it needs to be called in the other places
> > that TransparencyWin is used. This is RenderThemeChromiumWin as well as
> > webkit/tools/webcore_unit_tests/TransparencyWin_unittest.cpp in Chrome. I think
> > that it will assert in both places otherwise.
> 
> Sigh.  I'll re-open and you can fix this in a second patch which you can have
> Brett review.
> 

Thanks.  The RenderThemeChromiumWin change is already in there, and the unit tests Brett mentioned are in our webkit (not third_party), so the only thing that remains to do on the webkit.org side are comments, which I'll take care of on Monday.

[Brett Wilson (Google)]

(In reply to comment #13)
> Thanks.  The RenderThemeChromiumWin change is already in there

Whoops, I totally missed this! You should probably check this patch into Chromium's repo with the unit test change at the same time to avoid giving the merger a headache. Let me know if you have any questions.

[David Levin]

Comment on attachment 28594 [details]
Fix for TransparencyWin (2)

Clearing r+ on obsolete changes to move this out of the commit queue.

[David Levin]

Comment on attachment 28603 [details]
Fix for TransparencyWin (3)

Clearing r+ on obsolete patches to move this out of the commit queue.

[Stephen White]

Code comments added in http://bugs.webkit.org/show_bug.cgi?id=26320.

[Eric Seidel (no email)]

I'm surprised there was no layout test for this?

[Stephen White]

(In reply to comment #18)
> I'm surprised there was no layout test for this?
> 

See above:

LayoutTests/svg/text/text-text-08-b.svg
LayoutTests/fast/css/hsla-color.html

(I think acid3 was broken too).  The patch attached to this bug was committed long ago, I just promised Brett I'd add some comments, which I finally got around to doing in 26320.