1019326 - getBBox now returns an empty bbox when there is no fill

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles)]

Attachment: Test case

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.

Comment 1

[Brian Birtles (:birtles)]

Attachment: Turn off changes to SVG's getBBox due to

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.

Comment 2

[Brian Birtles (:birtles)]

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

Comment 3

[Ed Morley [:emorley]]

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

Comment 4

[Brian Birtles (:birtles)]

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

Comment 5

[Shigeyuki Tsukihashi]

(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".

Comment 6

[Brian Birtles (:birtles)]

(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?

Comment 7

[Shigeyuki Tsukihashi]

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.

Comment 8

[Robert Longson [:longsonr]]

Attachment: bounds.txt

Comment 9

[Brian Birtles (:birtles)]

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.

Comment 10

[Robert Longson [:longsonr]]

(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.

Comment 11

[Brian Birtles (:birtles)]

(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.

Comment 12

[Robert Longson [:longsonr]]

(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.

Comment 13

[Brian Birtles (:birtles)]

(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)

Comment 14

[Jonathan Watt [:jwatt]]

Any progress on this Robert?

Comment 15

[Robert Longson [:longsonr]]

Attachment: bbox.txt

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

Comment 16

[Robert Longson [:longsonr]]

Attachment: bbox.txt

Comment 17

[Robert Longson [:longsonr]]

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

Comment 18

[Sebastian Zartner [:sebo]]

Any progress on this since the last try push?

Sebastian

Comment 19

[Robert Longson [:longsonr]]

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

Comment 20

[Sebastian Zartner [:sebo]]

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

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 23

[Brian Birtles (:birtles)]

I'm pretty sure it's still a bug.

Comment 24

[Robert Longson [:longsonr]]

It sure is.

Comment 25

[Daniel Holbert [:dholbert]]

(Let's drop the leave-open keyword so that triagebot stops nagging about possibly-mistakenly-left-open status. That keyword really only matters for the short time window when a patch is being merged around.)

Comment 26

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 27

[Suhaib Mujahid [:suhaib]]

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.

Comment 28

[Robert Longson [:longsonr]]

Attachment: Bug 1019326 - fix some issues with svg.new-getBBox.enabled r=emilio

Comment 29

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/jobs?repo=try&revision=6ab7f934151c5fc2e239725a62fd48a4532a9616

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f3c000986ca
fix some issues with svg.new-getBBox.enabled r=emilio

Comment 31

[Sandor Molnar[:smolnar]]

Backed out for causing mochitest failures @ dom/svg/test/test_getBBox-options.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/efb862ccda78739d3329be790da0cb8700f19481

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | dom/svg/test/test_getBBox-options.html | Test timed out.

Comment 32

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/jobs?repo=try&revision=1eb1e103013b372ed1390d6acdd09c5cc13e3153

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2a99b40f516
fix some issues with svg.new-getBBox.enabled r=emilio

Comment 34

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/e2a99b40f516

Comment 35

[Robert Longson [:longsonr]]

svg.new-getBBox.enabled is still preffed off. I'll raise another bug to try turning it on again.

Comment 36

[Florin Bilt]

(In reply to Pulsebot from comment #33)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2a99b40f516
fix some issues with svg.new-getBBox.enabled r=emilio

Perfherder has detected a browsertime performance change from push e2a99b40f516d8e0dfff4b258f6b88f20b6f9642.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
4%	google-slides LastVisualChange	macosx1015-64-shippable-qr	bytecode-cached fission warm webrender	1,436.38 -> 1,375.84	Before/After
4%	google-slides LastVisualChange	macosx1015-64-shippable-qr	cold fission webrender	2,465.40 -> 2,363.07	Before/After
3%	google-slides LastVisualChange	macosx1015-64-shippable-qr	bytecode-cached cold fission webrender	2,485.44 -> 2,402.64	Before/After
3%	google-slides LastVisualChange	macosx1015-64-shippable-qr	fission warm webrender	1,935.56 -> 1,877.12	Before/After
3%	google-slides LastVisualChange	macosx1015-64-shippable-qr	fission warm webrender	1,932.26 -> 1,874.06	Before/After

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 688

For more information on performance sheriffing please see our FAQ.

Comment 37

[Robert Longson [:longsonr]]

Well that's pretty surprising and close to impossible since this is all preffed off code.