Closed
Bug 1319667
Opened 7 years ago
Closed 7 years ago
Enable mask longhands on SVG elements
Categories
(Core :: Layout, defect, P2)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla53
| Tracking | Status | |
|---|---|---|
| firefox53 | --- | fixed |
People
(Reporter: u459114, Assigned: u459114)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
According to the spec https://drafts.fxtf.org/css-masking-1/#the-mask-image Name: mask-image Applies to: All elements. In SVG, it applies to container elements excluding the <defs> element and all graphics elements All mask longhands should be able to apply to svg elements. We currently only allow SVG-mask apply onto svg element: http://searchfox.org/mozilla-central/source/layout/svg/nsSVGUtils.cpp#528 Turning it on is not too difficult, test cases needed.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
It may not be a blocker of shipping positioned mask since neither safari or chrome support it. But it does a blocker of Bug 1303623 and Bug 1311270. mask-clip:fill-box/stroke-box/view-box takes effect only when applied to an SVG element. We can not even test the solution in 1311270 without fix here.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 8•7 years ago
|
||
Thanks for opening this bug and making CSS masking support on Firefox more complete.
Status: NEW → ASSIGNED
Priority: -- → P2
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
Attachment #8813540 -
Flags: review?(mstange)
Attachment #8813742 -
Flags: review?(mstange)
Attachment #8813541 -
Flags: review?(mstange)
|
||
Comment 14•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8813540 [details] Bug 1319667 - Part 1. Allow mask longhands apply to SVG elements. https://reviewboard.mozilla.org/r/94990/#review95302 ::: layout/svg/nsSVGIntegrationUtils.cpp:601 (Diff revision 5) > + // we have any item in maskFrame even if all of those items are > + // non-resolvable <mask-sources> or <images>. > + // Set paintResult.transparentBlackMask as true, the caller should stop > + // painting masked content as if this mask is a transparent black one. > + // For a SVG doc: > + // SVG 1.1 say that if we fail to resolve a mask, we should draw the there's two spaces between "that" and "if"
Attachment #8813540 -
Flags: review?(mstange) → review+
|
|
||
Comment 15•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8813742 [details] Bug 1319667 - Part 2. Move DrawResult from function parameter of PaintClipMask to the return value. https://reviewboard.mozilla.org/r/95132/#review95310
Attachment #8813742 -
Flags: review?(mstange) → review+
|
|
||
Comment 16•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8813541 [details] Bug 1319667 - Part 3. Test case for mask longhands on svg element. https://reviewboard.mozilla.org/r/94992/#review95312
Attachment #8813541 -
Flags: review?(mstange) → review+
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 21•7 years ago
|
||
Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/464230532ba3 Part 1. Allow mask longhands apply to SVG elements. r=mstange https://hg.mozilla.org/integration/autoland/rev/3ab240e653d8 Part 2. Move DrawResult from function parameter of PaintClipMask to the return value. r=mstange https://hg.mozilla.org/integration/autoland/rev/d9b26b438895 Part 3. Test case for mask longhands on svg element. r=mstange
|
||
Comment 22•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/464230532ba3 https://hg.mozilla.org/mozilla-central/rev/3ab240e653d8 https://hg.mozilla.org/mozilla-central/rev/d9b26b438895
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
|
|
||
Comment 23•7 years ago
|
||
Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c63c761e089c (follow-up) Remove incorrect assertion. r=me
|
||
Comment 24•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/c63c761e089c
|
||
Comment 25•7 years ago
|
||
C.J. Ku, does this feature have automated coverage? Would it benefit from manual testing?
Flags: qe-verify?
Flags: needinfo?(cku)
|
Assignee | |
Comment 26•7 years ago
|
||
S(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #25) > C.J. Ku, does this feature have automated coverage? Would it benefit from > manual testing? There is a ref-test case in Part 3. May I know which kind of test that you asking?
Flags: needinfo?(cku)
|
|
||
Comment 27•7 years ago
|
||
(In reply to C.J. Ku[:cjku](UTC+8) from comment #26) > S(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #25) > > C.J. Ku, does this feature have automated coverage? Would it benefit from > > manual testing? > There is a ref-test case in Part 3. May I know which kind of test that you > asking? Sure, I was just wondering if manual QA should be tracking this bug on Fx53. But since it has automated coverage, in the form of reftests, I think it's not necessary (or at least not as necessary as for other bug fixes that are currently riding Fx53). Thank you for following up!
Flags: qe-verify? → qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•