Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Description • 10 years ago

In the attached test case the result of calling getBBox on the path with fill:none returns an empty rectangle.

The cause appears to be bug 999964 which introduced the SVGBoundingBoxOptions parameter to getBBox. The default values of the SVGBoundingBoxOptions parameters are:

  bool fill = true;
  bool stroke = false;
  bool markers = false;
  bool clipped = false;

The algorithm for calculating the bounding box clearly indicates that:

  "Note that the values of the ‘opacity’, ‘visibility’, ‘fill’, ‘fill-opacity’, ‘fill-rule’, ‘stroke-dasharray’ and ‘stroke-dashoffset’ properties on an element have no effect on the bounding box of an element."
(https://svgwg.org/svg2-draft/coords.html#BoundingBoxes)

Specifically we have:

 "If fill is true, then set box to the tightest rectangle in the coordinate system space that contains fill-shape."

And the fill-shape is unaffected by the value of the fill property.

However, in our implementation of this, in SVGTransformableElement::GetBBox we test the 'fill' value of the options object and if it is set we add the "nsSVGUtils::eBBoxIncludeFill" flag which is dependent on the value of fill. We should, however, use "nsSVGUtils::eBBoxIncludeFillGeometry" which is independent of the value of the fill property. (See nsSVGPathGeometryFrame::GetBBoxContribution)

I suggest we need a lot more tests to check we cover all the cases described in that algorithm.

Note that this only affects Nightly and Aurora builds since this behaviour is preffed off for release builds.

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 1 • 10 years ago

We need to turn off the changes to getBBox until this issue is fixed since it
breaks content. The next uplift is next week and I don't think we're going to
get a fix landed before then so we should turn it off now.

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 2 • 10 years ago

Turned off changes to getBBox for now: https://hg.mozilla.org/integration/mozilla-inbound/rev/122b37a8fa4f

	Ed Morley [:emorley]
	Comment 3 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/122b37a8fa4f

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 4 • 10 years ago

Tsukihashi-san, do you have any time to look at this?

	Shigeyuki Tsukihashi
	Comment 5 • 10 years ago

(In reply to Brian Birtles (:birtles) from comment #4)
> Tsukihashi-san, do you have any time to look at this?

Brian-san, I'm sorry but I am busy with work of Konno-san until middle of next month.
And I told Konno-san that "Please make the specification clear".

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 6 • 10 years ago

(In reply to Shigeyuki Tsukihashi from comment #5)
> (In reply to Brian Birtles (:birtles) from comment #4)
> > Tsukihashi-san, do you have any time to look at this?
> 
> Brian-san, I'm sorry but I am busy with work of Konno-san until middle of
> next month.
> And I told Konno-san that "Please make the specification clear".

Ok. What is unclear about the specification?

	Shigeyuki Tsukihashi
	Comment 7 • 10 years ago

Brian-san, the specification means requirement specification from Konno-san to me.
As you know, I'm not good at English and it's difficult for me to check and modify all GetBBoxContribution function.

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 9 • 10 years ago

Comment on attachment 8447734 [details] [diff] [review]
bounds.txt

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

This patch is definitely an improvement but I think there are other bugs in the existing code/tests that we should address before turning this back on.

For example, should the following line be FillGeometry as well?

  http://hg.mozilla.org/mozilla-central/file/c3387c5ddba9/layout/svg/nsSVGUtils.cpp#l921

I don't think we have any tests (in getBBox-method-helper.svg) for clipPaths where the clip path content has fill="none" so I don't think this case is currently tested.

::: content/svg/content/src/SVGTransformableElement.cpp
@@ +214,5 @@
>      if (aOptions.mClipped) {
>        aFlags |= nsSVGUtils::eBBoxIncludeClipped;
>      }
>      if (aFlags == 0) {
> +      return NS_NewSVGRect(this);

I'm probably missing something here, but in SVGRect.cpp in mozilla-central I don't see the single-argument version of NS_NewSVGRect.

@@ +219,4 @@
>      }
>      if (aFlags == nsSVGUtils::eBBoxIncludeMarkers || 
>          aFlags == nsSVGUtils::eBBoxIncludeClipped) {
> +      aFlags |= nsSVGUtils::eBBoxIncludeFillGeometry;

I'm having trouble lining this up with the algorithm here:
https://svgwg.org/svg2-draft/coords.html#BoundingBoxes

Do you know why this is needed?

Looking through the tests in test_getBBox-method.html we do have one set of tests for stroke:false, fill:false, markers:true but they seem to be testing that the stroke/fill is included so the tests may be wrong.

For *:false, clipped: true, we *appear* to have some tests for that for <text> but actually we don't since we never pass the opt object anywhere (and even if we did pass it to compareBBox2 it probably wouldn't check this case correctly. And while I'm being critical, we could probably find better names than compareBBox1 and compareBBox2):
  http://hg.mozilla.org/mozilla-central/file/c3387c5ddba9/content/svg/content/test/test_getBBox-method.html#l86

Looking at the bounding box algorithm it looks like we should be doing the intersection between rect(0,0,0,0) and the clipping path in this case which should probably give us rect(0,0,0,0).

::: content/svg/content/test/test_bbox.xhtml
@@ +61,5 @@
>    checkBBox("v", 95, 45, 10, 155, 0.001);
>    checkBBox("h", 195, 45, 105, 55, 0.001);
>    checkBBox("e", 95, 95, 10, 10, 0.001);
> +
> +  checkBBox("none", 0, 0, 10, 10, 0.001);

It seems to me like we have test_bbox.html (with bbox-helper.svg) and test_getBBox-method.html (with getBBox-method-helper.svg) and, unless I'm missing something, they're doing pretty much the same thing. If that's the case, we really should merge these (and name it either test_bbox.html and test_getBBox.html, the '-method' part seems superfluous).

While we're at it, we really should rename the test elements to something more descriptive than "v", "b", "e", "h", "i" etc.

That could be a separate patch though. For this patch, test_getBBox-method.html seems like the more appropriate place to add test cases.

	Robert Longson [:longsonr] Assignee
	Comment 10 • 10 years ago

(In reply to Brian Birtles (:birtles) from comment #9)
> 
> This patch is definitely an improvement but I think there are other bugs in
> the existing code/tests that we should address before turning this back on.

I can remove the turning on code.

> 
> For example, should the following line be FillGeometry as well?
> 
>  
> http://hg.mozilla.org/mozilla-central/file/c3387c5ddba9/layout/svg/
> nsSVGUtils.cpp#l921

Maybe, I was only trying to address the regression aspect of the code though.

> 
> I don't think we have any tests (in getBBox-method-helper.svg) for clipPaths
> where the clip path content has fill="none" so I don't think this case is
> currently tested.

Indeed, but I'm not completely sure how that should work. I suspect that in that
case fill="none" should work normally.


> 
> I'm probably missing something here, but in SVGRect.cpp in mozilla-central I
> don't see the single-argument version of NS_NewSVGRect.

The parameters I removed default to 0.

> 
> I'm having trouble lining this up with the algorithm here:
> https://svgwg.org/svg2-draft/coords.html#BoundingBoxes
> 
> Do you know why this is needed?

FillGeometry ignores fill="none". Is that what you meant?

> 
> Looking at the bounding box algorithm it looks like we should be doing the
> intersection between rect(0,0,0,0) and the clipping path in this case which
> should probably give us rect(0,0,0,0).
> 

Either this bug should remain open or a follow up raised to track that.

> 
> It seems to me like we have test_bbox.html (with bbox-helper.svg) and
> test_getBBox-method.html (with getBBox-method-helper.svg) and, unless I'm
> missing something, they're doing pretty much the same thing. If that's the
> case, we really should merge these (and name it either test_bbox.html and
> test_getBBox.html, the '-method' part seems superfluous).

One case test the original bbox call without arguments and is always run with whatever the pref is,
the other tests the extended argument functionality and runs with the pref set. If we remove
the pref the code should be merged.

> 
> While we're at it, we really should rename the test elements to something
> more descriptive than "v", "b", "e", "h", "i" etc.
> 
> That could be a separate patch though. For this patch,
> test_getBBox-method.html seems like the more appropriate place to add test
> cases.

We need a test to run whatever the pref is so we don't get regressions.

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 11 • 10 years ago

(In reply to Robert Longson from comment #10)
> (In reply to Brian Birtles (:birtles) from comment #9)
> > I don't think we have any tests (in getBBox-method-helper.svg) for clipPaths
> > where the clip path content has fill="none" so I don't think this case is
> > currently tested.
> 
> Indeed, but I'm not completely sure how that should work. I suspect that in
> that
> case fill="none" should work normally.

Looking at that code though, we're using the painted fill bbox to work out the extent of the clip area. I don't know why we're doing that but it seems wrong if we're then going to intersect it with the fill geometry bbox or stroke bbox. At a glance it seems like the values of 'bbox' would give us what we are looking for.

> > I'm having trouble lining this up with the algorithm here:
> > https://svgwg.org/svg2-draft/coords.html#BoundingBoxes
> > 
> > Do you know why this is needed?
> 
> FillGeometry ignores fill="none". Is that what you meant?

No, I was referring to the existing code. Why do we force the bbox to include the fill area when markers/clipped is set. That doesn't seem right. That's also what the following comments were about.

> > Looking at the bounding box algorithm it looks like we should be doing the
> > intersection between rect(0,0,0,0) and the clipping path in this case which
> > should probably give us rect(0,0,0,0).
> > 
> 
> Either this bug should remain open or a follow up raised to track that.

I'd rather just fix all this at once.

> > It seems to me like we have test_bbox.html (with bbox-helper.svg) and
> > test_getBBox-method.html (with getBBox-method-helper.svg) and, unless I'm
> > missing something, they're doing pretty much the same thing. If that's the
> > case, we really should merge these (and name it either test_bbox.html and
> > test_getBBox.html, the '-method' part seems superfluous).
> 
> One case test the original bbox call without arguments and is always run
> with whatever the pref is,
> the other tests the extended argument functionality and runs with the pref
> set. If we remove
> the pref the code should be merged.

But these aren't reftests, they're mochitests, so when the pref it set, we can skip just the parts of the test that require the pref (or even just turn it on using setPrefEnv).

But, again, that can be a separate patch.

	Robert Longson [:longsonr] Assignee
	Comment 12 • 10 years ago

(In reply to Brian Birtles (:birtles) from comment #11)
> 
> Looking at that code though, we're using the painted fill bbox to work out
> the extent of the clip area. I don't know why we're doing that but it seems
> wrong if we're then going to intersect it with the fill geometry bbox or
> stroke bbox. At a glance it seems like the values of 'bbox' would give us
> what we are looking for.

So if you have a shape that's clipped and inside the clipPath there's a rect and
that rect has fill="none" then it will clip the whole path visually. Should
it result in an empty bounding box for the shape or not?

> I'd rather just fix all this at once.

OK, I'll look into that.

	Brian Birtles (:birtles, Feb 26~ parental leave) Reporter
	Comment 13 • 10 years ago

(In reply to Robert Longson from comment #12)
> (In reply to Brian Birtles (:birtles) from comment #11)
> > 
> > Looking at that code though, we're using the painted fill bbox to work out
> > the extent of the clip area. I don't know why we're doing that but it seems
> > wrong if we're then going to intersect it with the fill geometry bbox or
> > stroke bbox. At a glance it seems like the values of 'bbox' would give us
> > what we are looking for.
> 
> So if you have a shape that's clipped and inside the clipPath there's a rect
> and
> that rect has fill="none" then it will clip the whole path visually. Should
> it result in an empty bounding box for the shape or not?

Yes, I believe it should. The shape of the clipping path is independent of any fill/stroke properties:

 "The raw geometry of each child element exclusive of rendering properties such as ‘fill’, ‘stroke’, ‘stroke-width’ within a ‘clipPath’ conceptually defines a 1-bit mask (with the possible exception of anti-aliasing along the edge of the geometry) which represents the silhouette of the graphics associated with that element." (http://www.w3.org/TR/2012/WD-css-masking-20121115/#ClipPathElement)

So whether or not the rect has fill="none" or fill="white", the result is the same.

> > I'd rather just fix all this at once.
> 
> OK, I'll look into that.

Thanks Robert! If you need help let me know. As far as I can tell, the bulk of the work is just in writing the test cases. The logic itself is probably not too complex.

(And while we're at it, I think we could probably tidy up some of that logic. Some variables like isOk could probably be a bit more descriptive and the following block could be collapsed:

  } else {
    if (hasClip) {
      bbox = bbox.Intersect(clipRect);
    }
  }
  (From: http://hg.mozilla.org/mozilla-central/file/c3387c5ddba9/layout/svg/nsSVGUtils.cpp#l959)

	Jonathan Watt [:jwatt]
	Comment 14 • 9 years ago

Any progress on this Robert?

	Robert Longson [:longsonr] Assignee
	Comment 15 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01b9b81c130a

	Robert Longson [:longsonr] Assignee
	Comment 17 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a419450c1346

	Sebastian Zartner [:sebo]
	Comment 18 • 8 years ago

Any progress on this since the last try push?

Sebastian

	Robert Longson [:longsonr] Assignee
	Comment 19 • 8 years ago

no, I'm happy to assign it to you if you want it.

	Sebastian Zartner [:sebo]
	Comment 20 • 8 years ago

Thank you for the fast feedback! I was not planning to work on this, just asking for the status.
If you're not working on this anymore, you should remove yourself as assignee, so somebody else can take it over.

Sebastian

	BugBot [:suhaib / :marco/ :calixte]
	Comment 21 • 6 years ago

The leave-open keyword is there and there is no activity for 6 months.
:svoisen, maybe it's time to close this bug?