769742 - Account for nsSVGOuterSVGFrames' border/padding offset

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Normally the Position() of a frame is its offset from the top-left of the border box of its geometric parent. Breaking this invariant breaks things like nsIFrame::GetOffsetTo().

SVG child frames (SVG descendants of nsSVGOuterSVGFrame) use their mRect to store their "user space" offset. That is, their offset from the coordinate system established by their mContent's 'transform' attribute or, if none, the coordinate system established for them by their parent. Another way of thinking of it is as their offset from their parent's _content_ box.

This can't really be changed for various reasons, including having nsDisplayTransform play nice with SVG.

For SVG frames, with the exception of nsSVGOuterSVGFrame, the border and content boxes are actually the same thing, since border/padding do not apply to them. So GetOffsetTo works, and the SVG code is happy. The one case where things break down is when the outer-<svg> has non-zero border/padding. In this scenario the mRects of all its immediate children are wrong from the perspective of GetOffsetTo, and things break. Specifically, display list painting of SVG doesn't offset the SVG for the border/padding, since ToReferenceFrame now returns incorrect offsets.

I'm not sure that this is the best way to deal with the issue, but right now I'm thinking of giving nsSVGOuterSVGFrame an anonymous child to wrap its real children, and setting the anonymous child's mRect to account for any border/padding on the nsSVGOuterSVGFrame.

Comment 1

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

(In reply to Jonathan Watt [:jwatt] from comment #0)
> I'm not sure that this is the best way to deal with the issue, but right now
> I'm thinking of giving nsSVGOuterSVGFrame an anonymous child to wrap its
> real children, and setting the anonymous child's mRect to account for any
> border/padding on the nsSVGOuterSVGFrame.

That's a reasonable option. Another option would be to wrap nsSVGOuterSVGFrame itself in some wrapper.

If we fix bug 378923 by wrapping an outer <svg> in an nsHTMLScrollFrame, that would actually fix this ... except for overflow:visible <svg>s. But it wouldn't be much work to teach nsHTMLScrollFrame to handle overflow:visible (rename nsGfxScrollFrameInner::IsIgnoringViewportClipping to IsIgnoringClipping and make it return true for overflow:visible), and just always construct an nsHTMLScrollFrame for an outer <svg>. (Except for root element <svg>, since we already construct a scrollframe for that.)

Comment 2

[Jonathan Watt [:jwatt]]

I did briefly discuss wrapping the nsSVGOuterSVGFrame with bz, but he was of the opinion that that would be harder, and a more invasive hack.

Besides, wrapping it in an nsHTMLScrollFrame wouldn't help, would it? I'm not familiar with nsHTMLScrollFrame, but presumably we wouldn't be adding an offset to its mRect for the nsSVGOuterSVGFrame's border/padding, and if we did, then it would offset the _border_ box of the nsSVGOuterSVGFrame, which is not what we want. We just want to offset its content.

Comment 3

[Jonathan Watt [:jwatt]]

Also, would the nsHTMLScrollFrame always exist, even when overflow is set to hidden/scroll?

Comment 4

[Jonathan Watt [:jwatt]]

Specifically about the wrap-the-outer-svg option he said:

> This is actually a bit of a pain to do.
>
> Your options are to make the block have the main style context and 
> explicitly inherit any not-inherited-by-default styles that 
> SVGOuterFrame needs into it, or to do something like tables where the 
> parent is actually a style child.  The table setup is ... annoying. 
> You'd have to find all the places that explicitly deal with it.
>
> I think it would be much simpler to just have a custom SVG frame which 
> sits inside the nsSVGOuterSVGFrame....  I guess then you also have 
> issues with its mRect, though?

Comment 5

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

(In reply to Jonathan Watt [:jwatt] from comment #2)
> Besides, wrapping it in an nsHTMLScrollFrame wouldn't help, would it? I'm
> not familiar with nsHTMLScrollFrame, but presumably we wouldn't be adding an
> offset to its mRect for the nsSVGOuterSVGFrame's border/padding, and if we
> did, then it would offset the _border_ box of the nsSVGOuterSVGFrame, which
> is not what we want. We just want to offset its content.

Yeah actually I was wrong. When you wrap something in a scrollframe, the scrollframe gets the border, but the child gets the padding. So it's not quite enough for you.

So yeah, a custom child frame is what you want.

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 7

[Jonathan Watt [:jwatt]]

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

Might be good to have you look over the nsCSSFrameConstructor changes, bz.

Comment 8

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

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

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

::: content/base/src/nsGkAtomList.h
@@ +1747,5 @@
>  GK_ATOM(svgLinearGradientFrame, "SVGLinearGradientFrame")
>  GK_ATOM(svgMarkerFrame, "SVGMarkerFrame")
>  GK_ATOM(svgMaskFrame, "SVGMaskFrame")
>  GK_ATOM(svgOuterSVGFrame, "SVGOuterSVGFrame")
> +GK_ATOM(svgOuterSVGInnerFrame, "SVGOuterSVGInnerFrame")

confusing! How about SVGOuterAnonymousChildFrame?

::: layout/base/nsCSSFrameConstructor.cpp
@@ +4736,5 @@
> +
> +  // Create the pseudo SC for the anonymous wrapper child as a child of the SC:
> +  nsRefPtr<nsStyleContext> scForAnon;
> +  scForAnon = mPresShell->StyleSet()->
> +    ResolveAnonymousBoxStyle(nsCSSAnonBoxes::mozSVGOuterSVGContent,

mozSVGOuterAnonChild?

::: layout/svg/base/src/nsSVGOuterSVGFrame.cpp
@@ +696,5 @@
> +  // independently, so we don't call nsSVGUtils::PaintFrameWithEffects on it.
> +  for (nsIFrame* kid = GetFirstPrincipalChild()->GetFirstPrincipalChild();
> +       kid;
> +       kid = kid->GetNextSibling()) {
> +    nsSVGUtils::PaintFrameWithEffects(aContext, aDirtyRect, kid);

Skipping the anonymous child here probably isn't the safest approach. Better to call PaintSVG on it.

::: layout/svg/base/src/svg.css
@@ +31,5 @@
>  }
>  
> +*|*::-moz-svg-outer-svg-content {
> +  border: none !important;
> +  padding: 0 !important;

-moz-svg-outer-anon-child

You don't need these rules, though. There's no way this frame could be assigned border or padding.

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 10

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

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

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

Looks great other than that.

::: layout/base/nsCSSFrameConstructor.cpp
@@ +4775,5 @@
> +    rv = ProcessChildren(aState, content, styleContext, innerFrame,
> +                         true, childItems, false, aItem.mPendingBinding);
> +  }
> +  // XXXbz what about cleaning up?
> +  if (NS_FAILED(rv)) return rv;

Yeah, what about cleaning up? :-)

Comment 11

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e0f0a9228ce

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> Yeah, what about cleaning up? :-)

It isn't immediately clear to me what needs to be done, so I'll have to do that as a follow-up (and fix the other methods with the same comment?).

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6e0f0a9228ce

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Sorry for the lag here...

This looks fine in general, even though I hate the code duplication.

One red flag: before we had FCDATA_DISALLOW_GENERATED_CONTENT but now we pass true to ProcessChildren for aCanHaveGeneratedContent.  Was that a purposeful change?

One orange flag: You could have just asserted that !(aItem.mFCData->mBits & FCDATA_USE_CHILD_ITEMS).

Comment 14

[Jonathan Watt [:jwatt]]

Thanks Boris. I'm going to leave this open and circle around and address your comments shortly.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

It's really confusing to [leave open] a bug whose patch landed and is fixed as-summarized, just to track unrelated followup work. You should just file followups for that instead.

Comment 16

[Jonathan Watt [:jwatt]]

Please don't close bugs that aren't yours. That's a really good way to make potentially important work fall of the radar.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Filed bug 774460.