734082 - Set mRect and the overflow rect on SVG leaf and container frames

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

The display list code heavily depends on frames' mRect (at least the x/y part) and visual overflow rect containing appropriate values. To fix bug 614732 (use display lists for SVG painting and hit-testing) we need to make sure that this information is set on SVG frames, as appropriate. The patch in bug 614732 to replace covered regions on SVG leaf frames with user space bounds got us part of the way there. However, we still need to set the visual overflow region on leaf frames when appropriate, and both mRect and the visual overflow region on container frames.

The bulk of the work to fix this bug will be fixing bug 734079 to implement a reflow-like mechanism for SVG. Once that's done, we should be able to add code to the ReflowSVG (or whatever we call them) methods so that after reflowing their children, they then use their children's bounds to update their own mRect and visual overflow rect.

An additional advantage of having SVG containers maintain a visual overflow rect is that we can fix bug 309782, because then we can limit the size of the offscreen surface created for a container with group opacity to its overflow rect.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

This patch uses FinishAndStoreOverflow to store overflows on SVG containers. Right now the only thing it does with those overflows is use them to short circuit painting in nsSVGUtils::PaintFrameWithEffects (so we no longer have to traverse to all leaf frames). I'm also using them to subsequent WIP patches for bug 614732 to determine whether or not to create display list items for a frame.

One thing to note about this patch is the way I'm handling the viewBox attribute and the currentScale/currentTranslate properties. I am using nsDisplayTransform to handle these transforms, but it's a bit tricky because these transforms are unique. They are unique in the sense that they do not have an affect on the frame to which they belong. They only transform the frame's children. I've termed these "children-only" transforms, and the way I'm handling them (at least for now) is similar to the way we handle preserve-3d. What I do is have IsTransformed on the _children_ check for children-only transforms on their parent. We therefore count the children as being transformed despite the details of the transform actually being specified on the parent. At some future time we may want to try to create a separate display list item for this type of transform, or to have nsDisplayTransform maintain separate "normal" and "children-only" transforms, but I found that pretty hard to do writing the patch from scratch.

Comment 2

[Jonathan Watt [:jwatt]]

The XXXSDL markers are for things I have/will address in the subsequent SVG display list patches.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

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

::: content/svg/content/src/nsSVGGraphicElement.cpp
@@ +201,5 @@
> +      // XXXSDL Currently we don't need to reconstruct SVG frames when their
> +      // transform is set/unset since we don't currently create GFX layers for
> +      // SVG transforms, but we will after bug 614732 is fixed. Also change the
> +      // assertion in ApplyRenderingChangeToTree when we do that.
> +      NS_UpdateHint(retval, nsChangeHint_UpdateOverflow);

Why not just set this to ReconstructFrame now? Should be safe, and it's future-proof.

::: content/svg/content/src/nsSVGSVGElement.cpp
@@ +204,5 @@
>      mPreviousScale(1.0f),
>      mStartAnimationOnBindToTree(!aFromParser),
>      mImageNeedsTransformInvalidation(false),
>      mIsPaintingSVGImageElement(false)
> +  , mHasChildrenOnlyTransform(false)

put command at end of previous line for consistency

::: layout/base/nsCSSFrameConstructor.cpp
@@ +7666,5 @@
>        aFrame->InvalidateTransformLayer();
>      }
> +#if 0
> +    // XXXSDL Once we use GFX layers for SVG transforms, we should do
> +    // something along the lines of the following.

Why not just put this in without #if 0?

@@ +8003,5 @@
> +            if (pre) {
> +              // FinishAndStoreOverflow will change the overflow areas passed in,
> +              // so make a copy.
> +              nsOverflowAreas overflowAreas = *pre;
> +              childFrame->FinishAndStoreOverflow(overflowAreas, childFrame->GetSize());

This isn't right since FinishAndStoreOverflow adds outline and filter effects to the overflow area before storing PreTransformOverflowAreasProperty, and here you're going to add them again. I think you should just call UpdateOverflow().

::: layout/base/nsDisplayList.cpp
@@ +2781,4 @@
>    }
>  
> +  return nsLayoutUtils::ChangeMatrixBasis(newOrigin + toMozOrigin,
> +                          result * gfx3DMatrix::From2D(transformFromSVGParent));

transformFromSVGParent won't be valid unless hasSVGTransforms is true, right? So we should make these multiplies conditional on hasSVGTransforms. A helper function that takes three parameters (two matrices and a bool) would be useful for that.

::: layout/generic/nsFrame.cpp
@@ +6792,5 @@
>    }
>  
>    // Overflow area must always include the frame's top-left and bottom-right,
>    // even if the frame rect is empty.
> +  // XXX why?

To ensure we can scroll to those positions.

::: layout/generic/nsIFrame.h
@@ +314,5 @@
>  
> +// This bit is set on SVG frames that are laid out using SVG's coordinate
> +// system based layout (as opposed to any of the CSS layout models). Note that
> +// this does not include nsSVGOuterSVGFrame since it takes part is CSS layout.
> +#define NS_FRAME_SVG_WITH_SVG_LAYOUT                NS_FRAME_STATE_BIT(43)

Call this NS_FRAME_SVG_LAYOUT

::: layout/svg/base/src/nsSVGEffects.cpp
@@ +528,5 @@
> +  if (!prop) {
> +    // Create the property if we haven't done that yet
> +    prop = static_cast<nsSVGFilterProperty*>
> +             (GetEffectProperty(aFrame->GetStyleSVGReset()->mFilter,
> +                                aFrame, FilterProperty(), CreateFilterProperty));

Why do we need to do this? It's not good because during invalidation we rely on deleting the property to indicate that the filtered content has already been invalidated.

::: layout/svg/base/src/nsSVGGlyphFrame.cpp
@@ +511,5 @@
>    // See bug 614732 comment 32.
>    mCoveredRegion = nsSVGUtils::TransformFrameRectToOuterSVG(
>      mRect, GetCanvasTM(), PresContext());
>  
> +  // XXXSDL: what should we really be doing here?

I think this is fine.

::: layout/svg/base/src/nsSVGUtils.cpp
@@ +1837,5 @@
> +
> +/* static */ void
> +nsSVGUtils::PostRestyleEvent(Element* aElement,
> +                             nsRestyleHint aRestyleHint,
> +                             nsChangeHint aMinChangeHint)

Put this in nsLayoutUtils?

::: layout/svg/base/src/svg.css
@@ +73,5 @@
> +   noting that 'svg' as a child of 'foreignObject' counts as outer-<svg>.
> +*/
> +*:not(svg),
> +*:not(foreignObject) > svg {
> +  -moz-transform-origin:0 0;

I assume the SVG spec is going to say this.

Comment 4

[Jonathan Watt [:jwatt]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> Why not just set this to ReconstructFrame now? Should be safe, and it's
> future-proof.

Because it makes more sense to me that way, but more importantly because I want any perf gains/loses to be attributable to the logical piece to which they belong (the logical piece that made a change necessary).

> Why not just put this in without #if 0?

Same reason. I should probably just move that to the next patch rather than have it in this one ifdef'ed out.

> This isn't right since FinishAndStoreOverflow adds outline and filter
> effects to the overflow area before storing
> PreTransformOverflowAreasProperty, and here you're going to add them again.
> I think you should just call UpdateOverflow().

I took this code from the |while| immediately below. Is that existing code wrong too?

> transformFromSVGParent won't be valid unless hasSVGTransforms is true,
> right?

Good catch.

> Why do we need to do this? It's not good because during invalidation we rely
> on deleting the property to indicate that the filtered content has already
> been invalidated.

I can't remember the details now, but I was getting test failures and tracked it down to that function acting as if we didn't have a filter when we do. I'll need to remove that and run the tests and get back to you on that one.

> I assume the SVG spec is going to say this.

I'm not sure about the SVG spec, but the css-transforms spec already does:

http://dev.w3.org/csswg/css3-transforms/#transform-origin

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jonathan Watt [:jwatt] from comment #4)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> > Why not just set this to ReconstructFrame now? Should be safe, and it's
> > future-proof.
> 
> Because it makes more sense to me that way, but more importantly because I
> want any perf gains/loses to be attributable to the logical piece to which
> they belong (the logical piece that made a change necessary).
> 
> > Why not just put this in without #if 0?
> 
> Same reason. I should probably just move that to the next patch rather than
> have it in this one ifdef'ed out.

OK.

> > This isn't right since FinishAndStoreOverflow adds outline and filter
> > effects to the overflow area before storing
> > PreTransformOverflowAreasProperty, and here you're going to add them again.
> > I think you should just call UpdateOverflow().
> 
> I took this code from the |while| immediately below. Is that existing code
> wrong too?

Yes, there's a bug on it.

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
 ::: layout/svg/base/src/nsSVGEffects.cpp
> @@ +528,5 @@
> > +  if (!prop) {
> > +    // Create the property if we haven't done that yet
> > +    prop = static_cast<nsSVGFilterProperty*>
> > +             (GetEffectProperty(aFrame->GetStyleSVGReset()->mFilter,
> > +                                aFrame, FilterProperty(), CreateFilterProperty));
> 
> Why do we need to do this? It's not good because during invalidation we rely
> on deleting the property to indicate that the filtered content has already
> been invalidated.

Without it, the following three tests fail:

https://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/filters/feMerge-2.svg
https://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/filters/feTile-2.svg
https://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/filters/feComposite-1.svg

The problem occurs with filtered, empty <g> elements. The problem with these tests is that when we do the initial pre-first-paint update of the bounds of the frames, we don't find a filter property. As a result the empty <g>'s get empty overflow rects, which means they fail the new check in PaintFrameWithEffects and are not painted.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jonathan Watt [:jwatt] from comment #6)
> https://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/filters/
> feMerge-2.svg
> https://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/filters/
> feTile-2.svg
> https://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/filters/
> feComposite-1.svg
> 
> The problem occurs with filtered, empty <g> elements. The problem with these
> tests is that when we do the initial pre-first-paint update of the bounds of
> the frames, we don't find a filter property.

OK, then during reflow we need to create this property when it's not present. During painting we should never create this property.

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: patch

This should address all comments so far.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

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

::: layout/svg/base/src/nsSVGEffects.cpp
@@ +526,3 @@
>      (aFrame->Properties().Get(FilterProperty()));
> +
> +  return prop;

Just remove the change to this file

Comment 10

[Jonathan Watt [:jwatt]]

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/05a339620439

Thanks!

Comment 11

[Matt Brubeck (:mbrubeck)]

This bug (or bug 614732) caused a small regression in the Talos SVG benchmark:
http://graphs.mozilla.org/graph.html#tests=[[22,63,13]]&sel=1337144258000,1337317058000&displayrange=7&datatype=running

Should it be backed out until the benchmark is fixed?

Comment 12

[Jonathan Watt [:jwatt]]

No, that's actually not too surprising since we're currently doing extra work that we're not yet leveraging, and have yet to remove soon to be dead code that is still doing work. There are more patches coming that will more than make up for the hit, so just leave it in. Thanks, Matt.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/mozilla-central/rev/05a339620439