Closed
Bug 1346265
Opened 7 years ago
Closed 7 years ago
mask-mode:luminance doesn't work on gradient masks
Categories
(Core :: Web Painting, defect)
Core
Web Painting
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox52 | --- | unaffected |
firefox53 | --- | fixed |
firefox54 | --- | fixed |
firefox55 | --- | fixed |
People
(Reporter: dbaron, Assigned: u459114)
References
(Blocks 1 open bug, )
Details
(Keywords: css3)
Attachments
(5 files)
284 bytes,
text/html
|
Details | |
59 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
12.09 KB,
patch
|
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 5•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Attachment #8846470 -
Flags: review?(mstange)
Attachment #8846538 -
Flags: review?(mstange)
Attachment #8846472 -
Flags: review?(mstange)
Updated•7 years ago
|
Flags: needinfo?(ethlin)
Comment 13•7 years ago
|
||
mozreview-review |
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 14•7 years ago
|
||
mozreview-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 15•7 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•7 years ago
|
||
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
Assignee | ||
Comment 20•7 years ago
|
||
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fcdbf512a1be https://hg.mozilla.org/mozilla-central/rev/d2a3dfa4e1d2 https://hg.mozilla.org/mozilla-central/rev/d7b7b75a23af
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Reporter | ||
Comment 22•7 years ago
|
||
Please do make approval requests to backport this, given that we haven't shipped the bug yet.
Flags: needinfo?(cku)
Assignee | ||
Comment 23•7 years ago
|
||
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 24•7 years ago
|
||
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+
Comment 25•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/70d9504d91c469a8b367f5f5182fdf4ec1c375a2
Comment 26•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/8791447e10db25249a554d6cb474aaf47435089f
Comment 27•7 years ago
|
||
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.
Description
•