874507 - SVG marker can not be clipped with clip-path

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Dirk Schulze]

Attachment: marker1.svg

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_3) AppleWebKit/536.29.13 (KHTML, like Gecko) Version/6.0.4 Safari/536.29.13

Steps to reproduce:

I applied clipping by the clip-path presentation attribute to an SVG <marker> element.


Actual results:

The content of the marker should be clipped by the defined clip-path.


Expected results:

No clipping is applied at all.

Comment 1

[Dirk Schulze]

The rectangular marker in the attached example should be clipped by the circle of the clipPath element and make the marker a circle as well.

Comment 2

[(mostly gone) XtC4UaLL [:xtc4uall]]

Confirmed with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130625 Firefox/25.0 ID:20130625031238 CSet: bc569033125a

Comment 3

[Cameron McCormack (:heycam)]

Similarly, opacity, filter and mask are not applied.

Comment 4

[Cameron McCormack (:heycam)]

If we want this to work, we could make nsSVGMarkerFrame inherit from nsSVGDisplayContainerFrame (rather than nsSVGContainerFrame) and to override BuildDisplayList, PaintSVG, GetBBoxContribution, etc. to make the <marker> not paint as part of its parent (but to still be paintable when its mMarkedFrame is non-null.  This would mean that we do some unnecessary reflow of a <marker> that is a child of a display container frame, although we could try to stop that from happening too.

This is probably the right way to go about things for child <marker> elements of <path>, which SVG 2 supports.

Do you think this is the right approach Jonathan?

Comment 5

[Cameron McCormack (:heycam)]

Attachment: WIP (v0.1)

Something like this.  It's a bit ugly, having to check in various places whether we are painting a frame that is within a <marker> frame.

Maybe it would be better instead to just have a nsSVGUtils::PaintNonDisplayContainerFrameWithEffects that does most of what nsSVGUtils::PaintFrameWithEffects does.

Comment 6

[Jonathan Watt [:jwatt]]

 I like the idea of 

(In reply to Cameron McCormack (:heycam) from comment #4)
> If we want this to work, we could make nsSVGMarkerFrame inherit from
> nsSVGDisplayContainerFrame (rather than nsSVGContainerFrame) and to override
> BuildDisplayList, PaintSVG, GetBBoxContribution, etc. to make...

I'm not keen on this idea since it breaks the mental model of what nsSVGDisplayContainerFrame is vs nsSVGContainerFrame. That muddying of the code makes it harder to work with. E.g. adding methods to nsSVGDisplayContainerFrame may end up cause unexpected when you forget/don't know to make them a no-op for nsSVGMarkerFrame.

I think having a nsSVGUtils::PaintNonDisplayContainerFrameWithEffects would be preferable, or else giving nsSVGMarkerFrame an anonymous kid that wraps the real children and which applies any clip-path etc. properties specified on its nsSVGMarkerFrame parent.

Comment 7

[Robert Longson [:longsonr]]

I suspect the final paragraph of comment 6 may be the simplest/easiest approach. 

We should pass the bottom square part of this test once this is fixed: http://samples.msdn.microsoft.com/ietestcenter/svg112/svg_harness.htm?url=./svg112/svg/chapter_15.2.svg

We seem to have regressed the text aspect (the X) as we passed that at one time, although I'm looking at the X without the new text frames enabled at the moment. Something for another bug though.

Comment 8

[Jonathan Watt [:jwatt]]

Comment on attachment 767609 [details] [diff] [review]
WIP (v0.1)

I don't really want to f-, so cancelling for now until you give your thoughts on comment 6.

Comment 9

[Cameron McCormack (:heycam)]

Attachment: patch (v1)

I liked the give-nsSVGMarkerFrame-an-anonymous-kid approach.  This let me avoid having a separate nsSVGUtils::PaintNonDisplayContainerFrameWithEffects function.  Only a couple of changes to nsSVGUtils::PaintFrameWithEffects were needed.

Comment 10

[Jonathan Watt [:jwatt]]

I've not forgotten about this. I just need to block out more time to review it.

Comment 11

[Sebastian Widmer]

In addition to clip path issue, markers do not receive mouse events and are not recognized in inspector when hovered. Seems like markers are not treated as integral parts of shapes at all. Do you guys suggest a dedicated bug should be filed on mouse events not reaching markers?

Comment 12

[Jonathan Watt [:jwatt]]

Sebastian, I don't think it makes sense to pass events on to marker elements. The idea is that the graphic that is marked is treated as an atomic unit in terms of event handling. If you wanted to have that changed you would need to propose the behavior that you want on the [e-mail] mailing list.

Robert, thanks for taking over review.

Comment 13

[Sebastian Widmer]

(In reply to Jonathan Watt [:jwatt] from comment #12)

Jonathan, sorry for my obscure message; I meant, right now there is no way to detect if a marker (not its "owner" element) has been clicked/hovered.

Comment 14

[Robert Longson [:longsonr]]

No there isn't and that's not a bug as that's per the SVG specification. Please feel free to raise this on the w3c mailing list per comment 12.

Comment 15

[Robert Longson [:longsonr]]

Comment on attachment 779018 [details] [diff] [review]
patch (v1)

>+// Construct an nsSVGMarkerFrame, the anonymous child that wraps its real
>+// children, and its descendant frames.
>+nsIFrame*
>+nsCSSFrameConstructor::ConstructMarker(nsFrameConstructorState& aState,
>+                                       FrameConstructionItem&   aItem,
>+                                       nsIFrame*                aParentFrame,
>+                                       const nsStyleDisplay*    aDisplay,
>+                                       nsFrameItems&            aFrameItems)
>+{

...

>+
>+  nsFrameItems childItems;
>+
>+  // Process children
>+  if (aItem.mFCData->mBits & FCDATA_USE_CHILD_ITEMS) {
>+    ConstructFramesFromItemList(aState, aItem.mChildItems,
>+                                innerFrame, childItems);
>+  } else {
>+    ProcessChildren(aState, content, styleContext, innerFrame,
>+                    true, childItems, false, aItem.mPendingBinding);
>+  }
>+
>+  // Set the inner wrapper frame's initial primary list
>+  innerFrame->SetInitialChildList(kPrincipalList, childItems);
>+
>+  return newFrame;
>+}
>+

This whole method seems very similar to nsCSSFrameConstructor::ConstructConstructOuterSVG can we really not do more to share code here? At least the bit from the declaration of childItems on (i.e what I've quoted above) perhaps which looks identical.

> 
>   nsISVGChildFrame *svgChildFrame = do_QueryFrame(aFrame);
>   if (!svgChildFrame)
>     return;
> 
>-  float opacity = aFrame->StyleDisplay()->mOpacity;
>+  // Painting a marker is done by calling PaintFrameWithEffects on the
>+  // nsSVGMarkerFrame's anonymous child frame.  To handle the non-inherited
>+  // clip-path, filter, mask and opacity properties, we look up to
>+  // find the nsSVGMarkerFrame to get those from.
>+  nsIFrame* effectFrame = aFrame;
>+  if (effectFrame->GetType() == nsGkAtoms::svgMarkerAnonChildFrame) {
>+    effectFrame = effectFrame->GetParent();
>+  }

I'd prefer to see this done using the css inherit trick we used for text bidi and vector-effect. That should work shouldn't it? That would mean no code change here required then right?

Comment 16

[Cameron McCormack (:heycam)]

With comments addressed: https://tbpl.mozilla.org/?tree=Try&rev=cfa97a39cf6b

Comment 17

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/45283234d2d7

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/45283234d2d7