Closed Bug 358930 Opened 14 years ago Closed 14 years ago

Firefox 2.0 doesn't respect SVG gradient spreadMethod="pad"

Categories

(Core :: SVG, defect)

1.8 Branch
x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: unit3, Assigned: jwatt)

References

()

Details

(Keywords: fixed1.8.1.4)

Attachments

(3 files, 2 obsolete files)

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
Shows lack of padding in Firefox 2.0 rendering.
Shows correct gradient padding rendering (but not correct placed image rendering) from librsvg2 on Linux.
Assignee: nobody → general
Component: General → SVG
Product: Firefox → Core
QA Contact: general → ian
Version: unspecified → 1.8 Branch
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.
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.
Status: UNCONFIRMED → RESOLVED
Closed: 14 years ago
Resolution: --- → INVALID
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.
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.
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?


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
(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
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.
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
Status: UNCONFIRMED → NEW
Ever confirmed: true
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?
i already have the patch here ... testing.
Attached patch patch with comment (obsolete) — Splinter Review
Attachment #257028 - Attachment is patch: true
Attachment #257028 - Attachment mime type: text/x-patch → text/plain
for me it indeed fixes this issue.
... have not yet tried with 1.0 cairo though.
Blocks: 375659
Alexander, were you going to test 1.0?
Duplicate of this bug: 375849
Duplicate of this bug: 375659
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 =)
Jonathan, renders correctly with patch applied and built against 1.0 system cairo here. Who can review svg?
Johnathan can, and tor can SR...
Attached patch patchSplinter Review
Attachment #257027 - Attachment is obsolete: true
Attachment #257028 - Attachment is obsolete: true
Attachment #260571 - Flags: superreview?(tor)
Attachment #260571 - Flags: review+
(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
Attachment #260571 - Flags: superreview?(tor) → superreview+
Thanks for looking Carl.
Attachment #260571 - Flags: approval1.8.1.4?
Comment on attachment 260571 [details] [diff] [review]
patch

approved for 1.8.1.4, a=dveditz for release-drivers
Attachment #260571 - Flags: approval1.8.1.4? → approval1.8.1.4+
Assignee: general → jwatt
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
Status: NEW → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Summary: 2.0 doesn't respect SVG gradient spreadMethod="pad" → Firefox 2.0 doesn't respect SVG gradient spreadMethod="pad"
Keywords: fixed1.8.1.4
You need to log in before you can comment on or make changes to this bug.