354295 - Gecko should change their fill="url(#invalidurl)" behavior to match Opera/Safari

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Eric Seidel]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7

Gecko should change their fill="url(#invalidurl)" behavior to match Opera/Safari

The SVG 1.1 FULL specification states that when an invalid paintserver URL is provided for fill/stroke, the document is "in error" and a "highly percievable error" message should be displayed.

No browser has any plans to implement these error messages however.

As a result, we've all implemented some sort of "fill fallback" behavior.  Opera and (recently) Safari have chosen to fall back to 'black' (the default value) on an invalid fill.  Gecko falls back to 'none'.

After discussing this with tor, we agree Gecko should change to match Safari/Opera in this instance.

Reproducible: Always

Steps to Reproduce:
See attached test case.

Comment 1

[Eric Seidel]

Attachment: simple test case demonstrating desired change

Comment 2

[Robert Longson [:longsonr]]

Shouldn't we log an error in the Error Console in addition to this?

Comment 3

[Jonathan Watt [:jwatt]]

Sounds good to me.

Comment 4

[Robert Longson [:longsonr]]

Attachment: patch

I've done the same thing for invalid strokes in that they will also default to black.

I also ignore the opacity if there is an error in order to make the black more obvious but I could continue to use the actual value instead.

Comment 5

[tor]

Robert, did you want to add logging for this error?

Eric, does webkit respect the fill/stroke opacity in this situation?

Comment 6

[Robert Longson [:longsonr]]

Attachment: implement logging too since I suggested it.

Comment 7

[tor]

That will spew a warning each time a invalid referenced gradient/pattern is painted or sized.

Comment 8

[Jonathan Watt [:jwatt]]

I don't think we should warn in this case. It's useful for things like gradient reference loops where it could be hard for the SVG author to track down the cause, but in this case it's fairly obvious where to look if things don't work.

Comment 9

[Robert Longson [:longsonr]]

Attachment: logging removed

Comment 10

[Robert Longson [:longsonr]]

Attachment: add const

same as previous patch with addition of const to static variable.

Comment 11

[Robert Longson [:longsonr]]

checked in

Comment 12

[Eric Seidel]

Robert...

> I've done the same thing for invalid strokes in that they will also default to
> black.

The default stroke is "none" not "black".  So I don't follow your logic.

> I also ignore the opacity if there is an error in order to make the black more
> obvious but I could continue to use the actual value instead.

WebKit would continue to respect the opacity I believe.  It's a separate property.  I'm not sure what Opera does.  Unfortunately I'm not working on SVG much anymore...

Comment 13

[Eric Seidel]

Either way.  I'm not sure that it matters much.  I'll leave the current SVG hackers to debate this all later.  Thanks for the fix!

Comment 14

[Robert Longson [:longsonr]]

(In reply to comment #12)
> Robert...
> 
> > I've done the same thing for invalid strokes in that they will also default to
> > black.
> 
> The default stroke is "none" not "black".  So I don't follow your logic.

I mean that if you specify that an invalid stroke URL, you would now get a black stroke rather than no stroke.

> 
> > I also ignore the opacity if there is an error in order to make the black more
> > obvious but I could continue to use the actual value instead.
> 
> WebKit would continue to respect the opacity I believe.  It's a separate
> property.  I'm not sure what Opera does.  Unfortunately I'm not working on SVG
> much anymore...
> 

Tor, would you like me to produce another patch to honour opacity.

Comment 15

[Eric Seidel]

> > The default stroke is "none" not "black".  So I don't follow your logic.

> I mean that if you specify that an invalid stroke URL, you would now get a
> black stroke rather than no stroke.

I was referring to the fact that this bug was all about how previously invalid fills in Gecko were "none" instead of the default value of "black".  This bug was to correct that.  Now this patch has changed invalid strokes from the default value of "none" to "black". ;)  So following the original intention of the bug, the fix is both a fix and a "regression".

Again, this is a super-minor-edgecase issue.  I was just pointing out our logic-discrepancies.

Comment 16

[Jonathan Watt [:jwatt]]

FWIW, I agree with Eric, stroke and opacity should be fixed.

Comment 17

[Robert Longson [:longsonr]]

Attachment: revert stroke change and honour opacity

Comment 18

[Robert Longson [:longsonr]]

(In reply to comment #17)
> Created an attachment (id=245758) [edit]
> revert stroke change and honour opacity
> 

Checked in.