1246741 - Add more test case for basic shapes clipping in SVG content

Status: RESOLVED FIXED

(Core :: SVG, defect, P2)

Description

[Jonathan Watt [:jwatt]]

Bug 1075457 implements basic shape clipping, but not for SVG content. We should add support for SVG too.

Comment 1

[Robert Longson [:longsonr]]

Attachment: clip.txt

Comment 2

[Jonathan Watt [:jwatt]]

Comment on attachment 8720524 [details] [diff] [review]
clip.txt

Looks good, but this should land with more tests, at least for polygon.

 * percentage values
 * mixed absolute and percentage values
 * mixed other units (like clip-path-polygon-013.html)
 * fill-rule test
 * stroke-box, fill-box, view-box tests
 * some sort of elementFromPoint test

We should also start writing our reftests and JavaScript (aka Mochitest) tests for testing/web-platform/tests where possible (it should be possible in this case). See:

https://developer.mozilla.org/en-US/docs/Mozilla/QA/web-platform-tests

Comment 3

[Robert Longson [:longsonr]]

Attachment: meeting partway

i'll meet you partway on this but I'm not willing, nor do I have time to write so many tests.

 * percentage values
 * mixed absolute and percentage values
 * mixed other units (like clip-path-polygon-013.html)
 * fill-rule test

I've updated the reftest to cover all the above.

 * stroke-box, fill-box, view-box tests

I'm not going to do this, transform-box is preffed off. Raise something to track this if you want.

 * some sort of elementFromPoint test

This patch only covers rendering, raise another bug to cover this if you want, although the original html patch did seem to cover that.

I'll consider doing web-patform tests in future but I'm not going to rewrite this bugs tests. Can web-platform tests test preffed off features?

Comment 4

[Jonathan Watt [:jwatt]]

Sorry I'm taking a while to get to this. If the patch works in the other cases I mentioned I'm happy to r+ it, but I'll need to write those tests first. I hope to get to that soon but the security contexts work is rather pressing right now.

Comment 5

[u459114]

basic shapes clipping in SVG content is already supported in nightly, do we still need the patch here?

Comment 6

[Robert Longson [:longsonr]]

What makes you think it's supported? Have you tried the testcases in the patch without the code change that the patch contains?

Comment 7

[Astley Chen (inactive)]

Robert, what else left here before we are able to land your patch and resolve this bug ?

Comment 8

[Robert Longson [:longsonr]]

(In reply to Astley Chen [:astley] (UTC+8) from comment #7)
> Robert, what else left here before we are able to land your patch and
> resolve this bug ?

What it says in comment 3.

Comment 9

[Robert Longson [:longsonr]]

(In reply to Astley Chen [:astley] (UTC+8) from comment #7)
> Robert, what else left here before we are able to land your patch and
> resolve this bug ?

If you want to take over the bug and action that I'm quite happy to turn it over to you.

Comment 10

[Astley Chen (inactive)]

(In reply to Robert Longson from comment #9)
> If you want to take over the bug and action that I'm quite happy to turn it
> over to you.

Thanks~ Robert. Assigned to CJ who is able to help on this bug shortly.

Comment 11

[u459114]

Hmm, I will go back to bug 1250490 first.
In nsSVGIntegrationUtils, I had created a bunch of patches to re-factor nsSVGIntegrationUtils::PaintFrameWithEffects(been renamed to PaintMaskAndClipPath now).
Many functions/code are sharable between nsSVGIntegrationUtils::PaintFrameWithEffects & nsSVGUtils::PaintFrameWithEffects.

Comment 12

[u459114]

1. Test cases in clipPath-polygon-01.svg
   <!--
    Default reference box of clip-path is border-box.
    polygon(300px 100px,400px 100px,400px 200px,300px 200px) clip out
    whole rect, so you will not see this rect.
  -->
  <rect x="280" y="80" width="150" height="150" fill="lime"
   clip-path="polygon(300px 100px,400px 100px,400px 200px,300px 200px)"/>
  All the other test cases work correct.

2. I set a break point in nsSVGUtils::PaintFrameWithEffects, never been hit. All the test cases use nsSVGIntegrationUtils::PaintMaskAndClipPath to clip content.

Comment 13

[u459114]

To test this function(nsSVGUtils::PaintFrameWithEffects), the easiest way is to add a clip-path into a mask frame[1].

[1] http://searchfox.org/mozilla-central/source/layout/svg/nsSVGMaskFrame.cpp#269

Comment 14

[u459114]

Attachment: clip-path-in-mask.html

Comment 15

[u459114]

Patch in bug 1250490 also fix this bug.

Comment 16

[Robert Longson [:longsonr]]

Really? I would have thought this was at the very least neeeded for CSS clipping within masks, clip-paths and patterns.

Comment 17

[Markus Stange [:mstange]]

Yes, it's needed, and it's what bug 1250490 implements. The patches there add the missing functionality to nsSVGUtils::PaintFrameWithEffects.

Comment 18

[Markus Stange [:mstange]]

(In reply to Jonathan Watt [:jwatt] from comment #2)
> Comment on attachment 8720524 [details] [diff] [review]
> clip.txt
> 
> Looks good, but this should land with more tests, at least for polygon.
> 
>  * percentage values
>  * mixed absolute and percentage values
>  * mixed other units (like clip-path-polygon-013.html)
>  * fill-rule test
>  * stroke-box, fill-box, view-box tests
>  * some sort of elementFromPoint test
> 
> We should also start writing our reftests and JavaScript (aka Mochitest)
> tests for testing/web-platform/tests where possible (it should be possible
> in this case). See:

I realize now that we still haven't done this. CJ, would you mind writing these tests?

Comment 19

[u459114]

(In reply to Markus Stange [:mstange] from comment #18)
> (In reply to Jonathan Watt [:jwatt] from comment #2)
> > Comment on attachment 8720524 [details] [diff] [review]
> > clip.txt
> > 
> > Looks good, but this should land with more tests, at least for polygon.
> > 
> >  * percentage values
> >  * mixed absolute and percentage values
> >  * mixed other units (like clip-path-polygon-013.html)
> >  * fill-rule test
> >  * stroke-box, fill-box, view-box tests
> >  * some sort of elementFromPoint test
> > 
> > We should also start writing our reftests and JavaScript (aka Mochitest)
> > tests for testing/web-platform/tests where possible (it should be possible
> > in this case). See:
> 
> I realize now that we still haven't done this. CJ, would you mind writing
> these tests?
I already create some in Bug 1289011 for reference-box. For all the other, will do.
Reopen this one for creating more test cases

Comment 20

[u459114]

This bug should not be a blocker of shipping clip-path anymore

Comment 21

[Chris Mills [:cmills]]

I've checked out the MDN docs. We say that clip-path can be applied to all elements including SVG:

https://developer.mozilla.org/en-US/docs/Web/CSS/clip-path

And there's a note in the Fx52 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/52#CSS

Give me a shout if you think there's anything else that we need to do.

Comment 22

[Jonathan Watt [:jwatt]]

Comment on attachment 8724425 [details] [diff] [review]
meeting partway

Seems this bug has moved on without me noticing.

I'd still like us to move to web-platform-tests and stop using reftest/mochitest, but I'm sorry to have blocked this bug for far too long on that. :/ (I've also not been doing a good job of using web-platform-tests myself recently!)

r+ on the tests.

C.J. I'd still appreciate if you can fill in the missing permutations I described. Maybe mark this 'leave-open' if you land these tests, Robert?

Comment 23

[u459114]

In commnet 2, jwatt listed the test cases needed for basic shape clipping. These test cases target on SVG content only.

Comment 24

[Robert Longson [:longsonr]]

The code change part of this landed as part bug 1250490 (part 3). There's only the reftest left to land here.

https://hg.mozilla.org/try/rev/c85989d57b0c8f7278f5709bbd8b03dc9df5d2c7

Comment 25

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49bd01f782a01d35d26c45fdb0586179366fba81

Comment 26

[Robert Longson [:longsonr]]

One of the tests in the testcase now fails on the trunk. It passed when it was originally created so this is a regression.

Comment 27

[Robert Longson [:longsonr]]

Attachment: unbitrotted

Comment 28

[Robert Longson [:longsonr]]

I won't land the patch now.

Comment 29

[Louis Chang [:louis]]

Attachment: reftest_clippath_polygon.patch

Hi CJ,

Could you take a look on the patch?
The patch covers the test cases mentioned in comment 3.

Comment 30

[u459114]

Comment on attachment 8871133 [details] [diff] [review]
reftest_clippath_polygon.patch

Review of attachment 8871133 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/svg/clipPath-polygon-01.svg
@@ +15,5 @@
> +   clip-path="polygon(100px 100px,200px 100px,200px 200px,100px 200px)"/>
> +  <rect x="98" y="98" width="105" height="105" fill="lime"/>
> +
> +  <!-- test the clip does not simply make the element not render -->
> +  <rect x="300" y="100" width="100" height="100" fill="blue"/>

fill="red"

@@ +42,5 @@
> +  </svg>
> +
> +  <!-- test the clip does clip away black stroke based on fill-box -->
> +  <svg x="700" y="100" width="150" height="150">
> +    <rect x="10" y="10" width="100" height="100" stroke="black" stroke-width="10px" fill="lime"

stroke="red"

@@ +48,5 @@
> +  </svg>
> +
> +  <!-- test the clip does clip away black stroke based on stroke-box -->
> +  <svg x="500" y="300" width="150" height="150">
> +    <rect x="10" y="10" width="100" height="100" stroke="black" stroke-width="10px" fill="lime"

stroke="red"

@@ +53,5 @@
> +     clip-path="polygon(10px 10px,100px 10px,100px 100px,10px 100px) stroke-box"/>
> +  </svg>
> +
> +  <!-- test the clip does clip away black stroke based on view-box -->
> +  <svg x="700" y="300" width="150" height="150">

Do not exceed 800X1000
http://searchfox.org/mozilla-central/source/layout/tools/reftest/reftest.jsm#314

@@ +67,5 @@
> +      var elem = document.elementFromPoint(900, 100);
> +      elem.style.fill = "lime";
> +    })();
> +  </script>
> +</svg>

Please also check what had done in clip-path-{strokeBox|fillBox|viewBox}-XX.html. You may ignore some text cases here.

Comment 31

[Louis Chang [:louis]]

Attachment: Bug 1246741 - Add more test cases for basic shapes clipping in SVG content.

Hi CJ,

Could you please review the patch? Thanks.

I have fixed the patch based on you comments. 
In the patch, I remove the reference-box test cases since they were already done. And I add a test case for mixing with other units. Also separate test case elementFromPoint to another file.

Comment 32

[Louis Chang [:louis]]

Hi Jonathan,

I will cover following tests in this bug:
 * percentage values
 * mixed absolute and percentage values
 * mixed other units (like clip-path-polygon-013.html)
 * fill-rule test
 * some sort of elementFromPoint test
And rettest cases for stroke-box, fill-box, view-box and Mochitest were already done by CJ.

And maybe we open another bug for handling wpt tests for clip-path?

Comment 33

[u459114]

Comment on attachment 8871557 [details] [diff] [review]
Bug 1246741 - Add more test cases for basic shapes clipping in SVG content.

Review of attachment 8871557 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

::: layout/reftests/svg/clipPath-polygon-elementFromPoint-01.svg
@@ +13,5 @@
> +  <!-- test elementFromPoint can get the element using clip-path -->
> +  <rect x="100" y="100" width="100" height="100" fill="red"
> +   clip-path="polygon(0 0, 50px 0,50px 50px,0 50px)"/>
> +  <script>
> +    (function() {

call this function in svg::onload event.

Comment 34

[Louis Chang [:louis]]

Comment on attachment 8871557 [details] [diff] [review]
Bug 1246741 - Add more test cases for basic shapes clipping in SVG content.

Hi Jonathan,

Could you please review the patch? Thanks.

Comment 35

[Louis Chang [:louis]]

Attachment: Bug 1246741 - Add more test cases for basic shapes clipping in SVG content.

Address CJ's comment.

Comment 36

[Robert Longson [:longsonr]]

We should figure out why one of my unbitrotted testcases now fails and fix the issue it has uncovered.

Comment 37

[Louis Chang [:louis]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b3920986e1b153c5f4b45a9c9cef4a5c994c8d6

Comment 38

[Louis Chang [:louis]]

(In reply to Robert Longson from comment #36)
> We should figure out why one of my unbitrotted testcases now fails and fix
> the issue it has uncovered.

Hi Robert,

In your test case "test the clip does not simply make the element not render", it seems to me that the coordinate given to clip-path should based on your rect element, so giving polygon(300px 100px,400px 100px,400px 200px,300px 200px) may not clip out a correct region that can cover the blue area. It should be polygon(20px 20px,120px 20px,120px 120px,20px 120px), no?
I guess this is the reason why test failed? But I am not sure why your test case passed before.

And in your "percentage values" and "mixed units" test cases, there are typos in both fourth coordinate, they should be 0 50% and 0 50px if I don't misunderstand anything. (Although they do not effect the test.)

Comment 39

[Jonathan Watt [:jwatt]]

(In reply to Louis Chang [:louis] from comment #32)
> And maybe we open another bug for handling wpt tests for clip-path?

That seems like doing double the work. If you plan to add tests to wpt, why create reftest's at all?

Comment 40

[Jonathan Watt [:jwatt]]

Comment on attachment 8871577 [details] [diff] [review]
Bug 1246741 - Add more test cases for basic shapes clipping in SVG content.

Review of attachment 8871577 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/svg/clipPath-polygon-01.svg
@@ +11,5 @@
> +  <rect width="100%" height="100%" fill="lime"/>
> +
> +  <!-- test the clip is not too big (or ignored altogether) -->
> +  <rect width="100%" height="100%" fill="red"
> +   clip-path="polygon(100px 100px,200px 100px,200px 200px,100px 200px)"/>

Indenting attributes by a single space from the opening angle bracket makes it a little harder to see skim quickly and see where tags start and end. I'd suggest if the tag indentation is 2 spaces then wrapped attribute indentation should be four spaces. Not that big a deal for one test, but maybe something to bear in mind for future?

::: layout/reftests/svg/clipPath-polygon-elementFromPoint-01.svg
@@ +11,5 @@
> +  <rect width="100%" height="100%" fill="lime"/>
> +
> +  <!-- test elementFromPoint can get the element using clip-path -->
> +  <rect x="100" y="100" width="100" height="100" fill="red"
> +   clip-path="polygon(0 0, 50px 0,50px 50px,0 50px)"/>

The comma positioning here is not what I'm used to. Usually the structure people seem to use is to put the comma between related x and y coordinates, and put spaces between the pairs. If you want to structure it this way can you also put a space after the commas so the coordinate that comes after it doesn't appear to be connected to the one before it?

@@ +14,5 @@
> +  <rect x="100" y="100" width="100" height="100" fill="red"
> +   clip-path="polygon(0 0, 50px 0,50px 50px,0 50px)"/>
> +  <script>
> +    function testElementFromPoint() {
> +      var elem = document.elementFromPoint(100, 100);

I think you should check both that parts of the element inside the clip path are hit, and that parts of the element outside the clip path are not hit. Personally I'd probably have tested 8 points - 4 points 1px outside the edge of the clip path, and 4 points 1px inside it.

Comment 41

[Louis Chang [:louis]]

Attachment: Bug 1246741 - Add more test cases for basic shapes clipping in SVG content. r=jwatt

Carry over r=jwatt.

The patch fixes the coding style and add more test points for elementFromPoint test based on Jonathan's comments.

Comment 42

[Louis Chang [:louis]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=41a96fccdaccc3a65928f168409fc3d00b1e46af

Comment 43

[Jonathan Watt [:jwatt]]

Comment on attachment 8875601 [details] [diff] [review]
Bug 1246741 - Add more test cases for basic shapes clipping in SVG content. r=jwatt

Review of attachment 8875601 [details] [diff] [review]:
-----------------------------------------------------------------

Since you asked me on IRC to take another look here are my comments. (I think you've spent long enough on this so I wouldn't bother fixing the spacing, but consider it in future tests you write. I think you should fix the console error though.

::: layout/reftests/svg/clipPath-polygon-01.svg
@@ +11,5 @@
> +  <rect width="100%" height="100%" fill="lime"/>
> +
> +  <!-- test the clip is not too big (or ignored altogether) -->
> +  <rect width="100%" height="100%" fill="red"
> +        clip-path="polygon(100px 100px,200px 100px,200px 200px,100px 200px)"/>

What I was talking about regarding spacing would be to change:

  polygon(100px 100px,200px 100px,200px 200px,100px 200px)

to:

  polygon(100px 100px, 200px 100px, 200px 200px, 100px 200px)

::: layout/reftests/svg/clipPath-polygon-elementFromPoint-01.svg
@@ +14,5 @@
> +  <rect id="in" x="100" y="100" width="100" height="100"
> +        clip-path="polygon(0 0,50px 0,50px 50px,0 50px)"/>
> +  <script>
> +    function testElementFromPoint() {
> +      let inCount = outCount = 0, inElem, outElem;

This will cause an error something like "ReferenceError: assignment to undeclared variable outCount" to be sent to the console. It would be good to avoid that since the more junk that gets sent to the console the slower builds are.

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/128c9d3bc047
Add more test cases for basic shapes clipping in SVG content. r=jwatt

Comment 45

[Louis Chang [:louis]]

(In reply to Jonathan Watt [:jwatt] from comment #43)
> Comment on attachment 8875601 [details] [diff] [review]
> 
> This will cause an error something like "ReferenceError: assignment to
> undeclared variable outCount" to be sent to the console. It would be good to
> avoid that since the more junk that gets sent to the console the slower
> builds are.

Ok, I will open a bug to fix this and the spacing issue.

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/128c9d3bc047

Comment 47

[Astley Chen (inactive)]

(In reply to Louis Chang [:louis] from comment #45)
> Ok, I will open a bug to fix this and the spacing issue.

Louis, have you opened a bug to follow up?

Comment 48

[Louis Chang [:louis]]

(In reply to Astley Chen [:astley] (UTC+8) from comment #47)
> (In reply to Louis Chang [:louis] from comment #45)
>
> Louis, have you opened a bug to follow up?

yep, bug 1371558 was already landed.

Comment 49

[Astley Chen (inactive)]

Thanks, Robert~