481614 - SVG masking-path test fails, clip property doesn't work properly with <image> element.

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Chwiej Pawel]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090223 Minefield/3.2a1pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090223 Minefield/3.2a1pre

Seems that <image ... clip="rect(10,10,10,10)" ... /> doesn't work.

Reproducible: Always

Steps to Reproduce:
1. Go to http://www.w3.org/Graphics/SVG/Test/20061213/svggen/masking-path-06-b.svg
2. Compare SVG raster image with the reference png image.
Actual Results:  
Image is not displayed within red rectangle.

Expected Results:  
SVG should match the reference file, raster image should be displayed within red rectangle.

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

The appropriate part of the SVG spec is here...

http://www.w3.org/TR/SVG11/masking.html#OverflowAndClipProperties

Note that this patch does not attempt to do anything with pattern or use elements (would need a followup bug), nor does it do outer svg elements (bug 378923).

Comment 2

[Robert Longson [:longsonr]]

Adding interested parties from Firefox 3 beta 4 bug in svg clipping module? thread in mozilla.dev.tech.svg

Comment 3

[Jonathan Watt [:jwatt]]

Comment on attachment 373497 [details] [diff] [review]
patch

nsSVGUtils::GetClipRectForFrame seems a bit wrong to me. From my reading of the CSS spec, 'overflow:visible' only makes content outside the "natural" clip rect visible, it does not make anything outside the rect specified by an explicit 'clip' property visible. That aside, could we have an early return along the lines of:

  if (!(disp->mClipFlags & NS_STYLE_CLIP_RECT)) {
    NS_ASSERTION(disp->mClipFlags != NS_STYLE_CLIP_AUTO,
                 "We don't know about this type of clip.");
    return gfxRect(aX, aY, aWidth, aHeight);
  }


>+    if (NS_STYLE_CLIP_RIGHT_AUTO & disp->mClipFlags) {
>+      clipRect.size.width = aWidth - clipRect.X();
>+    }

I don't think this is right for text direction RtL.

>+    if (NS_STYLE_CLIP_BOTTOM_AUTO & disp->mClipFlags) {
>+      clipRect.size.height = aHeight - clipRect.Y();
>+    }

And I'm curious. What does clipRect.size.width/.height contain in these cases before you set them? (I.e. what does the style system store in it's mClip in these cases?)

Comment 4

[Jonathan Watt [:jwatt]]

For the reftests, can you mention "property" in the title: e.g. "Test the 'clip' property". (Hmm, maybe we want to map the 'clip' attribute into style. Need to check the spec.) Also, can you add public domain dedication text?
For your first test, it would be good to check that the clip is *exactly* right.

<svg xmlns="http://www.w3.org/2000/svg" version="1.1">
  <title>Testcase for clip</title>
  <rect width="100%" height="100%" fill="lime"/>
  <rect x="10" y="10" width="100" height="100" fill="red"/>
  <svg clip="rect(10, 10, 110, 110)">
    <rect width="100%" height="100%" fill="red" />
    <rect x="10" y="10" width="100" height="100" fill="lime"/>
  </svg>
</svg>

That way it will fail if the clip is too big or too small.

Comment 5

[Jonathan Watt [:jwatt]]

Oh, it's mapped into style. Cool. :-)

It might be good to have some auto tests too for:

 * rect(auto, 10, 110, 110)
 * rect(10, auto, 110, 110)
 * rect(10, 10, auto, 110)
 * rect(10, 10, 110, auto)

Comment 6

[Jonathan Watt [:jwatt]]

Sorry, got the args for rect() wrong, obviously. Should be based on rect(10, 110, 110, 10). Might be nice to have a RtL tests too.

Can you also document GetClipRectForFrame with something like this:

/**
 * Get the clip rect for the given frame, taking into account the CSS 'clip'
 * property. See:
 * http://www.w3.org/TR/SVG11/masking.html#OverflowAndClipProperties
 * The arguments for aX, aY, aWidth and aHeight should be the dimensions of
 * the viewport established by aFrame.
 */

(In reply to comment #1)
> Note that this patch does not attempt to do anything with pattern or use
> elements (would need a followup bug), nor does it do outer svg elements (bug
> 378923).

Sure. I filed bug 489901 for that then.

I think that's everything.

Comment 7

[Robert Longson [:longsonr]]

I'm afraid RTL is totally non implemented for SVG (bug 311545), although I thing this code is right anyway. I can address all the other comments though.

Comment 8

[Jonathan Watt [:jwatt]]

We don't honor it in text, but we could honor it on viewport elements for 'clip'. We'd just need to look at the 'direction' property, no?

http://www.w3.org/TR/CSS21/visuren.html#propdef-direction

Comment 9

[Jonathan Watt [:jwatt]]

And in the case of tests, just set that property.

Comment 10

[Robert Longson [:longsonr]]

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#1023 which is the html equivalent for absolute positioned html frames doesn't have any direction code. I'll write some reftests to check though.

Comment 11

[Robert Longson [:longsonr]]

Attachment: address review comments and update to tip

Comment 12

[Robert Longson [:longsonr]]

Attachment: update to tip again

Comment 13

[Robert Longson [:longsonr]]

(In reply to comment #1)
> Note that this patch does not attempt to do anything with pattern or use
> elements (would need a followup bug), nor does it do outer svg elements (bug
> 378923).

Testing reveals that symbol/use works already with or without this patch as it rides along on an nsSVGOuterSVGFrame.

Comment 14

[Robert Longson [:longsonr]]

checked in http://hg.mozilla.org/mozilla-central/rev/128867ce454d

Comment 15

[Markus Stange [:mstange]]

The added reftests failed intermittently on the new Mac "everythingelse" test box today:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1245377401.1245379142.11871.gz
The runs before and after it were green.

Are intermittent failures of these tests possible? Or does it mean that bug 499161 isn't completely fixed yet?

Comment 16

[Robert Longson [:longsonr]]

There's no dynamic part to any of the tests so they shouldn't fail. The images produced are exactly what you'd get if the build didn't contain the code patch which is suspicious.