1304991 - Create test cases for css-masking + clip-path + opacity

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[u459114]

We already have test cases of composing svg-mask & clip-path. But there is no test cases for
1. image mask + clip-path
2. mask + clip-path + opacity
3. clip-path + opacity

Comment 1

[u459114]

Attachment: Bug 1304991 - Part 1. Remove aOpacity parameter of GenerateMaskSurface.

Review commit: https://reviewboard.mozilla.org/r/81074/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/81074/

Comment 2

[u459114]

Attachment: Bug 1304991 - Part 2. mask/clip/opacity combination test.

Review commit: https://reviewboard.mozilla.org/r/81076/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/81076/

Comment 3

[u459114]

Comment on attachment 8794773 [details]
Bug 1304991 - Part 1. Remove aOpacity parameter of GenerateMaskSurface.

https://reviewboard.mozilla.org/r/81074/#review79680

::: layout/svg/nsSVGIntegrationUtils.cpp:495
(Diff revision 1)
>    return result;
>  }
>  
>  static DrawResult
>  GenerateMaskSurface(const PaintFramesParams& aParams,
> -                    float aOpacity, nsStyleContext* aSC,
> +                    nsStyleContext* aSC,

The reason that we do not need this parameter is because we actually push opacity at [1] and blend it back at [2]. So, no need to pass opacity into nsSVGMaskFrame::GetMaskForMaskedFrame again.

I notice this bug when testing mask-clipPath-opacity-01b.xthml in Part 2.

[1] https://hg.mozilla.org/mozilla-central/file/5011b0476532/layout/svg/nsSVGIntegrationUtils.cpp#l925
[2] https://hg.mozilla.org/mozilla-central/file/5011b0476532/layout/svg/nsSVGIntegrationUtils.cpp#l958

Comment 4

[u459114]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ed345787c2b

Comment 5

[u459114]

Comment on attachment 8794774 [details]
Bug 1304991 - Part 2. mask/clip/opacity combination test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81076/diff/1-2/

Comment 6

[u459114]

Comment on attachment 8794774 [details]
Bug 1304991 - Part 2. mask/clip/opacity combination test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81076/diff/2-3/

Comment 7

[u459114]

Comment on attachment 8794773 [details]
Bug 1304991 - Part 1. Remove aOpacity parameter of GenerateMaskSurface.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81074/diff/1-2/

Comment 8

[u459114]

Comment on attachment 8794774 [details]
Bug 1304991 - Part 2. mask/clip/opacity combination test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81076/diff/3-4/

Comment 9

[Markus Stange [:mstange]]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #3)
> Comment on attachment 8794773 [details]
> Bug 1304991 - Part 1. Remove aOpcity parameter.
> 
> https://reviewboard.mozilla.org/r/81074/#review79680
> 
> ::: layout/svg/nsSVGIntegrationUtils.cpp:495
> (Diff revision 1)
> >    return result;
> >  }
> >  
> >  static DrawResult
> >  GenerateMaskSurface(const PaintFramesParams& aParams,
> > -                    float aOpacity, nsStyleContext* aSC,
> > +                    nsStyleContext* aSC,
> 
> The reason that we do not need this parameter is because we actually push
> opacity at [1] and blend it back at [2]. So, no need to pass opacity into
> nsSVGMaskFrame::GetMaskForMaskedFrame again.
> 
> I notice this bug when testing mask-clipPath-opacity-01b.xthml in Part 2.

Oh, so there is an actual bug here? What branches does it affect? Do we need to uplift?

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8794773 [details]
Bug 1304991 - Part 1. Remove aOpacity parameter of GenerateMaskSurface.

https://reviewboard.mozilla.org/r/81074/#review79752

::: layout/svg/nsSVGIntegrationUtils.cpp:568
(Diff revision 1)
>      // maskFrame == nullptr means we get an image mask.
>      if (maskFrame) {
>        Matrix svgMaskMatrix;
>        RefPtr<SourceSurface> svgMask =
>          maskFrame->GetMaskForMaskedFrame(maskContext, aParams.frame,
> -                                         cssPxToDevPxMatrix, aOpacity,
> +                                         cssPxToDevPxMatrix, 1.0,

I wonder if there's anything we can do about the remaining caller that passes an non-1.0 opacity here, nsSVGUtils::PaintFrameWithEffects. It would be nice to replace nsSVGUtils::PaintFrameWithEffects with nsLayoutUtils::PaintFrame at some point.

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8794774 [details]
Bug 1304991 - Part 2. mask/clip/opacity combination test.

https://reviewboard.mozilla.org/r/81076/#review79754

Very nice!

Comment 12

[Markus Stange [:mstange]]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #3)
> Comment on attachment 8794773 [details]
> Bug 1304991 - Part 1. Remove aOpcaity parameter of GenerateMaskSurface.

There is a typo in the commit message - it needs to be aOpacity.

Comment 13

[u459114]

(In reply to Markus Stange [:mstange] from comment #9)
> (In reply to C.J. Ku[:cjku](UTC+8) from comment #3)
> > Comment on attachment 8794773 [details]
> > Bug 1304991 - Part 1. Remove aOpcity parameter.
> > 
> > https://reviewboard.mozilla.org/r/81074/#review79680
> > 
> > ::: layout/svg/nsSVGIntegrationUtils.cpp:495
> > (Diff revision 1)
> > >    return result;
> > >  }
> > >  
> > >  static DrawResult
> > >  GenerateMaskSurface(const PaintFramesParams& aParams,
> > > -                    float aOpacity, nsStyleContext* aSC,
> > > +                    nsStyleContext* aSC,
> > 
> > The reason that we do not need this parameter is because we actually push
> > opacity at [1] and blend it back at [2]. So, no need to pass opacity into
> > nsSVGMaskFrame::GetMaskForMaskedFrame again.
> > 
> > I notice this bug when testing mask-clipPath-opacity-01b.xthml in Part 2.
> 
> Oh, so there is an actual bug here? What branches does it affect? Do we need
> to uplift?

I will do mozregression later. By roughly checking the history of this file, I would say this bug exist for a long time, even before I started to work in position mask:
https://hg.mozilla.org/mozilla-central/file/c474f2b77df4/layout/svg/nsSVGIntegrationUtils.cpp#l528

Comment 14

[u459114]

We have this regression after bas created gfxContext::PushGroupForBlendBack and use it in nsSVGIntegrationUtils in bug 1210560(checked-in in FF 45):
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1210560&attachment=8674267

Comment 15

[u459114]

Comment on attachment 8794773 [details]
Bug 1304991 - Part 1. Remove aOpacity parameter of GenerateMaskSurface.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81074/diff/2-3/

Comment 16

[u459114]

Comment on attachment 8794774 [details]
Bug 1304991 - Part 2. mask/clip/opacity combination test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81076/diff/4-5/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7c04fd81fc2
Part 1. Remove aOpacity parameter of GenerateMaskSurface. r=mstange
https://hg.mozilla.org/integration/autoland/rev/f055c158f12c
Part 2. mask/clip/opacity combination test. r=mstange

Comment 18

[u459114]

(In reply to Markus Stange [:mstange] from comment #10)
> Comment on attachment 8794773 [details]
> Bug 1304991 - Part 1. Remove aOpcaity parameter of GenerateMaskSurface.
> 
> https://reviewboard.mozilla.org/r/81074/#review79752
> 
> ::: layout/svg/nsSVGIntegrationUtils.cpp:568
> (Diff revision 1)
> >      // maskFrame == nullptr means we get an image mask.
> >      if (maskFrame) {
> >        Matrix svgMaskMatrix;
> >        RefPtr<SourceSurface> svgMask =
> >          maskFrame->GetMaskForMaskedFrame(maskContext, aParams.frame,
> > -                                         cssPxToDevPxMatrix, aOpacity,
> > +                                         cssPxToDevPxMatrix, 1.0,
> 
> I wonder if there's anything we can do about the remaining caller that
> passes an non-1.0 opacity here, nsSVGUtils::PaintFrameWithEffects. It would
> be nice to replace nsSVGUtils::PaintFrameWithEffects with
> nsLayoutUtils::PaintFrame at some point.
Filed Bug 1305636 and Bug 1305637

Comment 19

[u459114]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fb806de3cf7

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f7c04fd81fc2
https://hg.mozilla.org/mozilla-central/rev/f055c158f12c