608653 - SVG patterns with unspecified width/height attributes should not paint

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Cameron McCormack (:heycam)]

If the width="" or height="" attributes on a <pattern> element are omitted, then the <pattern> should not be painted.

It's not clear whether this test should paint black (because of paint fallback, as Opera does) or green (as IE does).  I think it should be green, but I'll raise an issue for it.  Either way, it's clear it shouldn't be red.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: test

Comment 2

[Robert Longson [:longsonr]]

nsSVGPatternFrame will need fixing. We'll need to take care as it should work even without width and height as attributes if you animate them into existence but that's bug 544809 too.

Comment 3

[Robert Longson [:longsonr]]

Attachment: patch

Actually we have the lacuna values for width/height wrong.

According to the spec width/height default to 0 which is an error so we shouldn't display anything. This patch makes us display black which is wrong, but at least it's not red any more :-)

Fallback colours should be restricted to URI invalid and when the geometry of the applicable element has no width or no height, such as the case of a horizontal or vertical line.

The first case is easy, the second is harder as we don't distinguish between different types of pattern failures, we just return a pattern or null. Can you raise a follow up for this please?

Comment 4

[Cameron McCormack (:heycam)]

(In reply to comment #3)
> According to the spec width/height default to 0 which is an error so we
> shouldn't display anything. This patch makes us display black which is wrong,
> but at least it's not red any more :-)

Is the black coming from the fact that it is trying to fall back, but there is nothing to fall back to?

> Fallback colours should be restricted to URI invalid and when the geometry of
> the applicable element has no width or no height, such as the case of a
> horizontal or vertical line.

I am not convinced about using the fallback paint if the referenced pattern has width/height 0.  I don't think the spec is clear about that.  But I think the test you have (with the black rect behind the patterned rect) is fine.

> The first case is easy, the second is harder as we don't distinguish between
> different types of pattern failures, we just return a pattern or null. Can you
> raise a follow up for this please?

I'll raise an issue with the WG and raise a bug depending on the outcome of that.

Comment 5

[Robert Longson [:longsonr]]

(In reply to comment #4)
> 
> Is the black coming from the fact that it is trying to fall back, but there is
> nothing to fall back to?

We have a default fallback colour of black if a fallback colour is not specified. I think that may have been right in according to SVG 1.1 release 1 and may be wrong according to SVG 1.1 release 2 where it may be that it should not draw although that's likely to be tricky to implement.

> 
> I am not convinced about using the fallback paint if the referenced pattern has
> width/height 0.  I don't think the spec is clear about that.  But I think the
> test you have (with the black rect behind the patterned rect) is fine.

FWIW, I'm pretty convinced that you shouldn't use the fallback paint even though we do with or without this patch.

Comment 6

[Robert Longson [:longsonr]]

Attachment: hg changeset patch

Comment 7

[Daniel Holbert [:dholbert]]

This bug's patch currently breaks four reftests:
  reftests/svg/smil/transform/translate-pattern-1.svg
  reftests/svg/objectBoundingBox-and-pattern-01a.svg
  reftests/svg/objectBoundingBox-and-pattern-01b.svg
  reftests/svg/objectBoundingBox-and-pattern-01c.svg

Reftest log coming up in a second...

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: reftest failures

Here's the reftest failure log, suitable for passing to http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml

With this bug's patch, these tests fail on all platforms, both opt & debug builds. (based on a tryserver run earlier today)

Comment 9

[Robert Longson [:longsonr]]

All the reftests are broken. Updated patch coming up.

Comment 10

[Robert Longson [:longsonr]]

Attachment: fix failing reftests

Sorry about that. It's very good of you to land everything.

Comment 11

[Daniel Holbert [:dholbert]]

No problem -- thanks for the quick fix!

Comment 12

[Daniel Holbert [:dholbert]]

Landed: http://hg.mozilla.org/mozilla-central/rev/0181db574a21