Implement basic shapes clip-path clipping for HTML elements

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: krit, Assigned: jwatt)

Tracking

(Blocks: 3 bugs, {dev-doc-complete})

unspecified
mozilla47
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [behind pref layout.css.clip-path-shapes.enabled])

Attachments

(2 attachments, 10 obsolete attachments)

(Reporter)

Description

5 years ago
Make use of basic shapes in clip-path
(Reporter)

Updated

5 years ago
Blocks: 1072894
(Reporter)

Comment 1

5 years ago
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.
Flags: needinfo?(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.
Flags: needinfo?(mstange)
Oh, and you'll also need to do something for hit testing. Probably in nsSVGUtils::HitTestClip.
(Reporter)

Comment 4

5 years ago
(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?
Flags: needinfo?(mstange)
(Reporter)

Comment 5

5 years ago
(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.
(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.
Flags: needinfo?(mstange)
(Reporter)

Comment 7

5 years ago
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?
(Reporter)

Comment 8

5 years ago
Posted patch clip-path-clip.patch (obsolete) — Splinter Review
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.
Flags: needinfo?(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.)
Flags: needinfo?(mstange)
(Reporter)

Comment 10

5 years ago
Posted patch clip-path-clip.patch (obsolete) — Splinter Review
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.
Attachment #8506789 - Attachment is obsolete: true
Flags: needinfo?(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.
Flags: needinfo?(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.
Or wait, gfxContext has a SetPath function that you can call with a Moz2D Path. Very good, please use that one.
Flags: needinfo?(jwatt)
Oops, sorry Jonathan, I didn't need anything from you after all.
Flags: needinfo?(jwatt)
(Reporter)

Comment 15

5 years ago
Posted patch clip-path-rendering-001.patch (obsolete) — Splinter Review
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 :(
Attachment #8506916 - Attachment is obsolete: true
Attachment #8510855 - Flags: review?(mstange)
(Assignee)

Comment 16

5 years ago
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().)
(Assignee)

Comment 17

5 years ago
Oh, and you forgot to |hg add| nsCSSClipPathInstance.cpp.
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.
Attachment #8510855 - Flags: review?(mstange)
(Reporter)

Comment 19

5 years ago
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 :(
(Reporter)

Comment 20

5 years ago
Posted patch clip-path-rendering-001.patch (obsolete) — Splinter Review
See comment 15.
Attachment #8510855 - Attachment is obsolete: true
Attachment #8511021 - Flags: review?(mstange)
Attachment #8511021 - Flags: review?(jwatt)
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.
Attachment #8511021 - Flags: review?(mstange) → review+
(Reporter)

Comment 22

5 years ago
(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.
Have you looked at the implementation of nsRenderingContext::GetDrawTarget()? :-)
(Reporter)

Comment 24

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

Well, true :P
I just realized that the patch doesn't handle hit testing. Are you going to take care of that in a different bug?
(Reporter)

Comment 26

4 years ago
(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.
What are the next steps in this bug? Are we all waiting for the review from Jonathan? Jonathan, can you take a look?
(Assignee)

Comment 28

4 years ago
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.
(Reporter)

Comment 29

4 years ago
Thanks Jonathan and Markus. I'll take a look into hit testing as soon as I have some minutes.
(Assignee)

Comment 30

4 years ago
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.
(Assignee)

Comment 31

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

Cancelling review for now until the hit testing is done.
Attachment #8511021 - Flags: review?(jwatt)
(Reporter)

Comment 32

4 years ago
(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.
(Reporter)

Updated

4 years ago
Flags: needinfo?(jwatt)
(Assignee)

Comment 33

4 years ago
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. ;)
Flags: needinfo?(jwatt)
(Reporter)

Comment 34

4 years ago
Posted patch clip-path-rendering-001.patch (obsolete) — Splinter Review
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
Attachment #8511021 - Attachment is obsolete: true
Attachment #8530800 - Flags: review?(mstange)
Attachment #8530800 - Flags: review?(jwatt)
(Reporter)

Comment 35

4 years ago
The build problem is probably an issue with namespaces. Adding

using namespace mozilla;
using namespace mozilla::gfx;

might fix it. Will test.
(Reporter)

Comment 36

4 years ago
Posted patch clip-path-rendering-001.patch (obsolete) — Splinter Review
Fix build and test problems.


https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4bbf4353bca6
Attachment #8530800 - Attachment is obsolete: true
Attachment #8530800 - Flags: review?(mstange)
Attachment #8530800 - Flags: review?(jwatt)
Attachment #8530864 - Flags: review?(mstange)
Attachment #8530864 - Flags: review?(jwatt)
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).
Attachment #8530864 - Flags: review?(mstange)
(Reporter)

Comment 38

4 years ago
Posted patch clip-path-rendering-001.patch (obsolete) — Splinter Review
New patch addresses reviewer comment.
Attachment #8530864 - Attachment is obsolete: true
Attachment #8530864 - Flags: review?(jwatt)
Attachment #8531183 - Flags: review?(mstange)
Comment on attachment 8531183 [details] [diff] [review]
clip-path-rendering-001.patch

This looks like the same patch as last time.
Attachment #8531183 - Flags: review?(mstange)
(Reporter)

Comment 40

4 years ago
Posted patch clip-path-rendering-001.patch (obsolete) — Splinter Review
Was indeed the wrong patch. Here the correct one.
Attachment #8531183 - Attachment is obsolete: true
Attachment #8531253 - Flags: review?(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();
Attachment #8531253 - Flags: review?(mstange) → review+
(Reporter)

Comment 42

4 years ago
Posted patch clip-path-rendering-001.patch (obsolete) — Splinter Review
patch for landing
(Reporter)

Updated

4 years ago
Keywords: checkin-needed
Please also get a review from Jonathan.
(Reporter)

Updated

4 years ago
Keywords: checkin-needed
(Reporter)

Comment 44

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

Ah sorry. Jonathan, could you take a look please?
Attachment #8531272 - Flags: review?(jwatt)
Keywords: dev-doc-needed
(Reporter)

Comment 45

4 years ago
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.
Attachment #8537201 - Flags: review?(mstange)
Ping on this bug. Any update here?
Flags: needinfo?(mstange)
Flags: needinfo?(jwatt)
OS: Mac OS X → All
Hardware: x86 → All
I'm waiting for jwatt to review first.
Flags: needinfo?(mstange)
Assignee: nobody → jwatt
Comment on attachment 8537201 [details] [diff] [review]
clip-path-circle-ellipse-rendering.patch

Clearing review until jwatt gets around to review the other patch.
Attachment #8537201 - Flags: review?(mstange)
(Assignee)

Comment 49

4 years ago
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?
Flags: needinfo?(jwatt) → needinfo?(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.
Flags: needinfo?(mstange)
(Assignee)

Comment 51

4 years ago
(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? :)
(Assignee)

Updated

4 years ago
Flags: needinfo?(jwatt)
Duplicate of this bug: 1222115
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
(Assignee)

Comment 54

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

I'll land this shortly with some review issues fixed inline.
Flags: needinfo?(jwatt)
Attachment #8531272 - Flags: review?(jwatt) → review+
(Assignee)

Updated

3 years ago
No longer blocks: 1072894
Depends on: 1072894
(Assignee)

Updated

3 years ago
See Also: → bug 1110460
(Assignee)

Comment 55

3 years ago
Attachment #8531253 - Attachment is obsolete: true
Attachment #8531272 - Attachment is obsolete: true
(Assignee)

Comment 57

3 years ago
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.
(Assignee)

Updated

3 years ago
Depends on: 1245845
(Assignee)

Comment 58

3 years ago
(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.
(Assignee)

Updated

3 years ago
Blocks: 1246539
(Assignee)

Updated

3 years ago
Blocks: 1246541
(Assignee)

Updated

3 years ago
Blocks: 1246741
(Assignee)

Updated

3 years ago
Blocks: 1246762
(Assignee)

Updated

3 years ago
Blocks: 1246764

Comment 60

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d8ceb13996c5
https://hg.mozilla.org/mozilla-central/rev/05ab4dd9fe06
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Comment 61

3 years ago
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)]:
relnote-firefox: --- → ?
Whiteboard: [behind pref layout.css.clip-path-shapes.enabled]
(Assignee)

Comment 62

3 years ago
Oh, and the support only includes HTML elements for now, not SVG elements.
(Assignee)

Updated

3 years ago
Summary: Make use of basic shapes in clip-path → Implement basic shapes clip-path clipping for HTML elements
(Assignee)

Updated

3 years ago
Blocks: 1247229
(Assignee)

Comment 63

3 years ago
Actually cancelling relnote-firefox? here and I'll add that flag to bug 1247229 when we know what we're doing there.
relnote-firefox: ? → ---
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1262627

Updated

2 years ago
Blocks: 1324179
You need to log in before you can comment on or make changes to this bug.