Add more test case for basic shapes clipping in SVG content

RESOLVED FIXED in Firefox 55

Status

()

defect
P2
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jwatt, Assigned: lochang)

Tracking

(Blocks 1 bug, {dev-doc-complete})

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 affected, firefox55 fixed)

Details

Attachments

(3 attachments, 5 obsolete attachments)

Reporter

Description

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

Updated

3 years ago
Posted patch clip.txt (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attachment #8720524 - Flags: review?(jwatt)
Reporter

Comment 2

3 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
Attachment #8720524 - Flags: review?(jwatt) → feedback+
Posted patch meeting partway (obsolete) — Splinter Review
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?
Attachment #8724425 - Flags: review?(jwatt)
Attachment #8720524 - Attachment is obsolete: true
Reporter

Updated

3 years ago
Attachment #8724425 - Flags: review?(jwatt) → review+
Reporter

Updated

3 years ago
Attachment #8724425 - Attachment is patch: true
Attachment #8724425 - Flags: review+ → review?(jwatt)
Reporter

Comment 4

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

Comment 5

3 years ago
basic shapes clipping in SVG content is already supported in nightly, do we still need the patch here?
What makes you think it's supported? Have you tried the testcases in the patch without the code change that the patch contains?
Robert, what else left here before we are able to land your patch and resolve this bug ?
Flags: needinfo?(longsonr)
Priority: -- → P3
(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.
Flags: needinfo?(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.
Blocks: 1303654
Flags: needinfo?(aschen)
Assignee: longsonr → nobody
(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.
Assignee: nobody → cku
Status: NEW → ASSIGNED
Flags: needinfo?(aschen)
Priority: P3 → P2

Comment 11

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

Comment 12

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

Comment 13

3 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

Comment 14

3 years ago

Updated

3 years ago
Depends on: 1250490

Comment 15

3 years ago
Patch in bug 1250490 also fix this bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1250490
Really? I would have thought this was at the very least neeeded for CSS clipping within masks, clip-paths and patterns.
Yes, it's needed, and it's what bug 1250490 implements. The patches there add the missing functionality to nsSVGUtils::PaintFrameWithEffects.
(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

3 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
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---

Comment 20

3 years ago
This bug should not be a blocker of shipping clip-path anymore
No longer blocks: basic-shape-ship, 1303654
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.
Reporter

Comment 22

2 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?
Attachment #8724425 - Flags: review?(jwatt) → review+

Updated

2 years ago
Summary: Support basic shapes clipping in SVG content → Add more test case for basic shapes clipping in SVG content

Comment 23

2 years ago
In commnet 2, jwatt listed the test cases needed for basic shape clipping. These test cases target on SVG content only.
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
Keywords: leave-open
One of the tests in the testcase now fails on the trunk. It passed when it was originally created so this is a regression.
Posted patch unbitrottedSplinter Review
Attachment #8724425 - Attachment is obsolete: true
I won't land the patch now.

Updated

2 years ago
Assignee: cku → lochang
Assignee

Comment 29

2 years ago
Hi CJ,

Could you take a look on the patch?
The patch covers the test cases mentioned in comment 3.
Attachment #8871133 - Flags: feedback?(cku)

Comment 30

2 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.
Attachment #8871133 - Flags: feedback?(cku) → feedback+
Assignee

Comment 31

2 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.
Attachment #8871133 - Attachment is obsolete: true
Attachment #8871557 - Flags: review?(cku)
Assignee

Comment 32

2 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?
Flags: needinfo?(jwatt)

Updated

2 years ago
Attachment #8871557 - Attachment is patch: true

Comment 33

2 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.
Attachment #8871557 - Flags: review?(cku) → review+
Assignee

Comment 34

2 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.
Attachment #8871557 - Flags: review+ → review?(jwatt)
Assignee

Comment 35

2 years ago
Address CJ's comment.
Attachment #8871557 - Attachment is obsolete: true
Attachment #8871557 - Flags: review?(jwatt)
Attachment #8871577 - Flags: review?(jwatt)
We should figure out why one of my unbitrotted testcases now fails and fix the issue it has uncovered.
Assignee

Comment 38

2 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.)
Reporter

Updated

2 years ago
Attachment #8871577 - Attachment is patch: true
Flags: needinfo?(jwatt)
Reporter

Comment 39

2 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?
Reporter

Comment 40

2 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.
Attachment #8871577 - Flags: review?(jwatt) → review+
Assignee

Comment 41

2 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.
Attachment #8871577 - Attachment is obsolete: true
Attachment #8875601 - Flags: review+
Assignee

Updated

2 years ago
Reporter

Comment 43

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

Comment 44

2 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
Keywords: checkin-needed
Assignee

Comment 45

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

Comment 46

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/128c9d3bc047
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(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?
Flags: needinfo?(lochang)
Assignee

Comment 48

2 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.
Flags: needinfo?(lochang)
Thanks, Robert~
You need to log in before you can comment on or make changes to this bug.