1075457 - Implement basic shapes clip-path clipping for HTML elements

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Dirk Schulze]

Make use of basic shapes in clip-path

Comment 1

[Dirk Schulze]

A basic shape used by nsStyleBasicShape should have all information necessary to create a clipping path. However, I am currently unsure where I need start and need to know where to look first.

Comment 2

[Markus Stange [:mstange]]

At the moment, a clip path is applied during painting from here: http://dxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGIntegrationUtils.cpp#570
You'll need to make this work even when there's no clip path frame, by generalizing nsSVGEffects::EffectProperties in a similar way to what Max did for filters in http://hg.mozilla.org/mozilla-central/rev/7913b3e61acb and by adding a new code path that calls aCtx->NewPath(); aCtx->SetPath(your computed mozilla::gfx::Path); gfx->SetFillRule(???); gfx->Clip(); for the basic shape.
You'll also need to make sure that a frame with a basic shape clip-path gets an nsDisplaySVGEffects display item built, by ensuring that nsSVGIntegrationUtils::UsingEffectsForFrame returns true in that case. But that may already be the case.

Comment 3

[Markus Stange [:mstange]]

Oh, and you'll also need to do something for hit testing. Probably in nsSVGUtils::HitTestClip.

Comment 4

[Dirk Schulze]

(In reply to Markus Stange [:mstange] from comment #2)
> At the moment, a clip path is applied during painting from here:
> http://dxr.mozilla.org/mozilla-central/source/layout/svg/
> nsSVGIntegrationUtils.cpp#570
> You'll need to make this work even when there's no clip path frame, by
> generalizing nsSVGEffects::EffectProperties in a similar way to what Max did
> for filters in http://hg.mozilla.org/mozilla-central/rev/7913b3e61acb and by
> adding a new code path that calls aCtx->NewPath(); aCtx->SetPath(your
> computed mozilla::gfx::Path); gfx->SetFillRule(???); gfx->Clip(); for the
> basic shape.
> You'll also need to make sure that a frame with a basic shape clip-path gets
> an nsDisplaySVGEffects display item built, by ensuring that
> nsSVGIntegrationUtils::UsingEffectsForFrame returns true in that case. But
> that may already be the case.

nsSVGFilterProperty inherits from nsSVGFilterChainObserver. I agree that I probably need a nsClipPathProperty. But do I need to create a new observer? As far as I can see, nsSVGFilterChainObserver is just there to handle invalidations with url references correctly. To precise: multiple references. clip-path can just have one.

Could I still let nsSVGClipPathProperty inherit from nsSVGRenderingObserverProperty?

Comment 5

[Dirk Schulze]

(In reply to Markus Stange [:mstange] from comment #2)

In addition to the question in comment #4:

> At the moment, a clip path is applied during painting from here:
> http://dxr.mozilla.org/mozilla-central/source/layout/svg/
> nsSVGIntegrationUtils.cpp#570

nsSVGIntegrationUtils.cpp just handles clip path frames, right? Your comment seems to imply that as well. So the aCtx->NewPath()... calls need to be somewhere else. Where do I need to add that?

> You'll need to make this work even when there's no clip path frame, by
> generalizing nsSVGEffects::EffectProperties in a similar way to what Max did
> for filters in http://hg.mozilla.org/mozilla-central/rev/7913b3e61acb and by

As said in the previous comment, this whole code path just seems to handle the reference cases. The functions HasValidFilter() and HasNoFilterOrHasValidFilter() never check for shorthand filters but only for reference filters. So we basically need the nsClipPathProperty to provide the same information: has a reference clip path or has no clip path or a reference clip path.

> adding a new code path that calls aCtx->NewPath(); aCtx->SetPath(your
> computed mozilla::gfx::Path); gfx->SetFillRule(???); gfx->Clip(); for the
> basic shape.
> You'll also need to make sure that a frame with a basic shape clip-path gets
> an nsDisplaySVGEffects display item built, by ensuring that
> nsSVGIntegrationUtils::UsingEffectsForFrame returns true in that case. But
> that may already be the case.

Yes, this is already the case: style->mClipPath.GetType() != NS_STYLE_CLIP_PATH_NONE.

Comment 6

[Markus Stange [:mstange]]

(In reply to Dirk Schulze from comment #4)
> (In reply to Markus Stange [:mstange] from comment #2)
> > At the moment, a clip path is applied during painting from here:
> > http://dxr.mozilla.org/mozilla-central/source/layout/svg/
> > nsSVGIntegrationUtils.cpp#570
> > You'll need to make this work even when there's no clip path frame, by
> > generalizing nsSVGEffects::EffectProperties in a similar way to what Max did
> > for filters in http://hg.mozilla.org/mozilla-central/rev/7913b3e61acb and by
> > adding a new code path that calls aCtx->NewPath(); aCtx->SetPath(your
> > computed mozilla::gfx::Path); gfx->SetFillRule(???); gfx->Clip(); for the
> > basic shape.
> > You'll also need to make sure that a frame with a basic shape clip-path gets
> > an nsDisplaySVGEffects display item built, by ensuring that
> > nsSVGIntegrationUtils::UsingEffectsForFrame returns true in that case. But
> > that may already be the case.
> 
> nsSVGFilterProperty inherits from nsSVGFilterChainObserver. I agree that I
> probably need a nsClipPathProperty. But do I need to create a new observer?
> As far as I can see, nsSVGFilterChainObserver is just there to handle
> invalidations with url references correctly. To precise: multiple
> references. clip-path can just have one.

Correct, Max's change did two things (or more): it added support for filter functions, and it extended the filter property to handle multiple references. You don't need to do the latter for clip paths.

> Could I still let nsSVGClipPathProperty inherit from
> nsSVGRenderingObserverProperty?

Yes, I think so. Precisely because you just need to observe one reference, not multiple.

(In reply to Dirk Schulze from comment #5)
> (In reply to Markus Stange [:mstange] from comment #2)
> 
> In addition to the question in comment #4:
> 
> > At the moment, a clip path is applied during painting from here:
> > http://dxr.mozilla.org/mozilla-central/source/layout/svg/
> > nsSVGIntegrationUtils.cpp#570
> 
> nsSVGIntegrationUtils.cpp just handles clip path frames, right? Your comment
> seems to imply that as well.

Correct.

> So the aCtx->NewPath()... calls need to be
> somewhere else.

Well, you need to change the code in nsSVGIntegrationUtils.cpp in such a way that it no longer only handles clip path frames, but also basic shapes.

> Where do I need to add that?

That's really up to you. You can start by just adding functions to nsSVGIntegrationUtils.cpp (which you call in the right places) and if that gets too complicated, maybe create a new class (nsClipPathInstance?) in its own files.

> 
> > You'll need to make this work even when there's no clip path frame, by
> > generalizing nsSVGEffects::EffectProperties in a similar way to what Max did
> > for filters in http://hg.mozilla.org/mozilla-central/rev/7913b3e61acb and by
> 
> As said in the previous comment, this whole code path just seems to handle
> the reference cases. The functions HasValidFilter() and
> HasNoFilterOrHasValidFilter() never check for shorthand filters but only for
> reference filters. So we basically need the nsClipPathProperty to provide
> the same information: has a reference clip path or has no clip path or a
> reference clip path.

Sounds about right... I think.

Comment 7

[Dirk Schulze]

Thinking about this more, we already handle URL references just fine. (clip-path: <url> works and is tested.) If we need to handle basic shapes in nsSVGIntegrationUtils.cpp, then we just need to check for the aFrame->StyleSVGReset()->mClipPath at the right place. Basic Shapes are complicated and we will need nsClipPathInstance to outsource the complexity. nsSVGEffects can stay as it is right now I think. Is nsSVGIntegrationUtils.cpp really the only thing that needs to be aware of basic shape clip-paths?

Comment 8

[Dirk Schulze]

Attachment: clip-path-clip.patch

This is an early prototype. I am uploading it since I see an issue with the CTM of the context and don't know where it is coming from.

In an example I tried to clip a triangle between 3 corners from a div. This is how I needed to write the code so that it does what I expected:

<html>
<style>
div {
	width: 200px;
	height: 200px;
	background-color: green;
	clip-path: polygon(0 0, 0 6.5px, 6.5px 0);
}
</style>
<div></div>
</html>

The 6.5px seem to be equal to the actual 200px of the div. I wonder where this is coming from.

The cssPxToDevPxMatrix has a scale factor of 2.

The result for the computed x, y of the polygon arguments are 0 and 390. (Since 6.5 is an estimate and not exactly the 200px, it would probably be 400 otherwise.)

So there is some CTM transformation of the context going on. But where and how do I get the transform to transform the path accordingly.

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8506789 [details] [diff] [review]
clip-path-clip.patch

>+  nscoord x = nsRuleNode::ComputeCoordPercentCalc(coords[0], aRect.width);
>+  nscoord y = nsRuleNode::ComputeCoordPercentCalc(coords[1], aRect.height);
>+  aGfx->NewPath();
>+  aGfx->MoveTo(gfxPoint(x, y));

The type nscoord indicates that a value is in "app units". 60 app units are always one CSS pixel. (nsPresContext::AppUnitsPerCSSPixel() is 60.)

Comment 10

[Dirk Schulze]

Attachment: clip-path-clip.patch

I would like to have some feedback if that is the right way to go.

Even though I have the nsCSSClipPathInstance class, it doesn't do anything particular that some more static functions couldn't do. Keep the class?

Currently ApplyBasicShapeClip is called by non-SVG content for SVG content I will need different reference boxes and some refactoring. I'll plan to do SVG afterwards.

Comment 11

[Markus Stange [:mstange]]

I don't have a strong opinion on static functions vs. a dedicated class. Are we going to support basic shapes in different contexts (e.g. for other CSS properties) in the future? In that case it would make sense to keep some of the code in a shared, non-clip-path-specific file, and nsSVGIntegrationUtils.cpp probably is as good as any.

I'll post more comments or your patch soon.

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8506916 [details] [diff] [review]
clip-path-clip.patch

>+  gfxContextMatrixAutoSaveRestore autoRestore(aGfx);
>+  gfxPoint t(r.x / nsPresContext::AppUnitsPerCSSPixel(),
>+             r.y / nsPresContext::AppUnitsPerCSSPixel());
>+  gfxMatrix newMatrix =
>+    aGfx->CurrentMatrix().PreMultiply(aMatrix).Translate(r).NudgeToIntegers();

I suggest not messing with the context transform and instead directly operating on device pixels. aMatrix here is just a scale matrix that scales device pixels to CSS pixels; you can get that scale from the frame. You'll probably also want to snap the rect to device pixels, and that's easier to understand if you don't have another CSS pixel scale in there.

I suggest something like this:

nscoord appUnitsPerDevPixel = aFrame->PresContext()->AppUnitsPerDevPixel();
nsIntRect devRect = r.ToNearestPixels(appUnitsPerDevPixel);
// and then maybe r = devRect.ToAppUnits(appUnitsPerDevPixel);

and then ...

>+void
>+nsCSSClipPathInstance::ApplyPolygonClipPath(gfxContext* aGfx,
>+                                            const nsRect& aRect,
>+                                            const gfxMatrix& aMatrix)
>+{
>+  nsStyleBasicShape* basicShape = mClipPath.GetBasicShape();
>+  const nsTArray<nsStyleCoord>& coords = basicShape->Coordinates();
>+  NS_ABORT_IF_FALSE(coords.Length() % 2 == 0 &&
>+                    coords.Length() >= 2, "wrong number of arguments");
>+
>+  nscoord x = nsRuleNode::ComputeCoordPercentCalc(coords[0], aRect.width);
>+  nscoord y = nsRuleNode::ComputeCoordPercentCalc(coords[1], aRect.height);
>+  aGfx->NewPath();
>+  aGfx->MoveTo(gfxPoint(x, y) / nsPresContext::AppUnitsPerCSSPixel());

aGfx->MoveTo(nsLayoutUtils::PointToGfxPoint(aRect.TopLeft() + nsPoint(x, y), appUnitsPerDevPixel));

Would it make sense to snap each polygon point to device pixels? I'm thinking no, do you have an opinion on it?

>+  // Basic shapes need to be handled separately from <clipPath> refereces.
>+  const nsStyleSVGReset *style = aFrame->StyleSVGReset();
>+  if (style->HasClipPath() && !clipPathFrame){
>+    gfx->Save();
>+    nsCSSClipPathInstance::ApplyBasicShapeClip(aCtx, aFrame,
>+                                               cssPxToDevPxMatrix);
>+  }
>+
>   /* Paint the child */
>   if (effectProperties.HasValidFilter()) {
>     RegularFramePaintCallback callback(aBuilder, aLayerManager,
>@@ -546,7 +554,7 @@ nsSVGIntegrationUtils::PaintFramesWithEffects(nsRenderingContext* aCtx,
>     gfx->SetMatrix(gfx->CurrentMatrix().Translate(devPixelOffsetToUserSpace));
>   }
> 
>-  if (clipPathFrame && isTrivialClip) {
>+  if ((clipPathFrame && isTrivialClip) || (style->HasClipPath() && !clipPathFrame)) {
>     gfx->Restore();
>   }

Hrm, it's unfortunate that the rest of this function basically requires you to operate on a gfxContext instead of on the wrapped DrawTarget directly. It would be much better to write the new code using Moz2D APIs (DrawTarget, Path, PathBuilder) because gfxContext is going away.

Comment 13

[Markus Stange [:mstange]]

Or wait, gfxContext has a SetPath function that you can call with a Moz2D Path. Very good, please use that one.

Comment 14

[Markus Stange [:mstange]]

Oops, sorry Jonathan, I didn't need anything from you after all.

Comment 15

[Dirk Schulze]

Attachment: clip-path-rendering-001.patch

IMO there is a lot of confusion with nsPoint, nsIntPoint, gfxPoint and Point. Especially getting from the first to the last is tricky when you have to add an already transformed point to a not yet transformed point. A lot of round tripping would be necessary and most of the time the results were not as expected. This seems to be the simplest solution.

I expect basic shapes to be used by other properties but don't think that there is much to share. CSS Shapes will mostly operate in layout/app space and won't create a Path.

I uploaded reference tests. In many situations I couldn't use a reference without clipping. AA issues seem to prevent that :(

Comment 16

[Jonathan Watt [:jwatt]]

Drive-by comment: The code is currently in a state of flux in the move from Thebes (gfxContext etc.) to Moz2D (DrawTarget etc.). One unfortunate gotcha of this state is that applying transform or clip path changes to a _DrawTarget_ (wrapped by a gfxContext) is not currently safe if you can't be sure that other code will not call gfxContext::Save()/Restore() before you restore the DrawTarget transform or pop the clips that you pushed. If any code between those two points does call gfxContext::Restore then it will not only wipe out the changes that it intended to wipe out, but it will also wipe out your DrawTarget transform/clip path changes. (This is because gfxContext doesn't know anything about the changes you made to the DrawTarget at the time Save() was called so it saves the gfxContext's transform/clip stack state, not the DrawTarget's, and that's what you'll get restored on Restore().)

Comment 17

[Jonathan Watt [:jwatt]]

Oh, and you forgot to |hg add| nsCSSClipPathInstance.cpp.

Comment 18

[Markus Stange [:mstange]]

Comment on attachment 8510855 [details] [diff] [review]
clip-path-rendering-001.patch

What Jonathan said.

And I think you need to pass the gfxContext to nsCSSClipPathInstance after all, and use gfxContext's clipping mechanism with a Moz2D Path. Then you don't need to mix DrawTarget::PushClip/PopClip and gfxContext::Save/Restore.

Comment 19

[Dirk Schulze]

Didn't get the review messages for some reason. Thank you both. This sounds a bit scary and I agree that I should use gfx's save and restore mechanisms. Will do. Sadly I didn't recognize that I forgot to add files :(

Comment 20

[Dirk Schulze]

Attachment: clip-path-rendering-001.patch

See comment 15.

Comment 21

[Markus Stange [:mstange]]

Comment on attachment 8511021 [details] [diff] [review]
clip-path-rendering-001.patch

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

Looks good! Jonathan should probably review this as well, and dbaron can review the nsStyleStruct.h change.

::: layout/svg/nsCSSClipPathInstance.cpp
@@ +19,5 @@
> +  instance.ComputeAndApplyClipPath(aCtx);
> +}
> +
> +void
> +nsCSSClipPathInstance::ComputeAndApplyClipPath(nsRenderingContext* aCtx)

Pass a gfxContext instead of an nsRenderingContext, please.

Comment 22

[Dirk Schulze]

(In reply to Markus Stange [:mstange] from comment #21)
> Comment on attachment 8511021 [details] [diff] [review]
> clip-path-rendering-001.patch
> 
> Review of attachment 8511021 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good! Jonathan should probably review this as well, and dbaron can
> review the nsStyleStruct.h change.
> 
> ::: layout/svg/nsCSSClipPathInstance.cpp
> @@ +19,5 @@
> > +  instance.ComputeAndApplyClipPath(aCtx);
> > +}
> > +
> > +void
> > +nsCSSClipPathInstance::ComputeAndApplyClipPath(nsRenderingContext* aCtx)
> 
> Pass a gfxContext instead of an nsRenderingContext, please.

aCtx->GetDrawTarget() is the easiest way to get a DrawTarget. This is the only reason I pass it. So that I can create a Path object.

Comment 23

[Markus Stange [:mstange]]

Have you looked at the implementation of nsRenderingContext::GetDrawTarget()? :-)

Comment 24

[Dirk Schulze]

(In reply to Markus Stange [:mstange] from comment #23)
> Have you looked at the implementation of
> nsRenderingContext::GetDrawTarget()? :-)

Well, true :P

Comment 25

[Markus Stange [:mstange]]

I just realized that the patch doesn't handle hit testing. Are you going to take care of that in a different bug?

Comment 26

[Dirk Schulze]

(In reply to Markus Stange [:mstange] from comment #25)
> I just realized that the patch doesn't handle hit testing. Are you going to
> take care of that in a different bug?

Yes, I will work on that after the clipping. I should have mentioned that.

Comment 27

[Markus Stange [:mstange]]

What are the next steps in this bug? Are we all waiting for the review from Jonathan? Jonathan, can you take a look?

Comment 28

[Jonathan Watt [:jwatt]]

Hmm. Where did my question about the clipping go? I thought I'd commented saying that we'd need the visual and hit-testing clipping to land together, so I wanted to see the hit-testing patch too. Sorry that didn't actually make it onto this bug.

Comment 29

[Dirk Schulze]

Thanks Jonathan and Markus. I'll take a look into hit testing as soon as I have some minutes.

Comment 30

[Jonathan Watt [:jwatt]]

Dirk, you probably figured this out, but the hit-testing involves changing nsSVGIntegrationUtils::HitTestFrameForEffects(), or rather its callee, nsSVGUtils::HitTestClip(). You should be able to use the path that nsCSSClipPathInstance::ApplyPolygonClipPath() returns there (Path::ContainsPoint()). Incidentally, please do rename that function since it no longer does any applying.

As for testing, this method should be helpful:

https://mxr.mozilla.org/mozilla-central/search?string=elementFromPoint%28&find=test_&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Can you also tweak nsDisplaySVGEffects::PrintEffects() so that it prints something other than "trivial"/"non-trivial" for this type of clipping.

Comment 31

[Jonathan Watt [:jwatt]]

Comment on attachment 8511021 [details] [diff] [review]
clip-path-rendering-001.patch

Cancelling review for now until the hit testing is done.

Comment 32

[Dirk Schulze]

(In reply to Jonathan Watt [:jwatt] from comment #30)
> Dirk, you probably figured this out, but the hit-testing involves changing
> nsSVGIntegrationUtils::HitTestFrameForEffects(), or rather its callee,
> nsSVGUtils::HitTestClip(). You should be able to use the path that
> nsCSSClipPathInstance::ApplyPolygonClipPath() returns there
> (Path::ContainsPoint()). Incidentally, please do rename that function since
> it no longer does any applying.

Hm, while it is probably possible to get all necessary information like drawing context from the frame, I wonder if that wouldn't be a major performance hit. This function creates a new path every single time. On some rendering libs that is a non-trivial function and takes time. On the other hand, creating/storing/invalidating a path might be tricky. For <clipPath>, SVGEffects stores a reference in "properties" by url. Not sure if something similar for basic shapes is going to work. Any thoughts?

> 
> As for testing, this method should be helpful:
> 
> https://mxr.mozilla.org/mozilla-central/
> search?string=elementFromPoint%28&find=test_&findi=&filter=^[^\0]*%24&hitlimi
> t=&tree=mozilla-central

Thanks.

> 
> Can you also tweak nsDisplaySVGEffects::PrintEffects() so that it prints
> something other than "trivial"/"non-trivial" for this type of clipping.

Ok, I guess from the name that this is for printing? :P So it probably can be tested with the printing dialog.

Comment 33

[Jonathan Watt [:jwatt]]

I think it's probably best to keep things simple in this bug (no caching of the Path) and file a follow-up to work out whether/how we should cache it. (The reference that SVGEffects stores is a reference to a frame, so that's somewhat different. Here we'd need to invalidate if the CSS clip-path property changes, or if whatever (if anything) affects the clip-rule for basic shapes (presumably SVG's clip-rule property does).)

To create the Path for hit testing you should get a PathBuilder using gfxPlatform::GetPlatform()->ScreenReferenceDrawTarget(). Probably make ApplyPolygonClipPath() call that if the passed DrawTarget is nullptr.

nsDisplaySVGEffects::PrintEffects() is for "printing" debug strings to the console/log, so the change there is trivially just to use a different string. ;)

Comment 34

[Dirk Schulze]

Attachment: clip-path-rendering-001.patch

This adds support for hit testing as well and renames the new functions. Currently there is a problem with Linux debug builds that I do not understand and that I can not reproduce on my Mac. I would still like to get a review since I think the patch is ready otherwise.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f3a33e3c4a92

Comment 35

[Dirk Schulze]

The build problem is probably an issue with namespaces. Adding

using namespace mozilla;
using namespace mozilla::gfx;

might fix it. Will test.

Comment 36

[Dirk Schulze]

Attachment: clip-path-rendering-001.patch

Fix build and test problems.


https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4bbf4353bca6

Comment 37

[Markus Stange [:mstange]]

Comment on attachment 8530864 [details] [diff] [review]
clip-path-rendering-001.patch

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

::: layout/base/nsDisplayList.cpp
@@ +5881,5 @@
> +  if (style->HasClipPath() && !clipPathFrame) {
> +    if (!first) {
> +      aTo += ", ";
> +    }
> +    aTo += nsPrintfCString("clip(basic-shape)");

Just aTo += "clip(basic-shape)"; since you're not making use of any printf-style placeholders like %s and thus don't need nsPrintfCString.

::: layout/style/test/test_clip-path_polygon.html
@@ +28,5 @@
> +if (SpecialPowers.getBoolPref("layout.css.clip-path-shapes.enabled")) {
> +	isnot(a, document.elementFromPoint(199, 199), "a shouldn't be found");
> +	isnot(a, document.elementFromPoint(199, 250), "a shouldn't be found");
> +	isnot(a, document.elementFromPoint(250, 199), "a shouldn't be found");
> +	isnot(a, document.elementFromPoint(255, 255), "a should't be found");

shouldn't

@@ +33,5 @@
> +	isnot(a, document.elementFromPoint(301, 200), "a shouldn't be found");
> +	isnot(a, document.elementFromPoint(200, 301), "a shouldn't be found");
> +}
> +is(a, document.elementFromPoint(200, 200), "a should be found");
> +is(a, document.elementFromPoint(200, 200), "a should be found");

Was this supposed to be 299, 299? Because you're testing 200, 200 twice.

::: layout/svg/nsCSSClipPathInstance.cpp
@@ +54,5 @@
> +    return false;
> +  }
> +
> +  nsCSSClipPathInstance instance(aFrame, clipPath,
> +                                 aFrame->PresContext()->AppUnitsPerCSSPixel());

Hmm, I don't like the fact the the pixel ratio is to device pixels in one place and CSS pixels in the other. Can you make it always in device pixels and convert the point that you're hit testing from CSS pixels to device pixels first?

@@ +59,5 @@
> +  RefPtr<DrawTarget> drawTarget =
> +    gfxPlatform::GetPlatform()->ScreenReferenceDrawTarget();
> +  RefPtr<Path> path = instance.CreateClipPath(drawTarget);
> +  Matrix t;
> +  return path->ContainsPoint(Point(aPoint.x, aPoint.y), t);

#include "gfx2DGlue.h" and then
return path->ContainsPoint(ToPoint(aPoint), Matrix()); though this might look different if you need to convert aPoint first

@@ +96,5 @@
> +    case nsStyleBasicShape::Type::ePolygon:
> +      return CreateClipPathPolygon(aDrawTarget, r);
> +    default:
> +      // XXXkrit support all basic shapes
> +      NS_NOTREACHED("unexpected basic shape");

MOZ_ASSERT_UNREACHABLE seems to be the modern way to do this.

::: layout/svg/nsCSSClipPathInstance.h
@@ +22,5 @@
> +public:
> +  static void ApplyBasicShapeClip(gfxContext& aContext,
> +                                  nsIFrame* aFrame);
> +  static bool HitTestBasicShapeClip(nsIFrame* aFrame,
> +                                    const gfxPoint& aPoint);

Document that aPoint is in CSS pixels.

@@ +24,5 @@
> +                                  nsIFrame* aFrame);
> +  static bool HitTestBasicShapeClip(nsIFrame* aFrame,
> +                                    const gfxPoint& aPoint);
> +
> +  explicit nsCSSClipPathInstance(nsIFrame* aFrame,

The constructor and the two methods below it could even be private. You only really want to expose the two static methods to outsiders, don't you?

@@ +26,5 @@
> +                                    const gfxPoint& aPoint);
> +
> +  explicit nsCSSClipPathInstance(nsIFrame* aFrame,
> +                                 const nsStyleClipPath aClipPath,
> +                                 const nscoord aPixelRatio)

Once the paths you create are always in device pixels, you can remove this argument and make the users of mPixelRatio get mTargetFrame->PresContext()->AppUnitsPerDevPixel() directly (or store it in a local appUnitsPerDevPixel variable).

Comment 38

[Dirk Schulze]

Attachment: clip-path-rendering-001.patch

New patch addresses reviewer comment.

Comment 39

[Markus Stange [:mstange]]

Comment on attachment 8531183 [details] [diff] [review]
clip-path-rendering-001.patch

This looks like the same patch as last time.

Comment 40

[Dirk Schulze]

Attachment: clip-path-rendering-001.patch

Was indeed the wrong patch. Here the correct one.

Comment 41

[Markus Stange [:mstange]]

Comment on attachment 8531253 [details] [diff] [review]
clip-path-rendering-001.patch

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

::: layout/svg/nsCSSClipPathInstance.cpp
@@ +58,5 @@
> +  RefPtr<DrawTarget> drawTarget =
> +    gfxPlatform::GetPlatform()->ScreenReferenceDrawTarget();
> +  RefPtr<Path> path = instance.CreateClipPath(drawTarget);
> +  float pixelRatio = aFrame->PresContext()->AppUnitsPerCSSPixel() /
> +                     aFrame->PresContext()->AppUnitsPerDevPixel();

Both of these things are int32_t, so you're using integer division here. Also, AppUnitsPerCSSPixel is a static method of nsPresContext, so let's make this
float pixelRatio = float(nsPresContext::AppUnitsPerCSSPixel()) / 
                   aFrame->PresContext()->AppUnitsPerDevPixel();

Comment 42

[Dirk Schulze]

Attachment: clip-path-rendering-001.patch

patch for landing

Comment 43

[Markus Stange [:mstange]]

Please also get a review from Jonathan.

Comment 44

[Dirk Schulze]

Comment on attachment 8531272 [details] [diff] [review]
clip-path-rendering-001.patch

Ah sorry. Jonathan, could you take a look please?

Comment 45

[Dirk Schulze]

Attachment: clip-path-circle-ellipse-rendering.patch

This follows up on the other patch that still needs review from jwatt and probably will need to address the same comments later. I think it still helps to get a review for the functionality.

Comment 46

[Xidorn Quan [:xidorn] UTC+11]

Ping on this bug. Any update here?

Comment 47

[Markus Stange [:mstange]]

I'm waiting for jwatt to review first.

Comment 48

[Markus Stange [:mstange]]

Comment on attachment 8537201 [details] [diff] [review]
clip-path-circle-ellipse-rendering.patch

Clearing review until jwatt gets around to review the other patch.

Comment 49

[Jonathan Watt [:jwatt]]

Unfortunately I think this is the wrong way to implement this. The nsSVGUtils and nsSVGIntegrationUtils code exists to apply SVG effects to SVG and non-SVG content respectively. Basic shapes clipping is a CSS feature, and nothing to do with SVG. It seems like this should be implemented by extending the capabilities of DisplayListClipState and DisplayItemClip. Doing it this way would also provide better layers integration of the clipping which should give us better performance.

Markus, was there a reason you recommended integrating into nsDisplaySVGEffects?

Comment 50

[Markus Stange [:mstange]]

I wasn't aware that basic shape clipping is not allowed to work on SVG content. Where does it say that? On https://drafts.fxtf.org/css-masking-1/#the-clip-path , under "Applies to:", SVG elements are not excluded.

The infrastructure around DisplayItemClip is much more complicated than what we need here, because it needs to handle multiple clips and interleaved display items with different clips. But clip-path doesn't require most of that complexity because it always creates a stacking context. So we can just set a single clip-path clip on a container layer and then reset the clip for the container layer contents - which is what nsDisplaySVGEffects does.

You're right that DisplayItemClip gives us better performance and layers integration than nsDisplaySVGEffects. But I'd like to fix that for SVG clip-path/mask anyway.

Comment 51

[Jonathan Watt [:jwatt]]

(In reply to Markus Stange [:mstange] from comment #50)
> I wasn't aware that basic shape clipping is not allowed to work on SVG
> content. Where does it say that? On
> https://drafts.fxtf.org/css-masking-1/#the-clip-path , under "Applies to:",
> SVG elements are not excluded.

What I meant is that basic shape clipping is a pure CSS feature (that applies to everything) but nsSVGUtils/nsSVGIntegrationUtils is specifically about applying SVG features. Sorry that wasn't clear.

If we're going to start using them to apply non-SVG features (instead of going the DisplayItemClip route) then I guess we can rename them to something like nsEffectsUtils, or something...

> The infrastructure around DisplayItemClip is much more complicated than what
> we need here, because it needs to handle multiple clips and interleaved
> display items with different clips. But clip-path doesn't require most of
> that complexity because it always creates a stacking context. So we can just
> set a single clip-path clip on a container layer and then reset the clip for
> the container layer contents - which is what nsDisplaySVGEffects does.

It's true that the DisplayItemClip stuff does more, but it didn't seem like using it would cause problems, but maybe the overhead would be undesirable.

> You're right that DisplayItemClip gives us better performance and layers
> integration than nsDisplaySVGEffects. But I'd like to fix that for SVG
> clip-path/mask anyway.

Sounds great, when will that happen? :)

Comment 53

[Sebastian Zartner [:sebo]]

Regarding the comments it looks like the implementation was already pretty far, though fell asleep the last few months.
Jonathan, I guess the latest patch(es) still need your review.

Sebastian

Comment 54

[Jonathan Watt [:jwatt]]

Comment on attachment 8531272 [details] [diff] [review]
clip-path-rendering-001.patch

I'll land this shortly with some review issues fixed inline.

Comment 55

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Implement rendering for polygon() [r=mstange, r=jwatt]

Comment 56

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Implement circle and ellipse clipping function [r=mstange, r=jwatt]

Comment 57

[Jonathan Watt [:jwatt]]

The patches took a while to unbitrot, but they pass most of the tests now. I've still got a failure in layout/reftests/svg/svg-integration/clip-path/clip-path-polygon-004.html to figure out which I'll do tomorrow.

Comment 58

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #57)
> I've still got a failure in
> layout/reftests/svg/svg-integration/clip-path/clip-path-polygon-004.html

Turns out this is a gfx bug. I've got a fix for that in bug 1245845 awaiting review.

Comment 59

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8ceb13996c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/05ab4dd9fe06

Comment 60

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d8ceb13996c5
https://hg.mozilla.org/mozilla-central/rev/05ab4dd9fe06

Comment 61

[Jonathan Watt [:jwatt]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Support for clipping elements using a clip path specified directly as a CSS clip-path property value without requiring linking to a separate SVG <clipPath> element has been added behind the layout.css.clip-path-shapes.enabled pref. The currently supported clip paths are circle(), ellipse() and polygon().
[Suggested wording]: the above
[Links (documentation, blog post, etc)]:

Comment 62

[Jonathan Watt [:jwatt]]

Oh, and the support only includes HTML elements for now, not SVG elements.

Comment 63

[Jonathan Watt [:jwatt]]

Actually cancelling relnote-firefox? here and I'll add that flag to bug 1247229 when we know what we're doing there.

Comment 65

[Jean-Yves Perrier [:teoli]]

Updated
https://developer.mozilla.org/en-US/docs/Web/CSS/clip-path
https://developer.mozilla.org/en-US/docs/Web/CSS/basic-shape
and
https://developer.mozilla.org/en-US/Firefox/Releases/47#CSS