732930 - foreignObject does not work in patterns

Status: NEW

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

If a foreignObject is a child of a defs then it does not render.

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

This patch addresses two issues.

a) foreignObject must have initialUpdate called on it if it is to display properly and initialUpdate stops descending at a defs frame currently

b) frames which are children of defs have an empty mRect as they only render via patterns and markers and one other thing I can't remember

Comment 2

[Jonathan Watt [:jwatt]]

Comment on attachment 602860 [details] [diff] [review]
patch

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

::: layout/svg/base/src/nsSVGContainerFrame.cpp
@@ -129,1 @@
>  

While you're here, can you add an NS_ABORT_IF_FALSE to check that this is not called on a foreignObject please?

::: layout/svg/base/src/nsSVGForeignObjectFrame.cpp
@@ -212,5 @@
>  nsSVGForeignObjectFrame::PaintSVG(nsRenderingContext *aContext,
>                                    const nsIntRect *aDirtyRect)
>  {
> -  if (IsDisabled())
> -    return NS_OK;

Why remove this?

@@ +542,5 @@
> +  // doomed to take the NS_FRAME_IS_DIRTY early-return below. To avoid that
> +  // problem, we need to bail out *before* we mark our kid as dirty.
> +  nsSVGForeignObjectElement *fO = static_cast<nsSVGForeignObjectElement*>
> +                                             (mContent);
> +  if (!fO->HasValidDimensions()) {

HasValidDimensions() seems buggy, and thus wrong to use here. If the computed 'font-size' on the element is zero, then if the width or height are in em units it doesn't matter what GetAnimValInSpecifiedUnits() returns, the width/height will be zero.

@@ +579,2 @@
>    // Skip reflow if we're zero-sized, unless this is our first reflow.
> +  if (!fO->HasValidDimensions() &&

Same issue here.

::: layout/svg/base/src/nsSVGUtils.cpp
@@ +963,5 @@
>      nsISVGChildFrame* SVGFrame = do_QueryFrame(kid);
>      if (SVGFrame) {
>        SVGFrame->NotifySVGChanged(aFlags); 
>      } else {
> +      if (kid->IsFrameOfType(nsIFrame::eSVG)) {

Can you make this:

  }
  else if (...) {

@@ +967,5 @@
> +      if (kid->IsFrameOfType(nsIFrame::eSVG)) {
> +        // recurse into the children of container frames e.g. <clipPath>, <mask>
> +        // in case they have child frames with transformation matrices
> +        NotifyChildrenOfSVGChange(kid, aFlags);
> +      }

Also, why did you have to get rid of the assertion?

@@ +1007,5 @@
>  }
>  
> +void
> +nsSVGUtils::InitialUpdate(nsIFrame *aFrame)
> +{

Please add another NS_ABORT_IF_FALSE to make sure we don't pass an nsSVGForeignObjectFrame to this.

Comment 3

[Robert Longson [:longsonr]]

Attachment: address review comments

Not sure what you mean with the first comment but I think this addresses everything else.

Comment 4

[Jonathan Watt [:jwatt]]

Comment on attachment 604407 [details] [diff] [review]
address review comments

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

I meant an NS_ABORT_IF_FALSE IN nsSVGDisplayContainerFrame::InsertFrames.

Please rename nsSVGUtils::InitialUpdate to nsSVGUtils::InitialUpdateChildren, since at the callers it looks like the request is for the frame itself to get an initial update.

I don't understand why you're having us stop using mRect for the width/height checking. It's values are set by UpdateCoveredRegion(), which uses GetAnimatedLengthValues(). Since GetAnimatedLengthValues can be a bit expensive if non-user unit values need to be resolved, it seems good to use the cached result in mRect. (I do think IsDisabled is a pretty unhelpful name though - WidthOrHeightIsZero would be more informative, although inlining the code would be just as good.)

Regarding nsSVGForeignObjectFrame::PaintSVG, moving the width/height check out of the |if| statement means that in the common case that visibility!=visible we will call GetAnimatedLengthValues, whereas usually we'd avoid it.

Comment 5

[Robert Longson [:longsonr]]

(In reply to Jonathan Watt [:jwatt] from comment #4)
> 
> I don't understand why you're having us stop using mRect for the
> width/height checking. It's values are set by UpdateCoveredRegion(), which

non-display children don't call UpdateCoveredRegion. nsSVGUtils::UpdateGraphic returns early in that case. mRect is always empty for objects that are children of <defs>

Comment 6

[Robert Longson [:longsonr]]

(In reply to Jonathan Watt [:jwatt] from comment #4)
> 
> I meant an NS_ABORT_IF_FALSE IN nsSVGDisplayContainerFrame::InsertFrames.

nsSVGForeignObjectFrame implements nsISVGChildFrame so we're always going to end up calling SVGFrame->InitialUpdate();

Comment 7

[Robert Longson [:longsonr]]

Attachment: address other review comments

Comment 8

[Robert Longson [:longsonr]]

void
nsSVGUtils::UpdateGraphic(nsIFrame *aFrame)
{
  nsSVGEffects::InvalidateRenderingObservers(aFrame);

  if (aFrame->GetStateBits() & NS_STATE_SVG_NONDISPLAY_CHILD)
    return;

So we never call UpdateRegion with non-display children.

Comment 9

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #5)
> mRect is always empty for objects that are children of <defs>

Ah, right.

On further reflection, it seems that making InitialUpdate cross the NS_STATE_SVG_NONDISPLAY_CHILD boundary isn't going to work properly and isn't the right approach. Consider how the frames for mask, clipPath, pattern and marker paint. Before painting their contents, they call NotifySVGChange on their children to have them update for the element they're currently being painted for. If a fO is hit under that call, it will currently resize if necessary and *schedule* a reflow; but what we need it to do for correct results is to *synchronously* reflow, since we're going to paint that fO *right now*. So while your testcase may pass, it would seem that that will only be the case when reflowing during the the outer-<svg>'s initial reflow by jumping the NS_STATE_SVG_NONDISPLAY_CHILD boundary happens to produce the same layout in the fO as the layout that should be produced for the element using it at any given point in the document. It seems to me that fO with more complex content, or used by multiple elements (via mask, clipPath, pattern or marker) in different contexts would usually fail.

I'm not sure whether we can do synchronous reflow under nsSVGForeignObjectFrame::NotifySVGChanged. At any rate, we'd want to do that only if the NS_STATE_SVG_NONDISPLAY_CHILD bit is set on the frame.

Comment 10

[Robert Longson [:longsonr]]

Comment on attachment 604638 [details] [diff] [review]
address other review comments

I don't know how to proceed here.