534612 - SVG: clipPath references within clipPath-elements don't work correctly

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[hum]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5 (.NET CLR 3.5.30729)

When you create an element that uses a clip path that uses another clip path, clipping doesn't work correctly.

Reproducible: Always

Steps to Reproduce:
1. Save the following as an svg-file:

<?xml version= "1.0" encoding="UTF-8" standalone="no"?>
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 20000802//EN" "http://www.w3.org/TR/2000/03/WD-SVG-20000303/DTD/svg-20000303-stylable.dtd">

<svg width="100%" height="100%" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 1000 1000">
  <g transform="translate(0, 100)">
    <clipPath id="clip1">
      <rect x="0" y="40" width="220" height="40" />
    </clipPath>
    <clipPath id="clip2">
      <rect x="0" y="-40" width="220" height="40" clip-path="url(#clip1)" />
    </clipPath>
    <rect x="0" y="-40" width="220" height="40" style="fill: #aaaaaa; stroke:#000000; stroke-width:1;" />
    <text x="90" y="-13" style="font-size:20px;" clip-path="url(#clip2)">Bug?</text>
  </g>
</svg>

2. Open this svg page in Firefox 3.5
3. Compare with the results generated using e.g. IE 8.0 + Adobe SVG Viewer 3.0.

Actual Results:  
"Bug?"-text is shown inside gray box.


Expected Results:  
Only the gray box is shown.

http://www.w3.org/TR/SVG/masking.html#EstablishingANewClippingPath

Comment 1

[hum]

Attachment: Described test page as an attachment

Comment 2

[Robert Longson [:longsonr]]

Attachment: WIP

Problems with WIP...

Clip is affected by opacity=0 when it should not be.
hit testing changes not complete

Comment 3

[Robert Longson [:longsonr]]

Attachment: patch

Comment 4

[Robert Longson [:longsonr]]

(In reply to comment #3)

Passes http://dev.w3.org/SVG/profiles/1.1F2/test/svg/masking-path-07-b.svg but not the test in this bug so still WIP.

Comment 5

[Robert Longson [:longsonr]]

Attachment: updated patch (fixes this bug)

Comment 6

[Robert Longson [:longsonr]]

Attachment: patch

Comment 7

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

+          gfx->PushGroup(gfxASurface::CONTENT_COLOR_ALPHA);

Wouldn't CONTENT_ALPHA be enough here?

I actually don't understand how this whole thing works.

+        if (isTrivial) {
+          clipPathFrame->ClipPaint(aContext, kid, aMatrix);
This sets up clipping on aContext. But it's inside a Save and Restore pair, so how does it actually affect the clipping state at the end of the outer call to ClipPaint? As far as I can tell, it doesn't affect it at all.

+          gfx->PopGroupToSource();
+          gfx->PushGroup(gfxASurface::CONTENT_COLOR_ALPHA);
+          clipPathFrame->ClipPaint(aContext, kid, aMatrix);
+          nsRefPtr<gfxPattern> clipMaskSurface = gfx->PopGroup();
Why not just set clipMaskSurface to the result of popping the first group, then call ClipPaint and then Mask?+      // or one which is itself clipped

+      PRBool isOK = PR_TRUE;
+      if (nsSVGEffects::GetEffectProperties(kid).GetClipPathFrame(&isOK))
+        return PR_FALSE;

Shouldn't we be doing a hit-test here? Just bailing out because there's a clip-path on the child can't be right.

Comment 8

[Robert Longson [:longsonr]]

(In reply to comment #7)
> +          gfx->PushGroup(gfxASurface::CONTENT_COLOR_ALPHA);
> 
> Wouldn't CONTENT_ALPHA be enough here?
> 
> I actually don't understand how this whole thing works.
> 

It's copied from nsSVGUtils::PaintFrameWithEffects with simplifications. I'll investigate your comments.

Comment 9

[Robert Longson [:longsonr]]

Attachment: updated patch

(In reply to comment #7)
> +          gfx->PushGroup(gfxASurface::CONTENT_COLOR_ALPHA);
> 
> Wouldn't CONTENT_ALPHA be enough here?

Brilliant idea. Indeed it would be!

> 
> I actually don't understand how this whole thing works.
> 
> +        if (isTrivial) {
> +          clipPathFrame->ClipPaint(aContext, kid, aMatrix);
> This sets up clipping on aContext. But it's inside a Save and Restore pair, so
> how does it actually affect the clipping state at the end of the outer call to
> ClipPaint? As far as I can tell, it doesn't affect it at all.

You paint it using the clip so you don't need to keep the clipping state. Removing the Save/Restore breaks clipping for http://dev.w3.org/SVG/profiles/1.1F2/test/svg/masking-path-07-b.svg 

> 
> +          gfx->PopGroupToSource();
> +          gfx->PushGroup(gfxASurface::CONTENT_COLOR_ALPHA);
> +          clipPathFrame->ClipPaint(aContext, kid, aMatrix);
> +          nsRefPtr<gfxPattern> clipMaskSurface = gfx->PopGroup();
> Why not just set clipMaskSurface to the result of popping the first group, then
> call ClipPaint and then Mask?+      // or one which is itself clipped

That works... Done.

> 
> +      PRBool isOK = PR_TRUE;
> +      if (nsSVGEffects::GetEffectProperties(kid).GetClipPathFrame(&isOK))
> +        return PR_FALSE;
> 
> Shouldn't we be doing a hit-test here? Just bailing out because there's a
> clip-path on the child can't be right.

This code is from the IsTrivial method it's not part of hit testing.

Comment 10

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

(In reply to comment #9)
> (In reply to comment #7)
> > +          gfx->PushGroup(gfxASurface::CONTENT_COLOR_ALPHA);
> > 
> > Wouldn't CONTENT_ALPHA be enough here?
> 
> Brilliant idea. Indeed it would be!

Seems like we should fix that in nsSVGUtils::PaintFrameWithEffects as well. Not necessarily in this patch.

> > I actually don't understand how this whole thing works.
> > 
> > +        if (isTrivial) {
> > +          clipPathFrame->ClipPaint(aContext, kid, aMatrix);
> > This sets up clipping on aContext. But it's inside a Save and Restore pair, so
> > how does it actually affect the clipping state at the end of the outer call to
> > ClipPaint? As far as I can tell, it doesn't affect it at all.
> 
> You paint it using the clip so you don't need to keep the clipping state.
> Removing the Save/Restore breaks clipping for
> http://dev.w3.org/SVG/profiles/1.1F2/test/svg/masking-path-07-b.svg 

I'm confused then. Basically we're doing
ctx->Save();
ctx->SetSomePath();
ctx->Clip();
ctx->SetSomeOtherPath();
ctx->Restore();
ctx->Clip();
... right?

My understanding of cairo/Thebes is that the first path has no effect.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #7)
> > > +          gfx->PushGroup(gfxASurface::CONTENT_COLOR_ALPHA);
> > > 
> > > Wouldn't CONTENT_ALPHA be enough here?
> > 
> > Brilliant idea. Indeed it would be!
> 
> Seems like we should fix that in nsSVGUtils::PaintFrameWithEffects as well. Not
> necessarily in this patch.
> 
> > > I actually don't understand how this whole thing works.
> > > 
> > > +        if (isTrivial) {
> > > +          clipPathFrame->ClipPaint(aContext, kid, aMatrix);
> > > This sets up clipping on aContext. But it's inside a Save and Restore pair, so
> > > how does it actually affect the clipping state at the end of the outer call to
> > > ClipPaint? As far as I can tell, it doesn't affect it at all.
> > 
> > You paint it using the clip so you don't need to keep the clipping state.
> > Removing the Save/Restore breaks clipping for
> > http://dev.w3.org/SVG/profiles/1.1F2/test/svg/masking-path-07-b.svg 
> 
> I'm confused then. Basically we're doing

Here's my understanding:

> ctx->Save();
> ctx->SetSomePath();
> ctx->Clip();

- initial clip is intersected with SomePath.

> ctx->SetSomeOtherPath();
> ctx->Restore();

- initial clip is restored

> ctx->Clip();

- initial clip is intersected with SomeOtherPath

> ... right?
> 
> My understanding of cairo/Thebes is that the first path has no effect.

The clip is part of gstate and is saved and restored.

Comment 12

[Robert Longson [:longsonr]]

(In reply to comment #10)
> 
> I'm confused then. Basically we're doing
> ctx->Save();
> ctx->SetSomePath();
> ctx->Clip();
> ctx->SetSomeOtherPath();
> ctx->Restore();
> ctx->Clip();
> ... right?
> 

In this scenario is the inner clip trivial? The outer one can't be.

<clipPath id="cp1">
    <circle id="c1" cx="100" cy="100" r="50"/>
</clipPath>
<clip-path id="cp2">
  <use xlink:href="#cp1" clip-path="url(#cp1)"/>
</clip-path>
<clip-path id="cp3">
  <use xlink:href="#cp1"/>
</clip-path>

cp2 is outer not trivial and inner not trivial
cp3 is outer not trivial and inner trivial

I'll check the reftests cover both cases (I think currently they just cover one) and add the missing case.

Comment 13

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

Why can't the outer one be trivial? That's the situation I'm worried about, when they're both trivial.

Comment 14

[Robert Longson [:longsonr]]

(In reply to comment #13)
> Why can't the outer one be trivial? That's the situation I'm worried about,
> when they're both trivial.

Ahh. I did wonder about the logic you posted. We're all on the same page now and you're not going completely mad either :-)

If the outer one was trivial and the inner was clipped (trivial or not) then the logic doesn't work, as you've surmised.

In mitigation:

a) The last few lines of the patch change the IsTrivial method so that if the inner is clipped (trivial or not) then the outer is forced non-trivial.

b) The reftest would fail if the added IsTrivial lines were removed.

Comment 15

[Robert Longson [:longsonr]]

Comment on attachment 426281 [details] [diff] [review]
updated patch

will write more tests.

Comment 16

[Robert Longson [:longsonr]]

Attachment: path with better reftest

The reftest covers 100% of the new code i.e. it has both inner trivial and non-trivial clips (the final case in the reftest is inner non-trivial).

One line of code is removed (the gfx->Paint() call from the previous patch) as that was not necessary, otherwise it's the same.

Comment 17

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

Comment on attachment 426830 [details] [diff] [review]
path with better reftest

Great!

Comment 18

[Robert Longson [:longsonr]]

Comment on attachment 426830 [details] [diff] [review]
path with better reftest

Fails some additional tests... new patch on the way

Comment 19

[Robert Longson [:longsonr]]

Attachment: updated based on new test data

Changed code passes additional tests

Comment 20

[Robert Longson [:longsonr]]

(In reply to comment #7)
> +          gfx->PopGroupToSource();
> +          gfx->PushGroup(gfxASurface::CONTENT_COLOR_ALPHA);
> +          clipPathFrame->ClipPaint(aContext, kid, aMatrix);
> +          nsRefPtr<gfxPattern> clipMaskSurface = gfx->PopGroup();
> Why not just set clipMaskSurface to the result of popping the first group, then
> call ClipPaint and then Mask?+      // or one which is itself clipped

That doesn't work. You end up with a union of clip regions when you need an intersection. The latest patch reverts that change.

Comment 21

[Robert Longson [:longsonr]]

pushed http://hg.mozilla.org/mozilla-central/rev/475768f37b1a