Jonathan Watt [:jwatt] Reporter
	Description • 8 years ago

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

	Jonathan Watt [:jwatt] Reporter
	Comment 2 • 8 years ago

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

	Robert Longson [:longsonr]
	Comment 3 • 8 years ago

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?

	Jonathan Watt [:jwatt] Reporter
	Comment 4 • 8 years ago

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.

	u459114
	Comment 5 • 8 years ago

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

	Robert Longson [:longsonr]
	Comment 6 • 8 years ago

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

	Astley Chen (inactive)
	Comment 7 • 8 years ago

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

	Robert Longson [:longsonr]
	Comment 8 • 8 years ago

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

	Robert Longson [:longsonr]
	Comment 9 • 8 years ago

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

	Astley Chen (inactive)
	Comment 10 • 8 years ago

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

	u459114
	Comment 11 • 8 years ago

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.

	u459114
	Comment 12 • 8 years ago

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.

	u459114
	Comment 13 • 8 years ago

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

	u459114
	Comment 15 • 8 years ago

Patch in bug 1250490 also fix this bug.

	Robert Longson [:longsonr]
	Comment 16 • 8 years ago

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

	Markus Stange [:mstange]
	Comment 17 • 8 years ago

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

	Markus Stange [:mstange]
	Comment 18 • 8 years ago

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

	u459114
	Comment 19 • 8 years ago

(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

	u459114
	Comment 20 • 8 years ago

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

	Chris Mills [:cmills]
	Comment 21 • 7 years ago

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.

	Jonathan Watt [:jwatt] Reporter
	Comment 22 • 7 years ago

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?

	u459114
	Comment 23 • 7 years ago

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

	Robert Longson [:longsonr]
	Comment 24 • 7 years ago

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

	Robert Longson [:longsonr]
	Comment 25 • 7 years ago

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

	Robert Longson [:longsonr]
	Comment 26 • 7 years ago

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

	Robert Longson [:longsonr]
	Comment 28 • 7 years ago

I won't land the patch now.

	Louis Chang [:louis] Assignee
	Comment 29 • 7 years ago

Hi CJ,

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

	u459114
	Comment 30 • 7 years ago

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.

	Louis Chang [:louis] Assignee
	Comment 31 • 7 years ago

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.

	Louis Chang [:louis] Assignee
	Comment 32 • 7 years ago

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?

	u459114
	Comment 33 • 7 years ago

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.

	Louis Chang [:louis] Assignee
	Comment 34 • 7 years ago

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.

	Louis Chang [:louis] Assignee
	Comment 35 • 7 years ago

Address CJ's comment.

	Robert Longson [:longsonr]
	Comment 36 • 7 years ago

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

	Louis Chang [:louis] Assignee
	Comment 37 • 7 years ago

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

	Louis Chang [:louis] Assignee
	Comment 38 • 7 years ago

(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.)

	Jonathan Watt [:jwatt] Reporter
	Comment 39 • 7 years ago

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

	Jonathan Watt [:jwatt] Reporter
	Comment 40 • 7 years ago

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.

	Louis Chang [:louis] Assignee
	Comment 41 • 7 years ago

Carry over r=jwatt.

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

	Louis Chang [:louis] Assignee
	Comment 42 • 7 years ago

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

	Jonathan Watt [:jwatt] Reporter
	Comment 43 • 7 years ago

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.

	Pulsebot
	Comment 44 • 7 years ago

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

	Louis Chang [:louis] Assignee
	Comment 45 • 7 years ago

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

	Carsten Book [:Tomcat]
	Comment 46 • 7 years ago
bugherder

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

	Astley Chen (inactive)
	Comment 47 • 7 years ago

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

	Louis Chang [:louis] Assignee
	Comment 48 • 7 years ago

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

	Astley Chen (inactive)
	Comment 49 • 7 years ago

Thanks, Robert~