423071 - SVG bad clippath rendering when changed via DOM in FF3 b4 but works in FF2

Status: VERIFIED FIXED

(Core :: SVG, defect, P1)

Description

[Gerald Wong]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4) Gecko/2008030714 Firefox/3.0b4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4) Gecko/2008030714 Firefox/3.0b4

The example is here:
http://gplanet.byethost13.com/tdos3/tdos3.php

There is inline SVG in XHTML in the first box, and an embedded SVG object in the second box. The SVG is changed using Javascript/ECMAScript.

When a black place is clicked, the blue triangle moves and leaves trails and blue marks instead of having a blue circle. The circle is generated using SVG clippath.

When the window has to be rerendered, e.g. switching tabs, the trails disappear.

Reproducible: Always

Steps to Reproduce:
1. Click on a black place.
Actual Results:  
The blue triangle moves and leaves trails and blue marks. When the window has to be rerendered, e.g. switching tabs, the trails disappear.

Expected Results:  
The blue triangle moves with a blue circle around it.

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

Comment 2

[Jonathan Watt [:jwatt]]

Ugh, more regressions.

Comment 3

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

Why don't you just call GetCoveredRegion and make frames that have a valid covered rect return mRect from GetCoveredRegion?

Comment 4

[Robert Longson [:longsonr]]

That would work, frames that have a valid covered rect do already return mRect from GetCoveredRegion pretty much. I did it this way because:

a) I'm nervous about messing with nsSVGPathGeometry's implementation of GetCoveredRegion and thereby causing regressions
b) This way is hopefully faster for frames that do have a valid covered rect as we do less work.

So I'd rather stick with what I've done at this stage.

Comment 5

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

> a) I'm nervous about messing with nsSVGPathGeometry's implementation of
> GetCoveredRegion and thereby causing regressions

Why do you need to mess with it?

> b) This way is hopefully faster for frames that do have a valid covered rect as
> we do less work.

I don't think we should worry about that.

Comment 6

[Robert Longson [:longsonr]]

Attachment: address review comments

(In reply to comment #5)
> > a) I'm nervous about messing with nsSVGPathGeometry's implementation of
> > GetCoveredRegion and thereby causing regressions
> 
> Why do you need to mess with it?

It does return what would be mRect but if there are markers it does more work to do so. I've left it as is.

> 
> > b) This way is hopefully faster for frames that do have a valid covered rect as
> > we do less work.
> 
> I don't think we should worry about that.
> 

OK.

Comment 7

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

Comment on attachment 309881 [details] [diff] [review]
address review comments

Lose the rect.IsEmpty() checks. If they're needed, they should happen in the implementation of InvalidateRect.

Comment 8

[Robert Longson [:longsonr]]

Attachment: version for check in

Comment 9

[Damon Sicore (:damons)]

Does this patch require a beta cycle?  Must it block beta 5?  Not saying we won't take the patch, but P1 means it blocks the beta.

Comment 10

[Jonathan Watt [:jwatt]]

This patch is ready to go. If Robert doesn't check it in tomorrow then I will.

Comment 11

[Robert Longson [:longsonr]]

checked in so hopefully question moot.

Comment 12

[Carsten Book [:Tomcat]]

verified fixed using the testurl/testcase and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041217 Minefield/3.0pre ID:2008041217

--> Verified fixed