78.61 KB, image/png
1.32 KB, text/html
7.35 KB, patch
Joe Drew (not getting mail): review+
|Details | Diff | Splinter Review|
528 bytes, patch
|Details | Diff | Splinter Review|
Created attachment 369057 [details] screenshot of testcase text-shadow: 1px 2px 0 rgba(250,250,250, 0.65); replace rgba() with rgb() and it works fine. if the blur value is larger than 1px, it displays fine. This seems limited to Windows 7. I'm not sure about the regression range. However, I'm sure I was doing tests on this with the 20090319 build, where it worked fine. Possibly the Cairo/Pixman upgrade ? (bug 484076) Displays correctly on OS X and Linux, btw.
Created attachment 369060 [details] correct test case
Same issue on Windows XP. Regression range: Works: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090320 Minefield/3.6a1pre Broken Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090321 Minefield/3.6a1pre Changesets: http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-03-20+04%3A50%3A46&enddate=2009-03-21+05%3A05%3A53
slightly better range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e1120120b1b7&tochange=d14de45019a0
Jeff, this might be a regression from the Cairo upgrade. Can you check?
I can reproduce this on vista, which makes it more likely to have been caused by the cairo update.
It looks like any text with an rgba color is broken on win32.
This seems to be caused by the source image wrapping.
It is. It turns out we don't propagate the component_alpha flag and so the surface gets treated as regular argb32 surface and the alpha values are used instead of the component alpha values.
Created attachment 376074 [details] [diff] [review] Propogate the component alpha flag when wrapping image surfaces Not setting the component alpha flag on the pixman image was causing us to misinterpret the pixman image mask. The patch doesn't do much more then pixman_image_set_composite_alpha(pixman_image_get_composite_alpha()) The rest of the diff is due to keeping around a cairo_image_surface_t instead of a cairo_surface_t to avoid a lot of casting. I'll update the associated wrap-source.patch before this gets committed.
Comment on attachment 376074 [details] [diff] [review] Propogate the component alpha flag when wrapping image surfaces Looks good to me
Created attachment 376280 [details] [diff] [review] v2 with the correct patches against upstream [Checkin: Comment 15]
Comment on attachment 376280 [details] [diff] [review] v2 with the correct patches against upstream [Checkin: Comment 15] This just adds the changes to our explicit patches against cairo.
http://hg.mozilla.org/mozilla-central/rev/89338a224278 I committed the patch on Thursday, but forgot to mark this bug as fixed.
This is not yet fixed, or regressed again. The attached test case still shows the ugly spread when using rgba() text shadow or rgba() for the color property on Windows 7 and Vista with Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090512 Minefield/3.6a1pre (.NET CLR 3.5.30729)
Did the patch have an observable effect on the testcase? Unless the patch needs backing out (Jeff?), you should prefer filing a new bug over REOPENing a FIXED one.
(In reply to comment #17) > Did the patch have an observable effect on the testcase? I don't think so. I fetched the first official nightly after that patch landed: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090508 Minefield/3.6a1pre (.NET CLR 3.5.30729) And the test case, along with some others I have on my dev server, fails. > > Unless the patch needs backing out (Jeff?), you should prefer filing a new bug > over REOPENing a FIXED one. Well, apparently, the patch as landed didn't fix the original issue (rba() text-shadow) as filed. Nor did it fix the problem with rgba() color mentioned in comment 8. ---- per comments in the forums, the issue can be seen on all Windows versions (XP, Vista, Windows 7 RC).
Yeah, I screwed up here. I'll see what's going on...
Created attachment 377166 [details] [diff] [review] Fix a typo The patch that was committed has a typo. It was getting the component_alpha flag from destination surface instead of the source surface.
Can we add a reftest or a mochitest for this somehow?
http://hg.mozilla.org/mozilla-central/rev/dd19a3546676 The reftest is bug 493067.
(In reply to comment #22) > http://hg.mozilla.org/mozilla-central/rev/dd19a3546676 > > The reftest is bug 493067. Works nicely now. Cool. Shall I mark this as FIXED ? Or is there something more to do here ? Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090515 Minefield/3.6a1pre (.NET CLR 3.5.30729)
Fixed it is!