1319667 - Enable mask longhands on SVG elements

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[u459114]

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 1

[u459114]

Attachment: Bug 1319667 - Part 1. Allow mask longhands apply to SVG elements.

Review commit: https://reviewboard.mozilla.org/r/94990/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94990/

Comment 2

[u459114]

Attachment: Bug 1319667 - Part 3. Test case for mask longhands on svg element.

Review commit: https://reviewboard.mozilla.org/r/94992/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94992/

Comment 3

[u459114]

Comment on attachment 8813540 [details]
Bug 1319667 - Part 1. Allow mask longhands apply to SVG elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94990/diff/1-2/

Comment 4

[u459114]

Comment on attachment 8813541 [details]
Bug 1319667 - Part 3. Test case for mask longhands on svg element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94992/diff/1-2/

Comment 5

[u459114]

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 6

[u459114]

Comment on attachment 8813540 [details]
Bug 1319667 - Part 1. Allow mask longhands apply to SVG elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94990/diff/2-3/

Comment 7

[u459114]

Comment on attachment 8813541 [details]
Bug 1319667 - Part 3. Test case for mask longhands on svg element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94992/diff/2-3/

Comment 8

[Astley Chen (inactive)]

Thanks for opening this bug and making CSS masking support on Firefox more complete.

Comment 9

[u459114]

Comment on attachment 8813540 [details]
Bug 1319667 - Part 1. Allow mask longhands apply to SVG elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94990/diff/3-4/

Comment 10

[u459114]

Comment on attachment 8813541 [details]
Bug 1319667 - Part 3. Test case for mask longhands on svg element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94992/diff/3-4/

Comment 11

[u459114]

Attachment: Bug 1319667 - Part 2. Move DrawResult from function parameter of PaintClipMask to the return value.

Enlight by "Bug 1314536 - Part 1"

Review commit: https://reviewboard.mozilla.org/r/95132/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95132/

Comment 12

[u459114]

Comment on attachment 8813540 [details]
Bug 1319667 - Part 1. Allow mask longhands apply to SVG elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94990/diff/4-5/

Comment 13

[u459114]

Comment on attachment 8813541 [details]
Bug 1319667 - Part 3. Test case for mask longhands on svg element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94992/diff/4-5/

Comment 14

[Markus Stange [:mstange]]

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"

Comment 15

[Markus Stange [:mstange]]

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

Comment 16

[Markus Stange [:mstange]]

Comment on attachment 8813541 [details]
Bug 1319667 - Part 3. Test case for mask longhands on svg element.

https://reviewboard.mozilla.org/r/94992/#review95312

Comment 17

[u459114]

Comment on attachment 8813540 [details]
Bug 1319667 - Part 1. Allow mask longhands apply to SVG elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94990/diff/5-6/

Comment 18

[u459114]

Comment on attachment 8813742 [details]
Bug 1319667 - Part 2. Move DrawResult from function parameter of PaintClipMask to the return value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95132/diff/1-2/

Comment 19

[u459114]

Comment on attachment 8813541 [details]
Bug 1319667 - Part 3. Test case for mask longhands on svg element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94992/diff/5-6/

Comment 20

[u459114]

Comment on attachment 8813541 [details]
Bug 1319667 - Part 3. Test case for mask longhands on svg element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94992/diff/6-7/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/464230532ba3
https://hg.mozilla.org/mozilla-central/rev/3ab240e653d8
https://hg.mozilla.org/mozilla-central/rev/d9b26b438895

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c63c761e089c
(follow-up) Remove incorrect assertion. r=me

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c63c761e089c

Comment 25

[Andrei Vaida [:avaida]]

C.J. Ku, does this feature have automated coverage? Would it benefit from manual testing?

Comment 26

[u459114]

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?

Comment 27

[Andrei Vaida [:avaida]]

(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!