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

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: u459114, Assigned: u459114)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

3 years ago
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
Assignee

Updated

3 years ago
Assignee: nobody → cku
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 3

3 years ago
mozreview-review
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
Assignee

Updated

3 years ago
Attachment #8794773 - Flags: review?(mstange)
Attachment #8794774 - Flags: review?(mstange)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(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

3 years ago
mozreview-review
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.
Attachment #8794773 - Flags: review?(mstange) → review+

Comment 11

3 years ago
mozreview-review
Comment on attachment 8794774 [details]
Bug 1304991 - Part 2. mask/clip/opacity combination test.

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

Very nice!
Attachment #8794774 - Flags: review?(mstange) → review+
(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.
Assignee

Comment 13

3 years ago
(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
Assignee

Comment 14

3 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

3 years ago
Pushed by cku@mozilla.com:
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
Assignee

Updated

3 years ago
Blocks: 1305637
Assignee

Comment 18

3 years ago
(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 20

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f7c04fd81fc2
https://hg.mozilla.org/mozilla-central/rev/f055c158f12c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.