Closed Bug 647914 Opened 13 years ago Closed 12 years ago

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

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Tracking Flags:
Tracking Status
firefox12 --- affected
firefox13 --- affected
firefox14 --- verified
firefox15 --- verified

People

(Reporter: papuska, Assigned: longsonr)

References

Details

(Whiteboard: [qa!])

Attachments

(6 files, 1 obsolete file)

patch
4.02 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
backout on aurora
4.69 KB, patch
akeybl
: approval-mozilla-aurora+
Details | Diff | Splinter Review
backout on beta
4.64 KB, patch
akeybl
: approval-mozilla-beta+
Details | Diff | Splinter Review
Screenshot of test case display in Firefox
38.97 KB, image/png
Details
displays as expected in trunk for me.
1.35 KB, image/png
Details
comment 0 testcase with width/height of outer SVG fixed
1.51 KB, text/html
Details 
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]
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.
Assignee: annaopensource → nobody
Is anyone working on this?
Attached patch patch — — Splinter Review 
Assignee: nobody → longsonr

        Attachment #590386 -
        Flags: review?(jwatt)

    
  
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+

    
    

    
  
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
Closed: 12 years ago
Resolution: --- → FIXED

    
  
Blocks: 736031

    
  
No longer blocks: 736031
Depends on: 736031

    
    

    
  
Daniel can you get this backed out of Mozilla12 and Mozilla13 please? See bug 736031 for why.

    
    

    
  


  
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?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        backout on beta
        — 
          — Splinter Review
      

    


  
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+

    
  

        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 → ---

    
  
Whiteboard: [qa+]

    
    

    
  
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.

    
    

    
  


  
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?

    
    

    
  
Virgil, can you retest this on Windows 7 64-bit as that's where this bug was originally reported?

    
    

    
  

      
      
      
      

        Attached file
          
        comment 0 testcase (obsolete)
        — 
      

    


  

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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!]

    
  
Depends on: 777476

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: