Closed Bug 1346618 Opened 7 years ago Closed 7 years ago

When applying an mask-image which is an svg, opacity does not work

Categories

(Core :: Layout, defect, P1)

Product:
Core
Component:
Layout
Version:
53 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox52 --- unaffected
firefox53 --- wontfix
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: d.huigens, Assigned: u459114)

References

Details

Attachments

(4 files, 2 obsolete files)

testcase.html
922 bytes, text/html
Details
Bug 1346618 - Part 2. Test case.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1346618 - Part 1. Remove SVGImageContext::mGlobalOpacity.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Beta patch
4.27 KB, patch
gchang
: approval-mozilla-beta+
Details | Diff | Splinter Review
Attached file testcase.html — 
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/
Flags: needinfo?(d.huigens)
I believe this is not a regression. The bug is in Firefox 53, which is the first version that supports mask-image.
Flags: needinfo?(d.huigens)
Summary: When applying an mask-image which is an svg, opacity no longer works → When applying an mask-image which is an svg, opacity does not work
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
Blocks: 1323912
status-firefox52: --- → unaffected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
Keywords: regression
Version: Trunk → 53 Branch
Assignee: nobody → cku
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]:
Has Regression Range: --- → yes
Has STR: --- → yes
tracking-firefox53: --- → ?
tracking-firefox54: --- → ?
tracking-firefox55: --- → ?
(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.
Status: UNCONFIRMED → ASSIGNED
tracking-firefox53: ? → ---
tracking-firefox54: ? → ---
tracking-firefox55: ? → ---
Ever confirmed: true
Keywords: regression
Priority: -- → P1
Attachment #8846459 - Flags: review?(mstange)
Attachment #8846460 - Flags: review?(mstange)
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.
See Also: → 1348378
Comment on attachment 8846460 [details]
Bug 1346618 - Part 2. Test case.

https://reviewboard.mozilla.org/r/119482/#review124080
Attachment #8846460 - Flags: review?(mstange) → review+
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.
Attachment #8849481 - Flags: review?(mstange)
Attachment #8849482 - Flags: review?(mstange)
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.
Attachment #8846459 - Attachment is obsolete: true
Attachment #8846459 - Flags: review?(mstange)
Attachment #8849481 - Attachment is obsolete: true
Attachment #8849481 - Flags: review?(mstange)
Comment on attachment 8849482 [details]
Bug 1346618 - Part 1. Remove SVGImageContext::mGlobalOpacity.

https://reviewboard.mozilla.org/r/122264/#review132224
Attachment #8849482 - Flags: review?(mstange) → review+
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
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
Flags: needinfo?(cku)
Whiteboard: leave-open
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: leave-open
status-firefox55: affected → fixed
Target Milestone: --- → mozilla55
CJ, are you still intending to uplift this to beta 54?
status-firefox53: affected → wontfix
Flags: needinfo?(cku)
Attached patch Beta patch — — Splinter Review 

    
    

    
  
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
Flags: needinfo?(cku)

        Attachment #8871147 -
        Flags: approval-mozilla-beta?

    
    

    
  
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.

        Attachment #8871147 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+

    
    

    
  
(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.
Flags: qe-verify-

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: