Closed Bug 1246762 Opened 8 years ago Closed 8 years ago

Add support for |clip-path: inset()|

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox47 --- affected
firefox51 --- fixed

People

(Reporter: jwatt, Assigned: manishearth)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(3 files)

Bug 1246762 - Add support for inset() in clip-path ;
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1246762 - Add reftests for non-rounded |clip-path: inset()|s ;
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1246762 - Add reftests for |clip-path: inset()|s with rounded corners ;
58 bytes, text/x-review-board-request
dholbert
: review+
Details 
Bug 1075457 adds basic support for basic shape clip paths, but it does not add support for inset(). We should add that.
Blocks: basic-shape-ship
Have a patch for this, will post soon.

Let me know if I shouldn't self-assign here.
Assignee: jwatt → manishearth
Status: NEW → ASSIGNED
No tests yet. I'll add a bunch to /layout/reftests/svg/svg-integration/clip-path/ soon (does this need to be in the same bug, or can it be a followup?). It does work, however.

I'm not sure how to write the refs for the tests involving round insets. I guess using border-radius on a filled div would work. The other refs all use `url()` clip-path to svg <clipPath> elements, but there's no `inset` shape in SVG. <rect> with rounded corners may work too.

We probably should move these tests into wpt or csswg at some point, but right now we're the only browser that fully implements clip-path (with this patch), so I'm not sure if this should be done yet. Chrome implements everything (I think) but uses vendor prefixes.
Attachment #8782403 - Flags: review?(xidorn+moz) → review?(dholbert)
I'm not familiar with SVG (and also not a peer of that part). Redirect to :dholbert.
Added some tests. The rounded corners ones are failing, and the analyzer is circling the corners. But I don't see much of a difference (if any), even when zooming in. Is this a mistake in my code, or is this just because border-radius will be drawn slightly differently from clip-path inset? Perhaps the test should be made fuzzier?


Reftest log for layout/reftests/svg/svg-integration/clip-path/clip-path-inset-002-a.html at https://manishearth.pastebin.mozilla.org/8902142 , analyser screenshot at http://i.stack.imgur.com/M1pVk.png
I suspect this might just be AA ruining our day. I can't see a difference (reftest analyser's pixel different thing isn't working for me) even when I zoom, so it's probably pixels with slightly different colors. I guess the AA behavior of round borders is different from that of round masks.
Added a test for the full rounded corners syntax (different radii for all 8 components), and made all the rounded corner tests fuzzy.

It occurs to me that we could get rid of the fuzziness by trying to create rounded rects in svg and use those as url clip paths, but I suspect border-radius will cause similar issues there.
Comment on attachment 8782461 [details]
Bug 1246762 - Add reftests for non-rounded |clip-path: inset()|s ;

https://reviewboard.mozilla.org/r/72628/#review70928

Thanks for fixing this!

A few things to clean up / tweak in the tests here.  Nits below -- I'm posting them for the first set of reftests, but they apply to both of your reftest patches here.

::: layout/reftests/svg/svg-integration/clip-path/clip-path-inset-001-a.html:10
(Diff revision 1)
> +<!DOCTYPE html>
> +<html>
> +<head>
> +  <title>CSS Masking: Test clip-path property and inset function with 10px on all corners</title>
> +  <link rel="help" href="https://drafts.csswg.org/css-shapes/#typedef-basic-shape">
> +  <link rel="match" href="clip-path-inset-001-ref.html">

As long as you're including these CSSWG-test-suite headers, could you include <link rel="author"> in each of these files as well? That's the only one you're missing here, I think.

(Related: ideally these tests should go some subdir within layout/reftests/w3c-css/submitted -- BUT, apparently our other clip-path reftests don't live there right now -- so it seems fine to put them where you're putting them now, with the other clip-path reftests, and we can move all the tests at a later point.)

::: layout/reftests/svg/svg-integration/clip-path/clip-path-inset-001-b.html:14
(Diff revision 1)
> +  <link rel="help" href="https://drafts.csswg.org/css-shapes/#typedef-basic-shape">
> +  <link rel="match" href="clip-path-inset-001-ref.html">
> +</head>
> +<body>
> +  <p>The test passes if there is a green square not touching the edges</p>
> +  <div style="width: 120px; height: 100px; border: solid green 50px; background-color: green; clip-path: inset(10px 20px);position:relative;left:-10px;"></div>

Nit: This line is hugely long, and it has mix of spacing styles (lots of spaces around ":" and ";" towards the beginning, none towards the end)

This makes the individual test hard to read, *and* it makes it hard to diff this test against its variants and against the reference, to see how they differ & what's actually being tested.

SO: could you just move all these styles to a <style> block, for each of these tests? (with one declaration per line in that <style> block)  Then the CSS will be more readable and diffable.

::: layout/reftests/svg/svg-integration/clip-path/clip-path-inset-001-ref.html:8
(Diff revision 1)
> +     http://creativecommons.org/publicdomain/zero/1.0/
> +-->
> +<!DOCTYPE html>
> +<html>
> +<head>
> +  <title>CSS Masking: Ref test for clip-path's inset function 001</title>

s/Ref test/Reference/ (or "Reference case")

(We use "reftest" (as in "please add a reftest") to refer to the *combination* of the testcase/reference pair.  So, labeling this particular file the "Ref test" is not quite accurate.)

::: layout/reftests/svg/svg-integration/clip-path/reftest.list:53
(Diff revision 1)
>  == clip-path-ellipse-007.html clip-path-ellipse-001-ref.html
>  == clip-path-ellipse-008.html clip-path-ellipse-001-ref.html
> +
> +== clip-path-inset-001-a.html clip-path-inset-001-ref.html
> +== clip-path-inset-001-b.html clip-path-inset-001-ref.html
> +== clip-path-inset-001-c.html clip-path-inset-001-ref.html

Please remove the last hyphen from the testcase files -- they should be "001a", not "001-a".

(This requirement is documented at https://wiki.csswg.org/test/format -- "There may also be a letter affixed after the number, which can be used to indicate variants of a test. For example, float-wrap-001l.xht".  Hopefully that's documented in the newer testthewebforward reftest documentation, too, but I couldn't find it offhand -- but it is the convention that we follow.)
Comment on attachment 8782461 [details]
Bug 1246762 - Add reftests for non-rounded |clip-path: inset()|s ;

https://reviewboard.mozilla.org/r/72628/#review70932

r- on the reftests for now, but likely r+ with my just-posted nits addressed. (Particularly after the test files are more usefully diffable, they'll be easier to compare & post any final review suggestions on.)
Attachment #8782461 - Flags: review?(dholbert) → review-
Comment on attachment 8782462 [details]
Bug 1246762 - Add reftests for |clip-path: inset()|s with rounded corners ;

https://reviewboard.mozilla.org/r/72630/#review70936

(my above review comments apply to this second patch, plus one more nit for what looks like some missing files. r- for now.)

::: layout/reftests/svg/svg-integration/clip-path/reftest.list:58
(Diff revision 2)
>  == clip-path-inset-001-c.html clip-path-inset-001-ref.html
> +# Anti-aliasing behavior for masking and borders is different
> +fuzzy(30,200) == clip-path-inset-002-a.html clip-path-inset-002-ref.html
> +fuzzy(30,200) == clip-path-inset-002-b.html clip-path-inset-002-ref.html
> +fuzzy(30,200) == clip-path-inset-002-c.html clip-path-inset-002-ref.html
> +fuzzy(40,300) == clip-path-inset-003.html clip-path-inset-003-ref.html

You've got a line for a "003.html"/"003-ref.html" set of files in the manifest here, but I don't see those files in this patch. Perhaps you forgot to "hg add" them?
Attachment #8782462 - Flags: review?(dholbert) → review-
Comment on attachment 8782403 [details]
Bug 1246762 - Add support for inset() in clip-path ;

https://reviewboard.mozilla.org/r/72592/#review70926

This looks pretty good -- but it needs a bit of work to better match (& make use of) existing code. r- for now, since it's worth another look once you've addressed the review feedback.

::: layout/svg/nsCSSClipPathInstance.h:49
(Diff revision 2)
>  
>    already_AddRefed<Path> CreateClipPathPolygon(DrawTarget* aDrawTarget,
>                                                 const nsRect& aRefBox);
>  
> +  already_AddRefed<Path> CreateClipPathInset(DrawTarget* aDrawTarget,
> +                                               const nsRect& aRefBox);

Please fix the indentation here so the args line up (as with the decls above this one).

::: layout/svg/nsCSSClipPathInstance.cpp:257
(Diff revision 2)
> +
> +  nscoord appUnitsPerDevPixel =
> +    mTargetFrame->PresContext()->AppUnitsPerDevPixel();
> +  nscoord top = nsRuleNode::ComputeCoordPercentCalc(coords[0], aRefBox.height);
> +  nscoord right = nsRuleNode::ComputeCoordPercentCalc(coords[1], aRefBox.width);
> +  nscoord bottom = nsRuleNode::ComputeCoordPercentCalc(coords[2],

Please declare these as a single nsMargin object like so:

  nsMargin inset(nsRuleNode::ComputeCoordPercentCalc(coords[0], aRefBox.height),
                 nsRuleNode::ComputeCoordPercentCalc(coords[1], aRefBox.width),
                 nsRuleNode::ComputeCoordPercentCalc(coords[2], aRefBox.height),
                 nsRuleNode::ComputeCoordPercentCalc(coords[3], aRefBox.width));

That groups them together nicely, and it makes all of the ComputeCoordPercentCalc calls line up nicely for easier quick-comparison of the args that we're passing to each one.

::: layout/svg/nsCSSClipPathInstance.cpp:262
(Diff revision 2)
> +  nscoord bottom = nsRuleNode::ComputeCoordPercentCalc(coords[2],
> +                                                       aRefBox.height);
> +  nscoord left = nsRuleNode::ComputeCoordPercentCalc(coords[3], aRefBox.width);
> +
> +  Point anchor =
> +      Point(aRefBox.x + left, aRefBox.y + top) / appUnitsPerDevPixel;

Wrong indentation here (4 spaces beyond the line above it -- should be 2 space indented).

(But I think this line will be going away anyway, as noted below; hence, not filing a MozReview "issue" for this one.)

::: layout/svg/nsCSSClipPathInstance.cpp:265
(Diff revision 2)
> +
> +  Point anchor =
> +      Point(aRefBox.x + left, aRefBox.y + top) / appUnitsPerDevPixel;
> +  Size size = Size(aRefBox.width - left - right,
> +                   aRefBox.height - top - bottom) / appUnitsPerDevPixel;
> +  Rect rect(anchor, size);

No need to independently calculate anchor/size/rect here.  You could do:
  nsRect insetRect = aRefBox.Deflate(inset);
  Rect insetGfxRect = NSRectToRect(insetRect);
...but probably better to just do it as a one-liner:
  Rect insetRect = NSRectToRect(aRefBox.Deflate(inset));

::: layout/svg/nsCSSClipPathInstance.cpp:277
(Diff revision 2)
> +      int cx = NS_FULL_TO_HALF_CORNER(corner, false);
> +      int cy = NS_FULL_TO_HALF_CORNER(corner, true);
> +      nscoord x = nsRuleNode::ComputeCoordPercentCalc(radius.Get(cx),
> +                                                      aRefBox.width);
> +      nscoord y = nsRuleNode::ComputeCoordPercentCalc(radius.Get(cy),
> +                                                      aRefBox.height);

Ideally, this should share code with nsIFrame::ComputeBorderRadii somehow (and the border-radius codepath in general).

That means:
 - We should perhaps refactor the logic in nsIFrame::ComputeBorderRadii to make it more suitable for calling here (or maybe we can just call it directly)
 - We should be representing the radii as an array of 8 nscoord values here, *not* as an array of 4 gfx::Size objects.
 - We should convert our nscoord array into a clip path the same way that we do for "border-radius" -- from a bit of code-following, I believe that means we should match DisplayItemClip::MakeRoundedRectPath and call nsCSSRendering::ComputePixelRadii() to produce our RectCornerRadii.

(With any luck, this might reduce the need for fuzz in the tests, too, since we'll be more consistent in terms of rounding/dividing behavior.)
Attachment #8782403 - Flags: review?(dholbert) → review-
Fuzz still necessary, sadly. I think it's because of AA behavior, which is probably going to differ between borders and masking.
Comment on attachment 8782403 [details]
Bug 1246762 - Add support for inset() in clip-path ;

https://reviewboard.mozilla.org/r/72592/#review71100

r=me on this part with 2 nits addressed:

::: layout/svg/nsCSSClipPathInstance.h:49
(Diff revision 3)
>  
>    already_AddRefed<Path> CreateClipPathPolygon(DrawTarget* aDrawTarget,
>                                                 const nsRect& aRefBox);
>  
> +  already_AddRefed<Path> CreateClipPathInset(DrawTarget* aDrawTarget,
> +                                               const nsRect& aRefBox);

Indentation is off here, as noted in my previous round of review comments. Please fix.

::: layout/svg/nsCSSClipPathInstance.cpp:263
(Diff revision 3)
> +                 nsRuleNode::ComputeCoordPercentCalc(coords[2], aRefBox.height),
> +                 nsRuleNode::ComputeCoordPercentCalc(coords[3], aRefBox.width));
> +
> +  nsRect insetRect(aRefBox);
> +  insetRect.Deflate(inset);
> +  const Rect gfxRect = NSRectToRect(insetRect, appUnitsPerDevPixel);

Please rename this variable: s/gfxRect/insetRectPixels/

Two reasons:
 (1) gfxRect is a type-name:
https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxRect.h#60
...so it (clearly) shouldn't be used as a variable-name, and it's also confusing if we have "gfx" in the name of this variabale at all when its type is not actually |gfxRect|.  (Sorry, I did suggest using "Gfx" in the name above - that was my mistake.)

 (2) It's nice to clarify what this rect represents. (hence, adding "inset" to the name)
Attachment #8782403 - Flags: review?(dholbert) → review+
Comment on attachment 8782461 [details]
Bug 1246762 - Add reftests for non-rounded |clip-path: inset()|s ;

https://reviewboard.mozilla.org/r/72628/#review71142

r=me with nits addressed:

::: layout/reftests/svg/svg-integration/clip-path/clip-path-inset-001-ref.html:18
(Diff revision 2)
> +      height: 100px;
> +      border: solid green 40px;
> +      background-color: green;
> +      position: relative;
> +      top:10px;
> +      left:10px;

Nit: add space after "top:" and "left:" here, for consistency / better-readability.

::: layout/reftests/svg/svg-integration/clip-path/clip-path-inset-001a.html:8
(Diff revision 2)
> +     http://creativecommons.org/publicdomain/zero/1.0/
> +-->
> +<!DOCTYPE html>
> +<html>
> +<head>
> +  <title>CSS Masking: Test clip-path property and inset function with 10px on all corners</title>

s/all corners/all sides/

::: layout/reftests/svg/svg-integration/clip-path/clip-path-inset-001b.html:8
(Diff revision 2)
> +     http://creativecommons.org/publicdomain/zero/1.0/
> +-->
> +<!DOCTYPE html>
> +<html>
> +<head>
> +  <title>CSS Masking: Test clip-path property and inset function with 10px,20px</title>

s/10px,20px/different insets in each axis/

(That's the relevant broader thing that you're testing here, and it's clearer why that's worth testing as compared to "10px,20px".)

::: layout/reftests/svg/svg-integration/clip-path/clip-path-inset-001b.html:20
(Diff revision 2)
> +      height: 100px;
> +      border: solid green 50px;
> +      background-color: green;
> +      clip-path: inset(10px 20px);
> +      position:relative;
> +      left:-10px;

Nit: add space after "position:" and "left:"

::: layout/reftests/svg/svg-integration/clip-path/clip-path-inset-001c.html:21
(Diff revision 2)
> +      border: solid green 50px;
> +      background-color: green;
> +      clip-path: inset(5px 10px 15px 20px);
> +      position:relative;
> +      left:-10px;
> +      top:5px;

Add space after "position:", "left:", and "top:"
Attachment #8782461 - Flags: review?(dholbert) → review+
Comment on attachment 8782462 [details]
Bug 1246762 - Add reftests for |clip-path: inset()|s with rounded corners ;

https://reviewboard.mozilla.org/r/72630/#review71148

r=me with nits addressed:

::: layout/reftests/svg/svg-integration/clip-path/clip-path-inset-002-ref.html:8
(Diff revision 4)
> +     http://creativecommons.org/publicdomain/zero/1.0/
> +-->
> +<!DOCTYPE html>
> +<html>
> +<head>
> +  <title>CSS Masking: Ref test for clip-path's inset function (with rounded corners) 002</title>

s/Ref test/Reference

::: layout/reftests/svg/svg-integration/clip-path/clip-path-inset-002-ref.html:18
(Diff revision 4)
> +      height: 180px;
> +      background-color: green;
> +      position:relative;
> +      top:10px;
> +      left:10px;
> +      border-radius:20px;

Add spaces after the last 4 lines' ":" characters.

::: layout/reftests/svg/svg-integration/clip-path/clip-path-inset-003-ref.html:18
(Diff revision 4)
> +      height: 180px;
> +      background-color: green;
> +      position:relative;
> +      top:10px;
> +      left:10px;
> +      border-radius:10px 20px 30px 40px / 50px 60px 70px 80px;

Add spaces after the last 4 lines' ":" characters.
Attachment #8782462 - Flags: review?(dholbert) → review-
Comment on attachment 8782462 [details]
Bug 1246762 - Add reftests for |clip-path: inset()|s with rounded corners ;

https://reviewboard.mozilla.org/r/72630/#review71152
Attachment #8782462 - Flags: review?(dholbert) → review+
339 for the 3rd test becomes 340 on android. Everything else passes. Fixed, pushing to autoland.
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0cd141c409f6
Add support for inset() in clip-path ; r=dholbert
https://hg.mozilla.org/integration/autoland/rev/7bbf95b8ed37
Add reftests for non-rounded |clip-path: inset()|s ; r=dholbert
https://hg.mozilla.org/integration/autoland/rev/1b0165fb16b5
Add reftests for |clip-path: inset()|s with rounded corners ; r=dholbert
I've added a compatibility note to https://developer.mozilla.org/en-US/docs/Web/CSS/clip-path and mentioned it in the release notes at https://developer.mozilla.org/en-US/Firefox/Releases/51.

Sebastian
Keywords: dev-doc-needed → dev-doc-complete
Blocks: basic-shape
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: