448906 - SVG objects with patterned fills in files saved by some versions of Inkscape show incorrect patterns in Firefox 3.0.1

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Will Pittenger]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: 

Examine the fill for the checkered pattern in the start/finish line of the page given.  It should be a checker board with white and black squares alternating.  For me, it doesn't show that.  Instead, I see the attached images.  The first shows what I see.  The second shows what should be visible.  The third is the problem SVG with everything except that object deleted.

Reproducible: Always

Steps to Reproduce:
1. View the SVG
Actual Results:  
Pattern won't be displayed correctly

Expected Results:  
Checker board pattern to display as alternating white and black squares

Comment 1

[Will Pittenger]

Attachment: This shows what I see when I use FF 3.0.1 to view the SVG.

This shows what I see when I use FF 3.0.1 to view the SVG.

Comment 2

[Will Pittenger]

Attachment: What the pattern should look like

Comment 3

[Will Pittenger]

Attachment: Simplified SVG showing just the problem object

Comment 4

[Takeshi Kurosawa :takenspc]

Attachment: Testcase 1 (broken)

Even more reduced testcase.

Referencing a transformed pattern from a transformed element via |xlink:href| causes the problem.

<rect/> -> <pattern
             @xlink:href -> <pattern/>
           />

Comment 5

[Takeshi Kurosawa :takenspc]

Attachment: Testcase 2 (works)

Even more reduced testcase.

Referencing a transformed pattern from a transformed element directly doesn't causes the problem.

<rect/> -> <pattern/>

Comment 6

[Takeshi Kurosawa :takenspc]

Attachment: Testcase 1 + 2 (broken)

Mysteriously, rendering pattern referenced directly is failed when |xlink:href|ed pattern was rendered before.


<rect/> -> <pattern
             @xlink:href -> <pattern/>
           />
<rect/> -> <pattern/>

both are wrong rendering.

Comment 7

[Takeshi Kurosawa :takenspc]

Attachment: Testcase 2 + 1 (works)

Rendering |xlink:href|ed pattern is when pattern referenced directly was rendered before.


<rect/> -> <pattern/>
<rect/> -> <pattern
             @xlink:href -> <pattern/>
           />

both are O.K.

Comment 8

[Robert Longson [:longsonr]]

Attachment: patch

Comment 9

[Robert Longson [:longsonr]]

Thanks for the testcases Takeshi. I would not have been able to fix this without them.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

   if (!mPaintLoopFlag) {
     mPaintLoopFlag = PR_TRUE;

Should we be setting/checking mPaintLoopFlag on patternFrame?

Should we just be calling nsSVGPatternFrame::PaintPattern on patternFrame instead?

Comment 11

[Robert Longson [:longsonr]]

(In reply to comment #10)
>    if (!mPaintLoopFlag) {
>      mPaintLoopFlag = PR_TRUE;
> 
> Should we be setting/checking mPaintLoopFlag on patternFrame?

I can change this as it's the only member variable that paintPattern sets and I guess it does make more sense to set it on the pattern you're rendering. However, I don't think it makes much difference. If there is a loop then either you come back round to the current frame or the linked frame and you'll see the flag set at some point either on the current frame or the linked frame. 

> 
> Should we just be calling nsSVGPatternFrame::PaintPattern on patternFrame
> instead?

This we can't do. We need to start from the current frame not the linked frame to get pattern attributes. e.g. with pattern x="3" xlink:href="..." we need to get x="3"

Comment 12

[Robert Longson [:longsonr]]

Attachment: patternFrame->mPaintLoop

Comment 13

[Robert Longson [:longsonr]]

pushed http://hg.mozilla.org/mozilla-central/rev/4d40ebb002a3

Comment 14

[Robert Longson [:longsonr]]

Comment on attachment 408293 [details] [diff] [review]
patternFrame->mPaintLoop

I think it might be worth taking this in 1.9.2 although I don't know how prevalent this is in Inkscape generated files. Pretty simple low risk fix that includes a test.

Comment 15

[Robert Longson [:longsonr]]

pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/94bda78c3ab9