mask-mode:luminance doesn't work on gradient masks

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: dbaron, Assigned: u459114)

Tracking

(Blocks 1 bug, {css3})

Trunk
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 unaffected, firefox53 fixed, firefox54 fixed, firefox55 fixed)

Details

()

Attachments

(5 attachments)

Posted file simplified testcase
This was originally reported in https://twitter.com/iamvdo/status/840133084716584960 pointing to the testcase M2 in http://lab.iamvdo.me/css-svg-masks/#testM2 .

It appears that 'mask-mode: luminance' doesn't work on gradient masks.  In particular, while test M1 in the above page (using a gradient to transparent) works fine, test M2 shows no masking with a gradient that goes from white to black combined with mask-mode: luminance.

It seems to me that this ought to work.  (Though there's a bit of complexity as to the behavior of the initial value of mask-mode, match-source, that doesn't seem to be relevant here.)

In the simplified testcase, I'd expect only the top half of the square image to be visible instead of the whole image being visible, and the bottom half of that top half to be fading out.  The expected results can be observed by uncommenting the commented-out line.
Flags: needinfo?(ethlin)
Flags: needinfo?(cku)
Let me check
Assignee: nobody → cku
Flags: needinfo?(cku)
Is this a similar problem for -moz-element() that should also be fixed the same way?

>      RefPtr<gfxDrawable> drawable = DrawableForElement(aDest,
>                                                          aRenderingContext);
Flags: needinfo?(cku)
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #5)
> Is this a similar problem for -moz-element() that should also be fixed the
> same way?
> 
> >      RefPtr<gfxDrawable> drawable = DrawableForElement(aDest,
> >                                                          aRenderingContext);

It's ok to pass aRenderingContext. DrawableForElement use aRenderingContext by this way:
  aRenderingContext.ThebesContext()->GetDrawTarget()->CreateSimilarDrawTarget(...SurfaceFormat::B8G8R8A8);
So, pass aRenderingContext is the same with pass ctx. The real drawcall is located several lines bellow(nsLayoutUtils::DrawImage), which use ctx already.

But I think pass ctx to DrawableForElement make code more consistent.
And, I will also need a test case for -moz-element.
Flags: needinfo?(cku)
Attachment #8846470 - Flags: review?(mstange)
Attachment #8846538 - Flags: review?(mstange)
Attachment #8846472 - Flags: review?(mstange)
Flags: needinfo?(ethlin)
Comment on attachment 8846470 [details]
Bug 1346265 - Part 1. Pass gfxContext to nsCSSRendering::PaintGradient.

https://reviewboard.mozilla.org/r/119510/#review123064
Attachment #8846470 - Flags: review?(mstange) → review+
Comment on attachment 8846538 [details]
Bug 1346265 - Part 2. Pass gfxContext to nsImageRenderer::DrawableForElement.

https://reviewboard.mozilla.org/r/119598/#review123066

::: layout/painting/nsCSSRendering.h:295
(Diff revision 2)
>     * mImageElementSurface.
>     * Requires mType is eStyleImageType_Element.
>     * Returns null if we cannot create the drawable.
>     */
>    already_AddRefed<gfxDrawable> DrawableForElement(const nsRect& aImageRect,
> -                                                   nsRenderingContext&  aRenderingContext);
> +                                                   gfxContext&  aRenderingContext);

Please rename the argument from aRenderingContext to aContext here as well.
Attachment #8846538 - Flags: review?(mstange) → review+
Comment on attachment 8846472 [details]
Bug 1346265 - Part 3. Test cases.

https://reviewboard.mozilla.org/r/119512/#review123068

::: layout/reftests/image-element/mask-image-element-ref.html:27
(Diff revision 4)
> +        background-image: url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" width="100%" height="100%"><rect x="0" y="0" width="100%" height="100%" fill="blue" fill-opacity="1"/></svg>');
> +      }
> +
> +      div.luminance1 {
> +        left: 230px;
> +        background-image: url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" width="100%" height="100%"><rect x="0" y="0" width="100%" height="100%" fill="RGB(238,238,255)" fill-opacity="1"/></svg>');

I've never seen the rgb() color function spelled with uppercase letters before :)
Feel free to change it, doesn't really matter though.
Attachment #8846472 - Flags: review?(mstange) → review+
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fcdbf512a1be
Part 1. Pass gfxContext to nsCSSRendering::PaintGradient. r=mstange
https://hg.mozilla.org/integration/autoland/rev/d2a3dfa4e1d2
Part 2. Pass gfxContext to nsImageRenderer::DrawableForElement. r=mstange
https://hg.mozilla.org/integration/autoland/rev/d7b7b75a23af
Part 3. Test cases. r=mstange
Please do make approval requests to backport this, given that we haven't shipped the bug yet.
Flags: needinfo?(cku)
Comment on attachment 8848442 [details] [diff] [review]
Patch for 54/53

Approval Request Comment
[Feature/Bug causing the regression]: bug 1228354
[User impact if declined]: apply a gradient mask in luminance-mask-mode take no effect. 
[Is this code covered by automated tests?]: yes
[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]:no
[Is the change risky?]:no
[Why is the change risky/not risky?]: Pass manual test and try. 
[String changes made/needed]:N/A

Please considerate uplift attachment 8848442 [details] [diff] [review] (Patch for 54/53) to aurora and beta.
Flags: needinfo?(cku)
Attachment #8848442 - Flags: approval-mozilla-beta?
Attachment #8848442 - Flags: approval-mozilla-aurora?
Comment on attachment 8848442 [details] [diff] [review]
Patch for 54/53

Fix a layout issue and include tests. Aurora54+ & Beta53+.
Attachment #8848442 - Flags: approval-mozilla-beta?
Attachment #8848442 - Flags: approval-mozilla-beta+
Attachment #8848442 - Flags: approval-mozilla-aurora?
Attachment #8848442 - Flags: approval-mozilla-aurora+
Setting qe-verify- based on C.J. Ku's assessment on manual testing needs (see Comment 23) and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.