Horizontal and vertical SVG paths are omitted from bbox calculations if they have siblings

VERIFIED FIXED

Status

()

Core
SVG
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: Pap Lorinc, Assigned: Robert Longson)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox12 affected, firefox13 affected, firefox14 verified, firefox15 verified)

Details

(Whiteboard: [qa!])

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
Component: General → SVG
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → 2.0 Branch
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.
Status: UNCONFIRMED → NEW
Depends on: 641426
Ever confirmed: true
Version: 2.0 Branch → Trunk
Keywords: student-project
Whiteboard: [good first bug]

Comment 2

6 years ago
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.
Assignee: nobody → annaopensource
Status: NEW → ASSIGNED
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!
Anna, this is now unblocked. Let us know if you need any help.

Updated

6 years ago
Assignee: annaopensource → nobody
(Reporter)

Comment 5

6 years ago
Is anyone working on this?
(Assignee)

Comment 6

5 years ago
Created attachment 590386 [details] [diff] [review]
patch
Assignee: nobody → longsonr
Attachment #590386 - Flags: review?(jwatt)
(Assignee)

Updated

5 years ago
Keywords: student-project
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [good first bug]
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).
Attachment #590386 - Flags: review?(jwatt) → review+
(Assignee)

Comment 8

5 years ago
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/e69ad50ec0d7
Flags: in-testsuite+
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/e69ad50ec0d7
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Blocks: 736031

Updated

5 years ago
No longer blocks: 736031
Depends on: 736031
(Assignee)

Comment 10

5 years ago
Daniel can you get this backed out of Mozilla12 and Mozilla13 please? See bug 736031 for why.
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
Attachment #607234 - Flags: approval-mozilla-aurora?
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.)
Attachment #607236 - Flags: approval-mozilla-beta?
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.
Attachment #607236 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

5 years ago
Attachment #607234 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
We'll track bug 736031 for 12/13/14 - please mark the status bugs appropriately there when these patches have landed.
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.)
Target Milestone: mozilla12 → ---
status-firefox12: --- → fixed
status-firefox13: --- → fixed
Whiteboard: [qa+]
(Assignee)

Comment 16

5 years ago
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.
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?
(Assignee)

Comment 18

5 years ago
Created attachment 624373 [details]
displays as expected in trunk for me.
Virgil, can you retest this on Windows 7 64-bit as that's where this bug was originally reported?
(Assignee)

Comment 20

5 years ago
Created attachment 624429 [details]
comment 0 testcase
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.
(Assignee)

Comment 22

5 years ago
Screenshot is on Windows 7 with fairly up to date trunk build
(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).
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.
(Assignee)

Comment 25

5 years ago
Created attachment 626845 [details]
comment 0 testcase with width/height of outer SVG fixed
Attachment #624429 - Attachment is obsolete: true
(Assignee)

Comment 26

5 years ago
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.
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.
Status: RESOLVED → VERIFIED
status-firefox12: fixed → affected
status-firefox13: fixed → affected
status-firefox14: --- → verified
status-firefox15: --- → verified
Whiteboard: [qa+] → [qa!]

Updated

5 years ago
Depends on: 777476
You need to log in before you can comment on or make changes to this bug.