getBBox returns incorrect results with empty containers

VERIFIED FIXED in Firefox 14

Status

()

VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: longsonr, Assigned: longsonr)

Tracking

({regression, testcase})

Trunk
mozilla14
regression, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox11 unaffected, firefox12+ unaffected, firefox13+ unaffected, firefox14+ verified, firefox-esr10 unaffected)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 606154 [details]
testcase
(Assignee)

Updated

7 years ago
Keywords: regression
(Assignee)

Updated

7 years ago
status-firefox-esr10: --- → unaffected
status-firefox11: --- → unaffected
status-firefox12: --- → affected
status-firefox13: --- → affected
status-firefox14: --- → affected
(Assignee)

Updated

7 years ago
Keywords: testcase
(Assignee)

Updated

7 years ago
Depends on: 647914
(Assignee)

Comment 1

7 years ago
Created attachment 606160 [details] [diff] [review]
patch
Assignee: nobody → longsonr
Attachment #606160 - Flags: review?(jwatt)
Comment on attachment 606160 [details] [diff] [review]
patch

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

::: content/svg/content/test/bbox-helper.svg
@@ +13,5 @@
>    <g id="h">
>      <circle cx="200" cy="50" r="5"/>
>      <path d="M 200,100 L 300,100"/>
>    </g>
> +  <g id="e">

Please add this here:

  <!-- empty container should not affect parent's bbox -->

::: layout/svg/base/src/nsSVGContainerFrame.cpp
@@ +262,5 @@
>      nsISVGChildFrame* svgKid = do_QueryFrame(kid);
>      if (svgKid) {
>        gfxMatrix transform = aToBBoxUserspace;
>        nsIContent *content = kid->GetContent();
> +      if (content->IsSVG()) {

Ah, yes, that would be rather a redundant check.
Attachment #606160 - Flags: review?(jwatt) → review+
(Assignee)

Updated

7 years ago
Flags: in-testsuite+
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/2205ed382ab0
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
status-firefox14: affected → ---

Updated

7 years ago
Blocks: 647914
No longer depends on: 647914
Hmm, change:

+  <g id="e">
+    <g/>
+    <circle cx="100" cy="100" r="5"/>
+  </g>

to:

+  <g id="e">
+    <g/>
+    <g/>
+    <circle cx="100" cy="100" r="5"/>
+  </g>

and the test will fail, right?
Err, I mean:

+  <g id="e">
+    <g/>
+    <circle cx="100" cy="100" r="5"/>
+    <g/>
+  </g>
(Assignee)

Comment 7

7 years ago
Created attachment 606988 [details] [diff] [review]
patch 2
Attachment #606988 - Flags: review?(jwatt)
This raises the question of whether or not a zero length path that still draws a circle/square due to linecap should contribute to the bbox or not. It seems that if the bbox is supposed to roughly be the bounds of the objects that paint something, then it should. (I think it should.) If so, then it would be better to also check that x and y are zero too, something like this:

  if (childBBox == gfxRect(0,0,0,0)) {
    // Assume the child is an empty container, and ignore it. There's a tiny chance
    // that someone someday will have a zero length path with a linecap that they
    // want included in the bbox that happens to be positioned exactly at
    // x=0,y=0, but that's going to be a very rare case.
    continue;
  }

Also please have the test do the following to more thoroughly test this:

+  <g id="e">
+    <g/>
+    <circle cx="100" cy="100" r="5"/>
+    <g/>
+    <circle cx="100" cy="100" r="5"/>
+    <g/>
+  </g>

And please add a test to make sure that zero length path with a linecap _does_ contribute.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
In fact the existing comment could do with some rewording, and breaking at 80 chars. If you rename |firstChild| to |firstChildWithBBox|, the code should look like this I think:

      gfxRect childBBox = svgKid->GetBBoxContribution(transform, aFlags);

      if (childBBox == gfxRect(0,0,0,0)) {
        // Assume the child is an empty container, and ignore it. There's a
        // tiny chance that someone someday will want a zero length path with a
        // linecap that happens to be positioned exactly at x=0,y=0 to be
        // included in a bbox, but that's going to be very rare since zero
        // length paths are themselves rare.
        continue;
      }
      // We need to include zero width/height vertical/horizontal lines, so we
      // have to use UnionEdges below, but that means that we must special case
      // the first bbox so that we don't include the initial gfxRect(0,0,0,0)
      // value of bboxUnion.
      if (firstChildWithBBox) {
        bboxUnion = childBBox;
        firstChildWithBBox = false;
        continue;
      }
      bboxUnion = bboxUnion.UnionEdges(childBBox);
Actually, I guess that second comment should change to something more like:

      // We need to include zero width/height vertical/horizontal lines, and
      // zero length paths that might have linecaps, so we have to use
      // UnionEdges below. That means that we must special case the first bbox
      // so that we don't include the initial gfxRect(0,0,0,0) value of
      // bboxUnion.
(Assignee)

Comment 11

7 years ago
(In reply to Jonathan Watt [:jwatt] from comment #8)
> This raises the question of whether or not a zero length path that still
> draws a circle/square due to linecap should contribute to the bbox or not.
> It seems that if the bbox is supposed to roughly be the bounds of the
> objects that paint something, then it should. (I think it should.) If so,
> then it would be better to also check that x and y are zero too, something
> like this:
> 

I decided to fix zero lengh path bounds properly in another bug.
(Assignee)

Comment 12

7 years ago
Created attachment 607225 [details] [diff] [review]
patch
Attachment #606988 - Attachment is obsolete: true
Attachment #606988 - Flags: review?(jwatt)
Attachment #607225 - Flags: review?(jwatt)
(Assignee)

Comment 13

7 years ago
(In reply to Jonathan Watt [:jwatt] from comment #8)
> This raises the question of whether or not a zero length path that still
> draws a circle/square due to linecap should contribute to the bbox or not.
> It seems that if the bbox is supposed to roughly be the bounds of the
> objects that paint something, then it should. (I think it should.) If so,
> then it would be better to also check that x and y are zero too, something
> like this:
> 
>   if (childBBox == gfxRect(0,0,0,0)) {
>     // Assume the child is an empty container, and ignore it. There's a tiny
> chance
>     // that someone someday will have a zero length path with a linecap that
> they
>     // want included in the bbox that happens to be positioned exactly at
>     // x=0,y=0, but that's going to be a very rare case.
>     continue;
>   }
> 

Zero length subpaths don't have zero height and width unless they have no stroke in which case they aren't painted and don't count. So with or without my multiple zero length subpath bugfix we don't need to account for them here.
(Assignee)

Comment 14

7 years ago
(In reply to Robert Longson from comment #13)
> Zero length subpaths don't have zero height and width unless they have no
> stroke in which case they aren't painted and don't count. So with or without
> my multiple zero length subpath bugfix we don't need to account for them
> here.

Of course this is getBBox so stroke doesn't count. So the question is does the fill of a zero length subpath count towards getBBox and I'd argue it doesn't on its own as only the stroke makes it visible.
(Assignee)

Comment 15

7 years ago
(In reply to Jonathan Watt [:jwatt] from comment #9)
> 
>       gfxRect childBBox = svgKid->GetBBoxContribution(transform, aFlags);
> 
>       if (childBBox == gfxRect(0,0,0,0)) {
>         // Assume the child is an empty container, and ignore it. There's a
>         // tiny chance that someone someday will want a zero length path
> with a
>         // linecap that happens to be positioned exactly at x=0,y=0 to be
>         // included in a bbox, but that's going to be very rare since zero
>         // length paths are themselves rare.

At the moment and with/without my other patch in the other bug you are reviewing all zero length paths return (0,0,0,0) wherever they are located as we only count such paths when there is a stroke and we're not ignoring the stroke here.
status-firefox14: --- → affected
tracking-firefox12: --- → +
tracking-firefox13: --- → +
tracking-firefox14: --- → +
I backed out bug 647914 on aurora (firefox12) & beta (firefox13), so those are now [unaffected] by this bug here.  Setting status flags accordingly.
status-firefox12: affected → unaffected
status-firefox13: affected → unaffected
(In reply to Robert Longson from comment #14)
> Of course this is getBBox so stroke doesn't count. So the question is does
> the fill of a zero length subpath count towards getBBox and I'd argue it
> doesn't on its own as only the stroke makes it visible.

Hmm. getBBox is supposed to give the user a rough approximation of the bounds of the area painted by a given part of the graphic. The only reason it's based only on path bounds is because the spec authors (correctly) worried about implementation performance if it had to return the exact paint bounds, taking stroke, dashing, clipping, masking, filtering, markers etc. into account. So while the line of what implementations should take into account was drawn at "just the path bounds", it is still meant to be API to get an approximation of the paint bounds. Given that, I definitely think that zero size objects and zero length [sub]paths should contribute to getBBox if they result in paint being laid down due to stroke.
I'm kind of thinking that it's a mistake that GetBBoxContribution returns a gfxRect. Seems like it should probably return an instance of something like the following so that we can distinguish between "zero size" and "no contribution":

struct SVGBBox {
  gfxRect bbox;
  bool hasBBox;
}

Or something like that. What do you think?
(Assignee)

Comment 19

7 years ago
Created attachment 615055 [details] [diff] [review]
address review comments
Attachment #607225 - Attachment is obsolete: true
Attachment #607225 - Flags: review?(jwatt)
Attachment #615055 - Flags: review?(jwatt)
Comment on attachment 615055 [details] [diff] [review]
address review comments

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

I'm much happier with this, thanks a lot Robert! r=me, with a few comments.

::: content/svg/content/test/bbox-helper.svg
@@ +17,5 @@
>    <g id="e">
>      <!-- empty container should not affect parent's bbox -->
>      <g/>
>      <circle cx="100" cy="100" r="5"/>
> +    <g/>

Can you add an extra:

    <circle cx="100" cy="100" r="5"/>
    <g/>

Just so that there's an empty container between two shapes with valid bbox too?

::: layout/svg/base/src/nsSVGMarkerFrame.cpp
@@ +182,5 @@
>                                            PRUint32 aFlags,
>                                            nsSVGPathGeometryFrame *aMarkedFrame,
>                                            const nsSVGMark *aMark,
>                                            float aStrokeWidth)
>  {

To get NRVO you could declare |SVGBBox bbox;| here, and return it at the early return paints. Up to you.

@@ +215,1 @@
>    bool firstChild = true;

firstChild can die.

::: layout/svg/base/src/nsSVGPathGeometryFrame.cpp
@@ +265,3 @@
>  nsSVGPathGeometryFrame::GetBBoxContribution(const gfxMatrix &aToBBoxUserspace,
>                                              PRUint32 aFlags)
>  {

Again, you could put |SVGBBox bbox;| here and return that from the early return points.

@@ +266,5 @@
>                                              PRUint32 aFlags)
>  {
>    if (aToBBoxUserspace.IsSingular()) {
>      // XXX ReportToConsole
>      return gfxRect(0.0, 0.0, 0.0, 0.0);

In fact, you want to return an explicit SVGBBox here so that this doesn't contribute.

::: layout/svg/base/src/nsSVGUtils.cpp
@@ -1544,3 @@
>    }
> -  NS_ASSERTION(bbox.Width() >= 0.0 && bbox.Height() >= 0.0, "Invalid bbox!");
> -  return bbox;

I think the idea here was NRVO, but not a big deal.

::: layout/svg/base/src/nsSVGUtils.h
@@ +152,5 @@
> +  SVGBBox() 
> +    : mIsEmpty(true) {}
> +
> +  SVGBBox(const gfxRect& aRect) 
> +    : mBBox(aRect), mIsEmpty(false) {}

Some folks might prefer that this be |explicit|, but I think it's okay in this case.
Attachment #615055 - Flags: review?(jwatt) → review+
Oh, and for the comment documenting SVGBBox, can you make it:

/**
 * ...
 */

instead of:

/*
 *
 */

so that it gets picked up by doxygen properly?
(Assignee)

Comment 22

7 years ago
Created attachment 615178 [details] [diff] [review]
final review comments addressed
Attachment #615055 - Attachment is obsolete: true

Comment 24

7 years ago
https://hg.mozilla.org/mozilla-central/rev/01fd11649cc8
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED

Updated

7 years ago
status-firefox14: affected → ---
status-firefox14: --- → fixed
The square from the testcase looks OK on FF 14b9 on Win 7, Ubuntu 12.04 and Mac OS X 10.6. Verified fixed.
Status: RESOLVED → VERIFIED
status-firefox14: fixed → verified
You need to log in before you can comment on or make changes to this bug.