Last Comment Bug 647914 - Horizontal and vertical SVG paths are omitted from bbox calculations if they have siblings
: Horizontal and vertical SVG paths are omitted from bbox calculations if they ...
Status: VERIFIED FIXED
[qa!]
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Robert Longson
:
Mentors:
Depends on: 641426 736031 777476
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-05 17:15 PDT by Pap Lorinc
Modified: 2012-07-25 15:57 PDT (History)
12 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
affected
verified
verified


Attachments
patch (4.02 KB, patch)
2012-01-20 16:33 PST, Robert Longson
jwatt: review+
Details | Diff | Splinter Review
backout on aurora (4.69 KB, patch)
2012-03-19 11:44 PDT, Daniel Holbert [:dholbert]
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
backout on beta (4.64 KB, patch)
2012-03-19 11:46 PDT, Daniel Holbert [:dholbert]
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Screenshot of test case display in Firefox (38.97 KB, image/png)
2012-05-16 05:42 PDT, Virgil Dicu [:virgil] [QA]
no flags Details
displays as expected in trunk for me. (1.35 KB, image/png)
2012-05-16 07:10 PDT, Robert Longson
no flags Details
comment 0 testcase (1.48 KB, text/html)
2012-05-16 10:06 PDT, Robert Longson
no flags Details
comment 0 testcase with width/height of outer SVG fixed (1.51 KB, text/html)
2012-05-24 09:17 PDT, Robert Longson
no flags Details

Description Pap Lorinc 2011-04-05 17:15:25 PDT
User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0) Gecko/20100101 Firefox/4.0

If an element has anything else besides a perfectly horizontal or vertical line, the line will be ignored from the bounds calculation.
The height of the displayed svg element is wrong too;

The following demonstrates the problem (the first and third box should look like the second and forth (they are not perfectly horizontal and vertical)):

<html>
  <head>
    <script type="text/javascript">
	function load()
	{
      var bounds = document.getElementById("line1").getBBox();
	  rect(bounds.x, bounds.y, bounds.width, bounds.height);

      bounds = document.getElementById("line2").getBBox();
	  rect(bounds.x, bounds.y, bounds.width, bounds.height);

      bounds = document.getElementById("line3").getBBox();
	  rect(bounds.x, bounds.y, bounds.width, bounds.height);

      bounds = document.getElementById("line4").getBBox();
	  rect(bounds.x, bounds.y, bounds.width, bounds.height);
	}
	function rect(x, y, w, h)
	{
		var shape = document.createElementNS('http://www.w3.org/2000/svg', 'rect');

		shape.setAttribute("x", x-1);
		shape.setAttribute("y", y-1);
		shape.setAttribute("width",  w+2);
		shape.setAttribute("height", h+2);
		shape.setAttribute("style", "stroke:black; fill:none");

		document.getElementsByTagName("svg")[0].appendChild(shape);
	}
    </script>
  </head>
  <body onload="load();">
<svg>
<g id="line1"><circle cx="100" cy="50" r="5"/><path style="stroke:black;stroke-width:2" d="M 100,100 L 100,200"/></g>
<g id="line2"><circle cx="200" cy="50" r="5"/><path style="stroke:black;stroke-width:2" d="M 200,100 L 200.01,200"/></g>
<g id="line3"><circle cx="300" cy="50" r="5"/><path style="stroke:black;stroke-width:2" d="M 300,100 L 400,100"/></g>
<g id="line4"><circle cx="500" cy="50" r="5"/><path style="stroke:black;stroke-width:2" d="M 500,100 L 600,100.01"/></g>
</svg>
</body>
</html>

Reproducible: Always




Workaround:
as demonstrated above, if the line is not perfectly horizontal or vertical then the bounds are correct;

A quick fix would be welcome. Chrome does the same, Opera is correct again.

Thanks.
Comment 1 Boris Zbarsky [:bz] 2011-04-05 17:37:33 PDT
nsSVGDisplayContainerFrame::GetBBoxContribution does this:

  bboxUnion = bboxUnion.Union(svgKid->GetBBoxContribution(transform));

where bboxUnion is a gfxRect.  But gfxRect::Union ignores empty rectangles:

61 gfxRect
62 gfxRect::Union(const gfxRect& aRect) const
63 {
64   if (IsEmpty())
65     return aRect;
66   if (aRect.IsEmpty())
67     return *this;

which is broken in this case.  nsRect has a UnionRectIncludeEmpty which can be used in cases like this.  Once bug 641426 lands, this will be available on gfxRect.
Comment 2 Anna [:annasob] 2011-04-06 09:10:02 PDT
Boris, 
From you last comment I am gathering that this bug is blocked by bug 641426 is this correct? I am going to take a shot at this.
Comment 3 Boris Zbarsky [:bz] 2011-04-06 09:22:46 PDT
It's blocked in the sense that it will be much easier to fix once bug 641426 is fixed.  It could be fixed without that bug being fixed by doing some copy/pasting, but it seems better to just wait for bug 641426 to land.

> I am going to take a shot at this.

Thanks!
Comment 4 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2011-04-20 04:24:00 PDT
Anna, this is now unblocked. Let us know if you need any help.
Comment 5 Pap Lorinc 2011-07-05 00:26:31 PDT
Is anyone working on this?
Comment 6 Robert Longson 2012-01-20 16:33:15 PST
Created attachment 590386 [details] [diff] [review]
patch
Comment 7 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-01-23 02:11:21 PST
Comment on attachment 590386 [details] [diff] [review]
patch

r=jwatt, but please add a comment before the |gfxRect childBBox = | lines, something like:

  // We need to include zero width/height vertical/horizontal lines, so we have
  // to use UnionEdges, but we must special case the first bbox so that we don't
  // include the initial gfxRect(0,0,0,0).
Comment 9 Marco Bonardo [::mak] 2012-01-24 04:46:24 PST
https://hg.mozilla.org/mozilla-central/rev/e69ad50ec0d7
Comment 10 Robert Longson 2012-03-19 11:25:30 PDT
Daniel can you get this backed out of Mozilla12 and Mozilla13 please? See bug 736031 for why.
Comment 11 Daniel Holbert [:dholbert] 2012-03-19 11:44:21 PDT
Created attachment 607234 [details] [diff] [review]
backout on aurora

Per longsonr's previous comment above, we'd like to back this out on aurora & beta, due to a functional regression (bug 736031).  Requesting approval to backout.

[Approval Request Comment]
User impact if declined:
  Web content breakage, from bug 736031

Testing completed (on m-c, etc.):
  None (but this is a backout, so not really applicable.)

Risk to taking this patch (and alternatives if risky):
  Low (patch is a clean backout).  This bug here will be re-broken, of course, but that's not a big deal because this bug here is not a regression (or not a recent one, at least).

String changes made by this patch:
  None
Comment 12 Daniel Holbert [:dholbert] 2012-03-19 11:46:00 PDT
Created attachment 607236 [details] [diff] [review]
backout on beta

Here's the patch for backout on beta branch.

(This is a clean backout there as well -- no manual editing was required. This patch differs slightly from the aurora backout due to a few minor diffs to contextual code.)
Comment 13 Alex Keybl [:akeybl] 2012-03-20 13:08:52 PDT
Comment on attachment 607236 [details] [diff] [review]
backout on beta

[Triage Comment]
Clean backout to a known good state - approved for Beta 12 and Aurora 13.
Comment 14 Alex Keybl [:akeybl] 2012-03-20 13:09:35 PDT
We'll track bug 736031 for 12/13/14 - please mark the status bugs appropriately there when these patches have landed.
Comment 15 Daniel Holbert [:dholbert] 2012-03-20 14:12:10 PDT
Landed backout on aurora:
 https://hg.mozilla.org/releases/mozilla-aurora/rev/9fb6ba8e0535
and beta:
 https://hg.mozilla.org/releases/mozilla-beta/rev/1d6cfc01ed14

(Should I update "target-milestone" to mozilla14 here now, since this is no longer on mozilla12 or mozilla13? For now, I'll leave that field alone & just set the tracking flags on bug 736031.)
Comment 16 Robert Longson 2012-04-27 03:01:23 PDT
This issue isn't fixed in Firefox 12 or Firefox 13 as the patch was backed out there. It is fixed in 14 onwards though.
Comment 17 Virgil Dicu [:virgil] [QA] 2012-05-16 05:42:46 PDT
Created attachment 624356 [details]
Screenshot of test case display in Firefox

Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (X11; Linux i686; rv:15.0) Gecko/15.0 Firefox/15.0a1

Using the test case from comment 0, I can't see any difference between 13beta 3 and Nightly. Both seem to still display the issue.

Chrome and opera display two identical pairs of figures.

Can anyone advise here?
Comment 18 Robert Longson 2012-05-16 07:10:49 PDT
Created attachment 624373 [details]
displays as expected in trunk for me.
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-16 09:46:53 PDT
Virgil, can you retest this on Windows 7 64-bit as that's where this bug was originally reported?
Comment 20 Robert Longson 2012-05-16 10:06:23 PDT
Created attachment 624429 [details]
comment 0 testcase
Comment 21 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-16 10:42:14 PDT
Robert, can you please include the Firefox versions and platforms your screenshots apply to? It's hard to make a QA judgement without that information.
Comment 22 Robert Longson 2012-05-16 11:46:41 PDT
Screenshot is on Windows 7 with fairly up to date trunk build
Comment 23 Virgil Dicu [:virgil] [QA] 2012-05-24 08:39:45 PDT
(sorry for the delay - did not receive bugmail for this bug-don't know the reason)

Using the test case from comment 0 in latest Nightly I get the same output as in comment 18 (Linux, Windows XP and 7). Is this expected? 

Google Chrome and Opera display different output (as in comment 20).
Comment 24 Virgil Dicu [:virgil] [QA] 2012-05-24 08:42:04 PDT
Also, as the issue was backed out in F12 and F13 (comment 15 and 16), can the flags be updated to reflect that?

See comment 17 for test case display in Firefox 13 beta.
Comment 25 Robert Longson 2012-05-24 09:17:13 PDT
Created attachment 626845 [details]
comment 0 testcase with width/height of outer SVG fixed
Comment 26 Robert Longson 2012-05-24 09:24:59 PDT
The original testcase didn't have a width/height on the outer SVG. That displays differently in Firefox now due to bug 611099 being fixed. I've added an explicit width/height to take that out of the mix.

With this Opera 11.6 displays the same as Firefox Trunk for me. Firefox 14 should display this way too while Firefox <= 13 should display with the unfilled rectangles being too small, along the lines of comment 17.

Webkit had this bug too till https://bugs.webkit.org/show_bug.cgi?id=76177 was fixed. I don't know what version of Chrome that would be in though.
Comment 27 Virgil Dicu [:virgil] [QA] 2012-05-25 00:46:26 PDT
Thanks, Robert. That did the trick. 

Verified fixed on Firefox 14 (Aurora) and 15 (Nightly). Windows 7, Ubuntu 12.04 and Mac OS 10.6. The expected output is displayed-same as Chrome and Opera.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120524 Firefox/14.0a2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1

Firefox 12 and 13 still display the issue (backed out in comment 15). Modifying the tracking flags to reflect that.

Note You need to log in before you can comment on or make changes to this bug.