1281215 - The outline of a SVG container should be narrowed to its children.

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daosheng Mu[:daoshengmu]]

Attachment: The left one is FF Nightly, and the right one is Chrome.

As discussion at Bug 778654 comment 90, after applying the patch of SVG outline, we notice that the origin of a SVG container outline would refer its transform. We expect its outline box should be the bounding box of its child elements.

Comment 1

[Daosheng Mu[:daoshengmu]]

Attachment: SVG container sample

Comment 2

[Daosheng Mu[:daoshengmu]]

Attachment: CSS outline sample

Comment 3

[Daosheng Mu[:daoshengmu]]

We expect the result of attachment 8763947 [details] should be the same with attachment 8763949 [details]. Please use FF 50 to open them.

Comment 4

[Daosheng Mu[:daoshengmu]]

Attachment: CSS outline sample

Update CSS outline sample. But this sample has different result in Google Chrome.

Comment 5

[Daosheng Mu[:daoshengmu]]

In the case of attachment 8763947 [details], its rect is (0, 0, 4200, 4200) at https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp#878. Because the coordinate unit is 60, that makes sense to get rect pixels (x, y, w, h) = (0, 0, 70, 70). 70 is from circle.cx + circle.r.

If we just want the outline to wrap its child elements, its rect needs to be (1800, 1800, 2400, 2400). That means rect pixels (x, y, w, h) = (30, 30, 40, 40). 30 is from circle.cx - circle.r, and 40 is 2*r.

I am not quite sure we should follow what Chrome did or remain the original one.

Heycam, any thought?

Comment 6

[Daosheng Mu[:daoshengmu]]

(In reply to Daosheng Mu[:daoshengmu] from comment #5)
> In the case of attachment 8763947 [details], its rect is (0, 0, 4200, 4200)
> at
> https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.
> cpp#878. Because the coordinate unit is 60, that makes sense to get rect
> pixels (x, y, w, h) = (0, 0, 70, 70). 70 is from circle.cx + circle.r.
> 
> If we just want the outline to wrap its child elements, its rect needs to be
> (1800, 1800, 2400, 2400). That means rect pixels (x, y, w, h) = (30, 30, 40,
> 40). 30 is from circle.cx - circle.r, and 40 is 2*r.
> 
> I am not quite sure we should follow what Chrome did or remain the original
> one.
> 
> Heycam, any thought?

Hey, jwatt. I think you would have some idea can share.

Comment 7

[Daosheng Mu[:daoshengmu]]

(In reply to Daosheng Mu[:daoshengmu] from comment #4)
> Created attachment 8764132 [details]
> CSS outline sample
> 
> Update CSS outline sample. But this sample has different result in Google
> Chrome.

Both attachment 8763947 [details] and attachment 8764132 [details] can have the same result in FF Nightly 50. Therefore, I think that is acceptable to let SVG keep using the original implementation.

But Chrome is still different to us, I don't know if there is a spec can describe we should follow which rule?

Comment 8

[Cameron McCormack (:heycam)]

I don't think there is anything in the SVG spec that says where the outline should be painted for SVG elements, but I think it does make sense to use the normal bounding box of the element: I think that is more useful and what authors would expect.

Comment 9

[Daosheng Mu[:daoshengmu]]

Attachment: makingSVGTightly.patch

I notice this issue is because ComputeAndIncludeOutlineArea() would check if the frame has nsCSSAnonBoxes::mozAnonymousBlock or nsCSSAnonBoxes::mozAnonymousPositionedBlock pseudo to let it use its children's bounding as its outline. Therefore, I think the outline of the general frames would not narrow to their children.

If we would like to make SVG supports this, the best method is to make SVG container can have nsCSSAnonBoxes::mozAnonymousBlock or nsCSSAnonBoxes::mozAnonymousPositionedBlock pseudo. But, as far as I know, SVG will not parse pseudo?

Another way is like the patch I filed, checking the type of frames. If it is SVGGFrame, we let it use childFrame's bounding.

Comment 10

[Robert Longson [:longsonr]]

What about <a> elements? They can act as containers in a similar way to <g>

What about <use> pointing to something other than a symbol or an svg element: https://www.w3.org/TR/SVG/struct.html#UseElement (Otherwise: paragraph)

And <switch> which paints one child. Does that work?

Comment 11

[Daosheng Mu[:daoshengmu]]

(In reply to Robert Longson from comment #10)
> What about <a> elements? They can act as containers in a similar way to <g>
> 
> What about <use> pointing to something other than a symbol or an svg
> element: https://www.w3.org/TR/SVG/struct.html#UseElement (Otherwise:
> paragraph)
> 
> And <switch> which paints one child. Does that work?

Yep. That is what I would like to discuss. I think it is a good idea to make nsSVGDisplayContainerFrame has this behavior, and we can use nsSVGDisplayContainerFrame::IsDisplayContainer() to check them.

Btw, I notice when SVGTextFrame has this behavior that would make its outline width/height to be float value. It would bring me a problem on writing outline reftest at bug 778654.

Comment 12

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8766759 [details] [diff] [review]
makingSVGTightly.patch

heycam, I have found where we should modify to make this function works. I am curious should we make all nsSVGDisplayContainerFrame work for this? or just for <g>?

Thanks.

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8766759 [details] [diff] [review]
makingSVGTightly.patch

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

I think we want to make this work for all nsSVGDisplayContainerFrames, so that it also includes <svg> elements.

But I wonder whether we should be fixing this somewhere else.  I don't understand why the rectangle that gets computed for an nsSVGGFrame includes the (0,0) point even if none of its children rectangles do.  Maybe we are doing something wrong in nsSVGDisplayContainerFrame::ReflowSVG(), which I think is where we compute mRect.  Can you find out why?

Comment 14

[Daosheng Mu[:daoshengmu]]

(In reply to Cameron McCormack (:heycam) from comment #13)
> Comment on attachment 8766759 [details] [diff] [review]
> makingSVGTightly.patch
> 
> Review of attachment 8766759 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think we want to make this work for all nsSVGDisplayContainerFrames, so
> that it also includes <svg> elements.
> 
> But I wonder whether we should be fixing this somewhere else.  I don't
> understand why the rectangle that gets computed for an nsSVGGFrame includes
> the (0,0) point even if none of its children rectangles do.  Maybe we are
> doing something wrong in nsSVGDisplayContainerFrame::ReflowSVG(), which I
> think is where we compute mRect.  Can you find out why?

Hi heycam,
I have checked nsSVGDisplayContainerFrame::ReflowSVG. I find even though we make some changes at nsSVGDisplayContainerFrames::ReflowSVG(), we still have the same result. Unless we want to get OutlineInnerRectProperty and recompute the outline from children frames at the end of this function. Please take a look at the code flow:

#0	0x0000000111d9d700 in ComputeAndIncludeOutlineArea(nsIFrame*, nsOverflowAreas&, nsSize const&) at /Volumes/FirefoxHD/gecko-dev/layout/generic/nsFrame.cpp:7982
#1	0x0000000111d979d2 in nsIFrame::FinishAndStoreOverflow(nsOverflowAreas&, nsSize, nsSize*) at /Volumes/FirefoxHD/gecko-dev/layout/generic/nsFrame.cpp:8124
#2	0x0000000111f3f1ab in nsSVGDisplayContainerFrame::ReflowSVG() at /Volumes/FirefoxHD/gecko-dev/layout/svg/nsSVGContainerFrame.cpp:401

In UnionBorderBoxes() (https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp?q=UnionBorderBoxes&redirect_type=direct#7981), it always starts at (0, 0). If we would like to make the SVGDisaplayContainer's outline be small, we have to use UnionRect() and get the frameForArea from its children. That is what I did in attachment 8766759 [details] [diff] [review].

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8766759 [details] [diff] [review]
makingSVGTightly.patch

I understand now.  I think your approach is good, but we will still need to handle all SVG container frames.  You can call IsFrameOfType(nsIFrame::eSVGContainer) for that.

Comment 16

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1281215 - Making the outline of SVG container narrows to its children;

Review commit: https://reviewboard.mozilla.org/r/66982/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66982/

Comment 17

[Daosheng Mu[:daoshengmu]]

Try result is good. https://treeherder.mozilla.org/#/jobs?repo=try&revision=811daa6df5af

I know I need to give a reftest. I have prepared it and I will put it at the third patch (test files) of Bug 778654

Comment 18

[Cameron McCormack (:heycam)]

https://reviewboard.mozilla.org/r/66982/#review63926

::: layout/generic/nsFrame.cpp:7966
(Diff revision 1)
> +    if (!frameForArea->IsFrameOfType(nsIFrame::eSVGContainer) &&
> +        pseudoType != nsCSSAnonBoxes::mozAnonymousBlock &&
>          pseudoType != nsCSSAnonBoxes::mozAnonymousPositionedBlock)
>        break;

Oh, I just realized this is in a loop, and we'll traverse down to the first child of each SVG container frame until we find a non-container frame.  So for example with this:

  <svg ...>
    <g tabindex="0">
      <g>
        <circle .../>
      </g>
      <rect .../>
    </g>
  </svg>

The outline for the <g> with tabindex set would include the circle but not the rect, since we traverse down the first child until finding the circle.  Is this right?  Do we have to handle this differently?

Comment 19

[Daosheng Mu[:daoshengmu]]

(In reply to Cameron McCormack (:heycam) from comment #18)
> https://reviewboard.mozilla.org/r/66982/#review63926
> 
> ::: layout/generic/nsFrame.cpp:7966
> (Diff revision 1)
> > +    if (!frameForArea->IsFrameOfType(nsIFrame::eSVGContainer) &&
> > +        pseudoType != nsCSSAnonBoxes::mozAnonymousBlock &&
> >          pseudoType != nsCSSAnonBoxes::mozAnonymousPositionedBlock)
> >        break;
> 
> Oh, I just realized this is in a loop, and we'll traverse down to the first
> child of each SVG container frame until we find a non-container frame.  So
> for example with this:
> 
>   <svg ...>
>     <g tabindex="0">
>       <g>
>         <circle .../>
>       </g>
>       <rect .../>
>     </g>
>   </svg>
> 
> The outline for the <g> with tabindex set would include the circle but not
> the rect, since we traverse down the first child until finding the circle. 
> Is this right?  Do we have to handle this differently?

In this case, it will be wrong. I forget to consider this case. We need to handle it. I cancel the review, please look forward my next update. Thanks!

Comment 20

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8774544 [details]
Bug 1281215 - Making the outline of SVG container narrows to its children;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66982/diff/1-2/

Comment 21

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8774544 [details]
Bug 1281215 - Making the outline of SVG container narrows to its children;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66982/diff/2-3/

Comment 22

[Daosheng Mu[:daoshengmu]]

In this update, I create a ComputeSVGContainerOutlineArea() that is specific for SVGContainer. Because the outline of SVGContainer has to consider its children whether is a SVGContainer either. Therefore, I decide to use recursive method to handle it.
Try result is also good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29eb15150512

Comment 23

[Cameron McCormack (:heycam)]

https://reviewboard.mozilla.org/r/66982/#review64702

I am concerned about the duplication of code here.  Is there a way we can avoid copying code from ComputeAndIncludeOutlineArea into ComputeSVGContainerOutlineArea?

I wonder if an alternative approach might be simpler.  The problem, as I understand it, is that UnionBorderBoxes will always union in the rectangle returned for the current frame (and each descendant frame), but for SVG container frames we need to skip doing that.  However, just using an nsRect is not enough, because we can't distinguish between (0, 0, 0, 0) for a frame that we should skip, and a non-SVG container frame that had that size, which we do want to include.  So we could use a bool outparam in UnionBorderBoxes to record whether the rectangle is valid or not.  Then, when we call UnionRectEdges, we can first check this bool, and if it is false, just assign the right-hand-side instead of unioning it.  And if the recursive UnionBorderBoxes call returned false, then we can skip the unioning and leave the current |u| value.

Does that make sense, and do you think it will be simpler than a separate ComputeSVGContainerOutlineArea function?

::: layout/generic/nsFrame.cpp:7941
(Diff revision 3)
>  
>    return u;
>  }
>  
>  static void
> +ComputeSVGContainerOutlineArea(const nsIFrame* aframeForArea, nsRect& aInnerRect) {

Nit: "{" on new line.

::: layout/generic/nsFrame.cpp:7953
(Diff revision 3)
> +      nsRect r;
> +      // If its sibling is a SVGContainer, we need to process its children,
> +      // and get its computed outline rect for unioning rect.
> +      if (frameForArea->IsFrameOfType(nsIFrame::eSVGContainer)) {
> +        ComputeSVGContainerOutlineArea(frameForArea, aInnerRect);
> +        r = *frameForArea->Properties().Get(nsIFrame::OutlineInnerRectProperty());

I don't understand why we do this.  Haven't we already accumulated the outline for the descendants of this SVG conatiner frame in aInnerRect, with the call on the previous line?

::: layout/generic/nsFrame.cpp:7994
(Diff revision 3)
> +  // When the frame is SVGContainer, we don't want a really wide
> +  // outline if the block inside the inline is narrow, so union the
> +  // actual contents of the anonymous blocks.

I don't think this comment makes sense.  "block inside inline" refers to cases like this:

  <style>
    #x { display: block; }
  </style>
  <span>abc<span id=x>def</span>ghi</span>

We should mention in the comment why the computed outline area will be larger than we want when we have an SVG container frame.  (The reason being that SVG container frames do not maintain an accurate mRect.)

So how about something like this:

// SVG container frames do not maintain an accurate mRect, so we need to
// compute their outline areas specially.

Nit: maybe put the comment inside the |if| statement, since the comment for the other branch is inside the |else| statement.

Comment 24

[Robert Longson [:longsonr]]

(In reply to Cameron McCormack (:heycam) from comment #23)
> https://reviewboard.mozilla.org/r/66982/#review64702
> 
> I am concerned about the duplication of code here.  Is there a way we can
> avoid copying code from ComputeAndIncludeOutlineArea into
> ComputeSVGContainerOutlineArea?
> 
> I wonder if an alternative approach might be simpler.  The problem, as I
> understand it, is that UnionBorderBoxes will always union in the rectangle
> returned for the current frame (and each descendant frame), but for SVG
> container frames we need to skip doing that.  However, just using an nsRect
> is not enough, because we can't distinguish between (0, 0, 0, 0) for a frame
> that we should skip, and a non-SVG container frame that had that size, which
> we do want to include.  So we could use a bool outparam in UnionBorderBoxes
> to record whether the rectangle is valid or not.  Then, when we call
> UnionRectEdges, we can first check this bool, and if it is false, just
> assign the right-hand-side instead of unioning it.  And if the recursive
> UnionBorderBoxes call returned false, then we can skip the unioning and
> leave the current |u| value.
> 

We already do something similar when calculating the mRect for various SVG objects (see SVGBBox in nsSVGUtils.h). Can we reuse that? https://dxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGUtils.h#81

Comment 25

[Cameron McCormack (:heycam)]

Yes, though if we do we should probably move it and rename it to something less SVG-specific sounding, since we'd need to make ComputeAndIncludeOutlineArea / UnionBorderBoxes use this type for all frames, not just the SVG ones.

Comment 26

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8774544 [details]
Bug 1281215 - Making the outline of SVG container narrows to its children;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66982/diff/3-4/

Comment 27

[Daosheng Mu[:daoshengmu]]

(In reply to Cameron McCormack (:heycam) from comment #23)
> https://reviewboard.mozilla.org/r/66982/#review64702
> 
> I am concerned about the duplication of code here.  Is there a way we can
> avoid copying code from ComputeAndIncludeOutlineArea into
> ComputeSVGContainerOutlineArea?
> 
> I wonder if an alternative approach might be simpler.  The problem, as I
> understand it, is that UnionBorderBoxes will always union in the rectangle
> returned for the current frame (and each descendant frame), but for SVG
> container frames we need to skip doing that.  However, just using an nsRect
> is not enough, because we can't distinguish between (0, 0, 0, 0) for a frame
> that we should skip, and a non-SVG container frame that had that size, which
> we do want to include.  So we could use a bool outparam in UnionBorderBoxes
> to record whether the rectangle is valid or not.  Then, when we call
> UnionRectEdges, we can first check this bool, and if it is false, just
> assign the right-hand-side instead of unioning it.  And if the recursive
> UnionBorderBoxes call returned false, then we can skip the unioning and
> leave the current |u| value.
> 
> Does that make sense, and do you think it will be simpler than a separate
> ComputeSVGContainerOutlineArea function?
> 

Awesome! Your idea is more simple and can make this works. Thanks for your feedback.

In this update, I assume SVGContainer frame will have an invalid rect. If its children has any non-SVGContainer, it can assign its rect to the parent, SVGContainter. Otherwise, do UnionRectEdges().


> ::: layout/generic/nsFrame.cpp:7941
> (Diff revision 3)
> >  
> >    return u;
> >  }
> >  
> >  static void
> > +ComputeSVGContainerOutlineArea(const nsIFrame* aframeForArea, nsRect& aInnerRect) {
> 
> Nit: "{" on new line.
> 
> ::: layout/generic/nsFrame.cpp:7953
> (Diff revision 3)
> > +      nsRect r;
> > +      // If its sibling is a SVGContainer, we need to process its children,
> > +      // and get its computed outline rect for unioning rect.
> > +      if (frameForArea->IsFrameOfType(nsIFrame::eSVGContainer)) {
> > +        ComputeSVGContainerOutlineArea(frameForArea, aInnerRect);
> > +        r = *frameForArea->Properties().Get(nsIFrame::OutlineInnerRectProperty());
> 
> I don't understand why we do this.  Haven't we already accumulated the
> outline for the descendants of this SVG conatiner frame in aInnerRect, with
> the call on the previous line?
> 
> ::: layout/generic/nsFrame.cpp:7994
> (Diff revision 3)
> > +  // When the frame is SVGContainer, we don't want a really wide
> > +  // outline if the block inside the inline is narrow, so union the
> > +  // actual contents of the anonymous blocks.
> 
> I don't think this comment makes sense.  "block inside inline" refers to
> cases like this:
> 
>   <style>
>     #x { display: block; }
>   </style>
>   <span>abc<span id=x>def</span>ghi</span>
> 
> We should mention in the comment why the computed outline area will be
> larger than we want when we have an SVG container frame.  (The reason being
> that SVG container frames do not maintain an accurate mRect.)
> 
> So how about something like this:
> 
> // SVG container frames do not maintain an accurate mRect, so we need to
> // compute their outline areas specially.
> 
> Nit: maybe put the comment inside the |if| statement, since the comment for
> the other branch is inside the |else| statement.

Comment 28

[Cameron McCormack (:heycam)]

Comment on attachment 8774544 [details]
Bug 1281215 - Making the outline of SVG container narrows to its children;

https://reviewboard.mozilla.org/r/66982/#review64960

r=me with these comments addressed, and let's make sure to have these tricky SVG container cases tested in the bug 778654 reftests.

::: layout/generic/nsFrame.cpp:7848
(Diff revision 4)
>   * in aFrame's coordinate space (if aApplyTransform is false) or its
>   * post-transform coordinate space (if aApplyTransform is true).
>   */
>  static nsRect
>  UnionBorderBoxes(nsIFrame* aFrame, bool aApplyTransform,
> +                 bool &outValid,

Nit: call this "aOutValid", and put the "&" next to the type.

Can you extend the comment above to say that aOutValid is set to false if the returned nsRect is not valid and should not be included in the outline rectangle?

::: layout/generic/nsFrame.cpp:7858
(Diff revision 4)
> +  if(aFrame->IsFrameOfType(nsIFrame::eSVGContainer)) {
> +    outValid = false;
> +  } else {
> +    outValid = true;
> +  }

Just simplify this to:

aOutValid = !aFrame->IsFrameOfType(...);

::: layout/generic/nsFrame.cpp:7905
(Diff revision 4)
>    nsRect clipPropClipRect;
>    bool hasClipPropClip =
>      aFrame->GetClipPropClipRect(disp, effects, &clipPropClipRect,
>                                  bounds.Size());
>  
> +  bool validRect = true;

Since this gets overwritten for each frame, let's declare it closer to where it is used (just above the UnionBorderBoxes call).

::: layout/generic/nsFrame.cpp:7945
(Diff revision 4)
> +      // If a SVGContainer has a non-SVGContainer child, we assign
> +      // its child's outline to this SVGContainer directly.
> +      if (!outValid && validRect) {
> +        u = childRect;
> +        outValid = true;
> +      }
> +      else {
> -      u.UnionRectEdges(u, childRect);
> +        u.UnionRectEdges(u, childRect);
> -    }
> +      }

Don't we need to skip the UnionRectEdges call if validRect is false?  How about we just do

  if (validRect) {
    continue;
  }

after the UnionBorderBoxes call.

::: layout/generic/nsFrame.cpp:7951
(Diff revision 4)
> +      // its child's outline to this SVGContainer directly.
> +      if (!outValid && validRect) {
> +        u = childRect;
> +        outValid = true;
> +      }
> +      else {

Nit: "else {" on the previous line.

Comment 29

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8774544 [details]
Bug 1281215 - Making the outline of SVG container narrows to its children;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66982/diff/4-5/

Comment 30

[Daosheng Mu[:daoshengmu]]

https://reviewboard.mozilla.org/r/66982/#review64960

> Don't we need to skip the UnionRectEdges call if validRect is false?  How about we just do
> 
>   if (validRect) {
>     continue;
>   }
> 
> after the UnionBorderBoxes call.

Yes, we should. thanks!

Comment 31

[Daosheng Mu[:daoshengmu]]

Please help land attachment 8774544 [details] to m-c. Thanks.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61c513b90c50
Make the outline of SVG container narrows to its children. r=heycam

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/61c513b90c50