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

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 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.

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

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.

(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>

(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();

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.

(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 on attachment 604638 [details] [diff] [review] address other review comments I don't know how to proceed here.