Last Comment Bug 692744 - Shadow rendered around space character using text-shadow
: Shadow rendered around space character using text-shadow
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla10
Assigned To: Jonathan Kew (:jfkthame)
:
:
Mentors:
: 690331 698155 (view as bug list)
Depends on:
Blocks: 564444
  Show dependency treegraph
 
Reported: 2011-10-07 02:45 PDT by Darren Kalck [:D-Kalck]
Modified: 2011-10-29 00:00 PDT (History)
10 users (show)
jfkthame: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
text-shadow-bug.png (11.17 KB, image/png)
2011-10-07 02:45 PDT, Darren Kalck [:D-Kalck]
no flags Details
Shadow bug testcase (194 bytes, text/html)
2011-10-07 03:09 PDT, Darren Kalck [:D-Kalck]
no flags Details
patch, don't create a 1x1 image surface when it should really be empty (1.07 KB, patch)
2011-10-10 06:26 PDT, Jonathan Kew (:jfkthame)
jmuizelaar: review+
Details | Diff | Splinter Review
reftest (1.77 KB, patch)
2011-10-10 07:04 PDT, Jonathan Kew (:jfkthame)
jmuizelaar: review+
Details | Diff | Splinter Review

Description Darren Kalck [:D-Kalck] 2011-10-07 02:45:35 PDT
Created attachment 565471 [details]
text-shadow-bug.png

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111002 Firefox/10.0a1
Build ID: 20111002040206

Steps to reproduce:

When using text-shadow property on a text with space character inside.


Actual results:

The shadow is rendered around the space character


Expected results:

The shadow shouldn't be rendered
Comment 1 j.j. 2011-10-07 02:50:42 PDT
Please attach a simple html test file with tis issue.
Comment 2 Darren Kalck [:D-Kalck] 2011-10-07 03:09:37 PDT
Created attachment 565481 [details]
Shadow bug testcase

There's small red dots when there's a space/tab character
Comment 3 Jonathan Kew (:jfkthame) 2011-10-07 03:20:33 PDT
Yes, I can reproduce this. It seems to be related to DirectWrite; if I disable HW acceleration (in Options), the issue goes away, but then it reappears if I force DirectWrite font rendering (via about:config) even with HW acc disabled.
Comment 4 Jonathan Kew (:jfkthame) 2011-10-07 03:24:20 PDT
Note that the dot is dependent on the shadow including a blur; if I omit the blur value from the text-shadow, the dot does not appear. It's easiest to see with a blur of 1px, or very close to this; as the value gets larger or smaller it fairly quickly fades away to invisibility.
Comment 5 Jonathan Kew (:jfkthame) 2011-10-07 03:38:33 PDT
This occurs in Firefox 7.0.1; I didn't see it in FF5 on the same system. Apparently a regression somewhere between those releases.
Comment 6 Jonathan Kew (:jfkthame) 2011-10-07 05:05:36 PDT
From testing m-c nightlies:

good: Gecko/20110527 Firefox/7.0a1
Built from http://hg.mozilla.org/mozilla-central/rev/0cf4fa02c0f2

bad: Gecko/20110528 Firefox/7.0a1
Built from http://hg.mozilla.org/mozilla-central/rev/1f25c65e9335

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0cf4fa02c0f2&tochange=1f25c65e9335

Possibly a result of the cairo-1.10 update?
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-07 14:04:04 PDT
Jonathan, can you work on this?
Comment 8 Jonathan Kew (:jfkthame) 2011-10-10 04:04:10 PDT
Ok, taking this, at least for further investigation. I've built specific changesets locally and confirmed that it begins with the cairo-1.10 update (http://hg.mozilla.org/mozilla-central/rev/102be3d1f103).
Comment 9 Jonathan Kew (:jfkthame) 2011-10-10 06:21:47 PDT
Turns out this is related to bug 564444, which only showed up with DirectWrite fonts rendered to GDI, and so wasn't a high priority as that is not really a supported configuration.

Here, creating the shadow involves rendering the glyphs to a cairo_image_surface, and since the cairo 1.10 update that hits the same problematic code path via cairo_image_surface_glyphs, which leads to getting the glyph image as a surface with cairo_dwrite_scaled_font_init_glyph_surface. (Prior to the cairo 1.10 update, the cairo_image_surface_glyphs function wasn't present, and so a different fallback code path was used when rendering to the image surface.)

The root of the problem is in _cairo_image_surface_create_with_pixman_format, which forces the width and height of the image that's created to be 1 pixel if the values were originally zero. This means that for the space glyph, which typically has a (0,0,0,0) bounding box (although a non-zero advance), we end up with the glyph info having surface that is a single opaque black pixel, rather than the expected empty surface. Although this pixel lies outside the glyph's clip, and so doesn't actually get rendered to the D2D target in the usual case (although it was a problem when rendering to GDI), it ends up contributing to the blur effect used by the shadow - hence this bug.

I don't see any real need to force non-zero width and height in _cairo_image_surface_create_with_pixman_format, as the pixman code that gets called looks like it'll handle this ok. So I think the simplest and cleanest fix here is to remove the special-casing of zero width/height in this function. Then we get - as expected - an empty/zero-sized surface for the glyph, and no blur/shadow results.
Comment 10 Jonathan Kew (:jfkthame) 2011-10-10 06:26:29 PDT
Created attachment 565921 [details] [diff] [review]
patch, don't create a 1x1 image surface when it should really be empty

This removes the special-case for width/height zero, so that we correctly get an empty surface in this situation. Jeff, do you see any potential problem with this? Is it something we should try to get fixed in upstream cairo?

Pushed this to tryserver, and it doesn't appear to break any unit tests there. It also fixes bug 564444, at least in my local testing.

We should be able to reftest this; I'll put together a testcase as a separate patch.
Comment 11 Jonathan Kew (:jfkthame) 2011-10-10 07:04:45 PDT
Created attachment 565930 [details] [diff] [review]
reftest

Reftest - the testcase has a bunch of trailing  s, which should _not_ produce any extra dots of text-shadow.
Comment 12 Jeff Muizelaar [:jrmuizel] 2011-10-12 12:21:11 PDT
Comment on attachment 565921 [details] [diff] [review]
patch, don't create a 1x1 image surface when it should really be empty

So this isn't upstream. I added it a long time ago, for reasons that I never wrote down. I'd be happy to try taking it out, but we should watch for crashes.
Comment 13 Jonathan Kew (:jfkthame) 2011-10-12 14:33:28 PDT
OK, sounds good. I tried to look for possible risks related to allowing zero here, but AFAICS it should be safe. (Maybe an older version of pixman_image_create_bits didn't handle zero well, but has since been made more robust?)

Pushed to -inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/328469ba44df (patch)
https://hg.mozilla.org/integration/mozilla-inbound/rev/bda77d81acbb (reftest)
Comment 15 j.j. 2011-10-13 13:12:05 PDT
*** Bug 690331 has been marked as a duplicate of this bug. ***
Comment 16 Jonathan Kew (:jfkthame) 2011-10-29 00:00:31 PDT
*** Bug 698155 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.