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

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
11 years ago
7 years ago

People

(Reporter: Graeme Humphries, Assigned: jwatt)

Tracking

({fixed1.8.1.4})

1.8 Branch
x86
Linux
fixed1.8.1.4
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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
(Reporter)

Comment 1

11 years ago
Created attachment 244217 [details]
Broken gradient redering in Firefox 2.0

Shows lack of padding in Firefox 2.0 rendering.
(Reporter)

Comment 2

11 years ago
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.
Assignee: nobody → general
Component: General → SVG
Product: Firefox → Core
QA Contact: general → ian
Version: unspecified → 1.8 Branch

Comment 3

11 years ago
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.
(Reporter)

Comment 4

11 years ago
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
Last Resolved: 11 years ago
Resolution: --- → INVALID

Comment 5

11 years ago
https://launchpad.net/distros/ubuntu/+source/firefox/+bug/69721

Comment 6

11 years ago
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.
(Assignee)

Comment 7

11 years ago
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

11 years ago
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?


(Assignee)

Comment 9

11 years ago
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

11 years ago
(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

11 years ago
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 → ---

Updated

11 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 12

11 years ago
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

11 years ago
i already have the patch here ... testing.

Comment 14

11 years ago
Created attachment 257027 [details] [diff] [review]
patch with comment

Comment 15

11 years ago
Created attachment 257028 [details] [diff] [review]
patch without comment (candidate)

Updated

11 years ago
Attachment #257028 - Attachment is patch: true
Attachment #257028 - Attachment mime type: text/x-patch → text/plain

Comment 16

11 years ago
for me it indeed fixes this issue.

Comment 17

11 years ago
... have not yet tried with 1.0 cairo though.
(Assignee)

Updated

11 years ago
Blocks: 375659
(Assignee)

Comment 18

11 years ago
Alexander, were you going to test 1.0?

Updated

11 years ago
Duplicate of this bug: 375849

Updated

11 years ago
Duplicate of this bug: 375659

Comment 21

11 years ago
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

11 years ago
Jonathan, renders correctly with patch applied and built against 1.0 system cairo here. Who can review svg?
Johnathan can, and tor can SR...
(Assignee)

Comment 24

11 years ago
Created attachment 260571 [details] [diff] [review]
patch
Attachment #257027 - Attachment is obsolete: true
Attachment #257028 - Attachment is obsolete: true
Attachment #260571 - Flags: superreview?(tor)
Attachment #260571 - Flags: review+

Comment 25

11 years ago
(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

Updated

11 years ago
Attachment #260571 - Flags: superreview?(tor) → superreview+
(Assignee)

Comment 26

11 years ago
Thanks for looking Carl.
(Assignee)

Updated

11 years ago
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
(Assignee)

Comment 28

10 years ago
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
Last Resolved: 11 years ago10 years ago
Resolution: --- → FIXED
Summary: 2.0 doesn't respect SVG gradient spreadMethod="pad" → Firefox 2.0 doesn't respect SVG gradient spreadMethod="pad"
(Assignee)

Updated

10 years ago
Keywords: fixed1.8.1.4

Updated

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