User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0 Build ID: 20170311084704 Steps to reproduce: When applying a (-webkit-)mask-image to an element, with an image that happens to be an svg (so *not* a reference to an svg <mask>, but a whole svg image that acts as a mask), the `opacity` for the element is no longer respected. See testcase. It contains two red squares with opacity: .5 and a circle mask; one is an svg and one is a png. They should be rendered identically. Actual results: With the svg mask image, the opacity is discarded and the circle is solid red instead of light red. Expected results: Chrome respects the opacity and renders both circles identically.

Can you find a regression range using -moz-regression. http://mozilla.github.io/mozregression/

I believe this is not a regression. The bug is in Firefox 53, which is the first version that supports mask-image.

Seems like bug 1323912 but that's been fixed for a while.

Yes, it regressed by bug 1323912: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6b1a87d941fa9 4bda048d48276ec2025eaf0f7c4&tochange=5de8385ec9731aca0832c5d10674a84f8bd0cdc8

1. http://searchfox.org/mozilla-central/source/image/VectorImage.cpp#789 opacity(aSVGContext ? aSVGContext->GetGlobalOpacity() : aOpacity) although we pass aOpacity(0.5), but opacity is actually be set as aSVGContext->GetGlobalOpacity() which is 1.0 2. We miss a test case of svg as mask with opacity != 1.0

[Tracking Requested - why for this release]:

(In reply to d.huigens from comment #2) > I believe this is not a regression. The bug is in Firefox 53, which is the > first version that supports mask-image. Exactly, it's not a regression actually as mask-image property is supported as of FF53. @CJ, to ensure we ship this feature in good shape, please help to uplift to aurora and beta after it's considered to be safe to do so in nightly. Thanks.

Comment on attachment 8846459 [details] Bug 1346618 - Part 1. Correct opacity pass to VectorImage. https://reviewboard.mozilla.org/r/119480/#review123448 ::: layout/base/nsLayoutUtils.cpp:6737 (Diff revision 2) > /* Fast path when there is no need for image spacing */ > if (aRepeatSize.width == aDest.width && aRepeatSize.height == aDest.height) { > return DrawImageInternal(aContext, aPresContext, aImage, > aSamplingFilter, aDest, aFill, aAnchor, > aDirty, svgContext, aImageFlags, aExtendMode, > aOpacity); Should we be passing 1.0 here then? Or is this argument used in other cases? I'm not familiar with this code, but the way it's written at the moment feels like we might accidentally double-apply opacity in some cases.

Comment on attachment 8846459 [details] Bug 1346618 - Part 1. Correct opacity pass to VectorImage. https://reviewboard.mozilla.org/r/119480/#review123448 > Should we be passing 1.0 here then? Or is this argument used in other cases? I'm not familiar with this code, but the way it's written at the moment feels like we might accidentally double-apply opacity in some cases. It is possible that pass aSVGContext as Nothing(), in that case, aOpacity is used. We may just skip the last parameter of DrawImageInternal, and use the defalut valeu of it static DrawResult DrawImageInternal(gfxContext& aContext, /... float aOpacity = 1.0)

Comment on attachment 8846459 [details] Bug 1346618 - Part 1. Correct opacity pass to VectorImage. https://reviewboard.mozilla.org/r/119480/#review123448 > It is possible that pass aSVGContext as Nothing(), in that case, aOpacity is used. We may just skip the last parameter of DrawImageInternal, and use the defalut valeu of it > > static DrawResult > DrawImageInternal(gfxContext& aContext, > /... > float aOpacity = 1.0) No, I am wrong. RasterImage path do nees aOpacity be passed

Comment on attachment 8846459 [details] Bug 1346618 - Part 1. Correct opacity pass to VectorImage. https://reviewboard.mozilla.org/r/119480/#review123448 > No, I am wrong. RasterImage path do nees aOpacity be passed RasterImage::Draw uses aOpacity, ignore aSVGContext VecorImage::Draw uses aSVGContext's op acity, ignore aOpacity. So we do need pass both opacity values.

Comment on attachment 8846460 [details] Bug 1346618 - Part 2. Test case. https://reviewboard.mozilla.org/r/119482/#review124080

Comment on attachment 8846459 [details] Bug 1346618 - Part 1. Correct opacity pass to VectorImage. https://reviewboard.mozilla.org/r/119480/#review124082 ::: commit-message-c40ca:4 (Diff revision 4) > +[1], SVGDrawingParameters::opacity will still be set as 1.0 since we also pass > +aSVGContext to it. As a result, SVGDrawingParameters::opacity is set by the > +return value of aSVGContext->GetGlobalOpacity(), which is 1.0. I tried to fix The SVGDrawingParameters constructor seems to be the only caller of aSVGContext->GetGlobalOpacity(). Why is this special case necessary? Could we get rid of it?

Comment on attachment 8846459 [details] Bug 1346618 - Part 1. Correct opacity pass to VectorImage. https://reviewboard.mozilla.org/r/119480/#review124082 > The SVGDrawingParameters constructor seems to be the only caller of aSVGContext->GetGlobalOpacity(). Why is this special case necessary? Could we get rid of it? I filed bug 1348378 to fix what you mentioned. We can also fix it in this bug at once. Update Part 3 to remove GetGlobalOpacity.

Comment on attachment 8849481 [details] Bug 1346618 - Part 2. Remove SVGImageContext::GetGlobalOpacity. https://reviewboard.mozilla.org/r/122262/#review124524 ::: layout/svg/SVGImageContext.h (Diff revision 2) > - gfxFloat GetGlobalOpacity() const { > - return mGlobalOpacity; > - } Doesn't this make mGlobalOpacity completely unused? If you can remove that field entirely, then the next patch will get a little simpler.

Comment on attachment 8849481 [details] Bug 1346618 - Part 2. Remove SVGImageContext::GetGlobalOpacity. https://reviewboard.mozilla.org/r/122262/#review124524 > Doesn't this make mGlobalOpacity completely unused? If you can remove that field entirely, then the next patch will get a little simpler. mGlobalOpacity is used in SVGImageContext::Hash[1]. Although there is no need to export this data member, I think SVGImageContext still need it internally. [1] http://searchfox.org/mozilla-central/source/layout/svg/SVGImageContext.h#101

Comment on attachment 8849481 [details] Bug 1346618 - Part 2. Remove SVGImageContext::GetGlobalOpacity. https://reviewboard.mozilla.org/r/122262/#review124558 ::: layout/svg/SVGImageContext.h (Diff revision 2) > - gfxFloat GetGlobalOpacity() const { > - return mGlobalOpacity; > - } SVGImageContext::Hash is just there so that SVGImageContext can be used as a key in a map. SVGImageContext::Hash uses mGlobalOpacity for the same reason that operator== does: Different SVGImageContext objects with different mGlobalOpacity should not have the same hash. But if mGlobalOpacity is removed, it no longer needs to affect the hash.

Comment on attachment 8849482 [details] Bug 1346618 - Part 1. Remove SVGImageContext::mGlobalOpacity. https://reviewboard.mozilla.org/r/122264/#review132224

Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6bd69f962dd5 Part 1. Remove SVGImageContext::mGlobalOpacity. r=mstange https://hg.mozilla.org/integration/autoland/rev/b2a3d0fd9675 Part 2. Test case. r=mstange

Backed out for permafailing reftest 315920-9.html on Android 4.3 API15+ debug: https://hg.mozilla.org/integration/autoland/rev/423ac063ee983bf1444b0d7190c0d38b40dccf8e https://hg.mozilla.org/integration/autoland/rev/21df5e7b04e8b439b26068f5ceebd12318e75bc6 Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b2a3d0fd967578dcce222d4396ee3ac70b99e50b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=91351580&repo=autoland >[task 2017-04-13T16:04:56.999552Z] 16:04:56 INFO - REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/bugs/315920-9.html == http://10.0.2.2:8854/tests/layout/reftests/bugs/315920-9-ref.html | image comparison, max difference: 229, number of differing pixels: 45

It because I added a new test, so 315920-9.html is pushed from R14 to R15, and 315920-9.html fails on R15. No idea of how to fix this kind of failure.

Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/52a7a9eda328 Part 1. Remove SVGImageContext::mGlobalOpacity. r=mstange

CJ, are you still intending to uplift this to beta 54?

Comment on attachment 8871147 [details] [diff] [review] Beta patch Approval Request Comment [Feature/Bug causing the regression]: Bug 1323912 [User impact if declined]: the opacity property of an element will be ignored if we also apply an SVG mask onto it. [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: this patch only [Is the change risky?]: no [Why is the change risky/not risky?]: small change [String changes made/needed]: N/A

Comment on attachment 8871147 [details] [diff] [review] Beta patch Fix a opacity issue when using svg mask image. Beta54+. Should be in 54 beta 11.

(In reply to C.J. Ku[:cjku](UTC+8) from comment #44) > It because I added a new test, so 315920-9.html is pushed from R14 to R15, > and 315920-9.html fails on R15. No idea of how to fix this kind of failure. I think it's valid to disable the flaky test (315920-9.html) in that case with a follow-up bug filed for fixing and re-enabling it. It's a ticking time bomb that you shouldn't be beholden to dealing with.

(In reply to C.J. Ku[:cjku](UTC+8) from comment #51) > [Is this code covered by automated tests?]: no > [Has the fix been verified in Nightly?]: yes > [Needs manual test from QE? If yes, steps to reproduce]: no Setting qe-verify- based on C.J. Ku's assessment on manual testing needs.