Last Comment Bug 358930 - Firefox 2.0 doesn't respect SVG gradient spreadMethod="pad"
: Firefox 2.0 doesn't respect SVG gradient spreadMethod="pad"
Status: RESOLVED FIXED
: fixed1.8.1.4
Product: Core
Classification: Components
Component: SVG (show other bugs)
: 1.8 Branch
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Jonathan Watt [:jwatt] (back in October - email directly if necessary)
: Hixie (not reading bugmail)
Mentors:
http://demoni.ca/svg/demonica.svg
: 375659 375849 (view as bug list)
Depends on:
Blocks: 375659
  Show dependency treegraph
 
Reported: 2006-10-31 11:17 PST by Graeme Humphries
Modified: 2010-09-17 13:37 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Broken gradient redering in Firefox 2.0 (34.60 KB, image/png)
2006-10-31 11:22 PST, Graeme Humphries
no flags Details
Correct gradient padding rendering by librsvg2. (17.10 KB, image/png)
2006-10-31 11:23 PST, Graeme Humphries
no flags Details
patch with comment (1.16 KB, patch)
2007-03-02 08:11 PST, Alexander Sack
no flags Details | Diff | Splinter Review
patch without comment (candidate) (870 bytes, patch)
2007-03-02 08:12 PST, Alexander Sack
no flags Details | Diff | Splinter Review
patch (1.61 KB, patch)
2007-04-04 02:42 PDT, Jonathan Watt [:jwatt] (back in October - email directly if necessary)
jwatt: review+
tor: superreview+
dveditz: approval1.8.1.4+
Details | Diff | Splinter Review

Description Graeme Humphries 2006-10-31 11:17:49 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1) Gecko/20060601 Firefox/2.0 (Ubuntu-edgy)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1) Gecko/20060601 Firefox/2.0 (Ubuntu-edgy)

New problem in 2.0, it looks like the SVG linear gradient rendering is ignoring spreadMethod="pad" from the SVG 1.1 spec (http://www.w3.org/TR/SVG11/pservers.html#LinearGradients). In the linked example, you can see that the bottom part of the letter shapes are transparent, instead of being padded with white (as Firefox 1.5 correctly did). It looks like start padding is working correctly just end padding is not.

Reproducible: Always

Steps to Reproduce:
1. Load SVG
2. View lack of end padding on fill gradient
Comment 1 Graeme Humphries 2006-10-31 11:22:50 PST
Created attachment 244217 [details]
Broken gradient redering in Firefox 2.0

Shows lack of padding in Firefox 2.0 rendering.
Comment 2 Graeme Humphries 2006-10-31 11:23:54 PST
Created attachment 244220 [details]
Correct gradient padding rendering by librsvg2.

Shows correct gradient padding rendering (but not correct placed image rendering) from librsvg2 on Linux.
Comment 3 tor 2006-10-31 14:05:42 PST
Works fine on my linux build here.  This sounds like a cairo version mismatch, and I see that Ubuntu uses the unsupported --enable-system-cairo build config option.

  https://launchpad.net/distros/ubuntu/+source/firefox/+bug/32561

Please test using the official linux build from mozilla.com.
Comment 4 Graeme Humphries 2006-11-01 09:30:44 PST
Yep, testing with official build shows no problems, it's a problem downstream. Sorry to waste your guys' time, I'll file a bug with Ubuntu.
Comment 6 Alexander Sack 2007-02-25 14:23:50 PST
So why has this been resolved as invalid? If --enable-system-cairo has a bug, then its worth an open bug. You might not suppor that, but you apparently maintain that, because it exists their. So, though this bug might be less severe its still a bug in code maintained by you.
Comment 7 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2007-02-25 16:28:46 PST
All --enable-system-cairo does is make Mozilla use the system cairo, and in that limited task it succeeds perfectly. However, the cairo binary installed on a system is of course outside Mozilla's control. Bugs in the version of cairo installed on the system are not bugs in Mozilla, and since the binary installed on a system is not (and should not be) controlled by Mozilla, --enable-system-cairo is naturally not something we can support for releases.

That aside, I'd have thought it a good idea for package maintainers to ensure their system version of cairo is the same (or at least as recent) as the version of cairo in the Mozilla tree before using such an option.
Comment 8 Alexander Sack 2007-02-26 05:18:55 PST
Jonathan, thanks clarifying that you think its a bug in our version of cairo and not that its due to the use of system cairo in general.

looking at cairo-features.h, you ship (from 1.8.1 branch):

#define CAIRO_VERSION_STRING "1.0.2"

We ship:

#define CAIRO_VERSION_STRING "1.0.2" (breezy)
#define CAIRO_VERSION_STRING "1.0.4" (dapper)
#define CAIRO_VERSION_STRING "1.2.4" (edgy - current stable)
#define CAIRO_VERSION_STRING "1.3.14" (feisty)

So the reporter has this issue with 1.2.4, which is more recent than cairo. Any chance that you might take a look if this is a regression in cairo, or if mozilla code base could be improved to support current cairo too?


Comment 9 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2007-02-26 09:17:03 PST
Tim points out that at some point between cairo 1.0.2 and 1.2.4 cairo's gradient API was changed. To get "pad" behavior in the former you had to use CAIRO_EXTEND_NONE.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/cairo/cairo/src/cairo.h&rev=1.3.4.1&mark=1293#1291

Later CAIRO_EXTEND_PAD was added and the meaning of CAIRO_EXTEND_NONE was changed to "pixels outside of the source pattern are fully transparent".

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/cairo/cairo/src/cairo.h&rev=1.23&mark=1575,1578#1558
Comment 10 Carl Worth 2007-03-02 00:54:28 PST
(In reply to comment #9)
> Tim points out that at some point between cairo 1.0.2 and 1.2.4 cairo's
> gradient API was changed. To get "pad" behavior in the former you had to use
> CAIRO_EXTEND_NONE.
...
> Later CAIRO_EXTEND_PAD was added and the meaning of CAIRO_EXTEND_NONE was
> changed to "pixels outside of the source pattern are fully transparent".
...

Yes, that's fairly accurate.

What didn't change is that the default extend mode for gradients has always been a "pad" behavior, (even though we did not have the PAD enum value in 1.0).

And we hoped that not many people would be bitten by making NONE actually live up to its name in 1.2 since in 1.0 there's no real benefit to explicitly calling cairo_pattern_set_extend (gradient_pattern, CAIRO_EXTEND_NONE) --- that call doesn't actually change anything.

So, writing code to get compatible PAD behavior should be as simple as not calling cairo_pattern_set_extend at all, (or, if you really want to call it for some reason, then yes it will require a version check and using NONE for 1.0 but PAD for 1.2 and on).

Sorry for this little gotcha in the cairo API. I hope you will find that there really aren't a lot of these as far as backwards compatibility is concerned.

-Carl
Comment 11 Alexander Sack 2007-03-02 05:56:50 PST
I reopen the bug for now, as imo we should definitly try to get something that works across cairo versions. And comments from Carl sound like this should be doable with little or no tradeoff.
Comment 12 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2007-03-02 06:59:25 PST
Yup, cool. Should be as simple as removing the two lines at:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/svg/renderer/src/cairo/nsSVGCairoGradient.cpp&rev=1.6.6.4&mark=182-183#179

I'll attach a patch later today. Would you be able to test it asac?
Comment 13 Alexander Sack 2007-03-02 08:07:05 PST
i already have the patch here ... testing.
Comment 14 Alexander Sack 2007-03-02 08:11:42 PST
Created attachment 257027 [details] [diff] [review]
patch with comment
Comment 15 Alexander Sack 2007-03-02 08:12:53 PST
Created attachment 257028 [details] [diff] [review]
patch without comment (candidate)
Comment 16 Alexander Sack 2007-03-02 08:14:36 PST
for me it indeed fixes this issue.
Comment 17 Alexander Sack 2007-03-02 08:15:21 PST
... have not yet tried with 1.0 cairo though.
Comment 18 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2007-03-28 03:49:34 PDT
Alexander, were you going to test 1.0?
Comment 19 tor 2007-03-29 09:08:45 PDT
*** Bug 375849 has been marked as a duplicate of this bug. ***
Comment 20 tor 2007-03-29 09:08:52 PDT
*** Bug 375659 has been marked as a duplicate of this bug. ***
Comment 21 FReeZ 2007-03-29 22:12:18 PDT
I've applied the patch 257028 into FF 2.0.0.3 (my cairo version is 1.2.6) and now SVG images are finally rendereder fine. Good job guys =)
Comment 22 Alexander Sack 2007-04-02 09:32:32 PDT
Jonathan, renders correctly with patch applied and built against 1.0 system cairo here. Who can review svg?
Comment 23 Mike Connor [:mconnor] 2007-04-03 20:57:51 PDT
Johnathan can, and tor can SR...
Comment 24 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2007-04-04 02:42:52 PDT
Created attachment 260571 [details] [diff] [review]
patch
Comment 25 Carl Worth 2007-04-04 10:41:15 PDT
(In reply to comment #24)
> Created an attachment (id=260571) [details]
> patch

From a cairo point-of-view, the patch and the description look good to me.

-Carl
Comment 26 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2007-04-04 10:53:25 PDT
Thanks for looking Carl.
Comment 27 Daniel Veditz [:dveditz] 2007-04-09 18:00:51 PDT
Comment on attachment 260571 [details] [diff] [review]
patch

approved for 1.8.1.4, a=dveditz for release-drivers
Comment 28 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2007-04-12 12:19:23 PDT
Checking in mozilla/layout/svg/renderer/src/cairo/nsSVGCairoGradient.cpp;
/cvsroot/mozilla/layout/svg/renderer/src/cairo/Attic/nsSVGCairoGradient.cpp,v  <--  nsSVGCairoGradient.cpp
new revision: 1.6.6.5; previous revision: 1.6.6.4
done

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